Bug 76519 - Use RenderTheme more in HTMLSelectElement Rather than ifdefs
Summary: Use RenderTheme more in HTMLSelectElement Rather than ifdefs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-18 00:08 PST by Jun Mukai
Modified: 2012-01-22 20:24 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.86 KB, patch)
2012-01-18 00:22 PST, Jun Mukai
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2012-01-18 00:59 PST, Jun Mukai
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2012-01-18 01:07 PST, Jun Mukai
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2012-01-18 01:08 PST, Jun Mukai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jun Mukai 2012-01-18 00:08:30 PST
Currently HTMLSelectElement changes the behavior based on the two #defines; ARROW_KEYS_POP_MENU and SPACE_OR_RETURN_POP_MENU.
We want to replace these definitions and ifdef rules by RenderTheme.
Comment 1 Jun Mukai 2012-01-18 00:22:40 PST
Created attachment 122881 [details]
Patch
Comment 2 Kent Tamura 2012-01-18 00:47:16 PST
Comment on attachment 122881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122881&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:985
> +        UNUSED_PARAM(event);

UNUSED_PARAM(event) is not needed. Please remove it.

> Source/WebCore/platform/gtk/RenderThemeGtk.h:85
> +    virtual bool popsMenuByArrowKeys() const { return true; }

Please add 'OVERRIDE' between const and {.

> Source/WebCore/rendering/RenderTheme.h:184
> +    // Method for <select> elements.
> +    virtual bool popsMenuByArrowKeys() const { return false; }
> +    virtual bool popsMenuBySpaceOrReturn() const { return false; }
> +

RenderTheme::delegatesMenuListRendering() is for <select> too.  Please move these declarations beside delegatesMenuListRendering().

> Source/WebCore/rendering/RenderThemeChromiumLinux.h:77
> +        virtual bool popsMenuBySpaceOrReturn() const { return true; }

Please add 'OVERRIDE' between const and {.

> Source/WebCore/rendering/RenderThemeMac.h:83
> +    virtual bool popsMenuByArrowKeys() const { return true; }

ditto.

> Source/WebCore/rendering/RenderThemeSafari.h:88
> +    virtual bool popsMenuByArrowKeys() const { return true; }
> +

You should not modify RenderThemeSafari, which is for Apple Windows port.
Comment 3 Jun Mukai 2012-01-18 00:59:58 PST
Created attachment 122885 [details]
Patch
Comment 4 Jun Mukai 2012-01-18 01:01:17 PST
Comment on attachment 122881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122881&action=review

>> Source/WebCore/html/HTMLSelectElement.cpp:985
>> +        UNUSED_PARAM(event);
> 
> UNUSED_PARAM(event) is not needed. Please remove it.

Done

>> Source/WebCore/platform/gtk/RenderThemeGtk.h:85
>> +    virtual bool popsMenuByArrowKeys() const { return true; }
> 
> Please add 'OVERRIDE' between const and {.

Done

>> Source/WebCore/rendering/RenderTheme.h:184
>> +
> 
> RenderTheme::delegatesMenuListRendering() is for <select> too.  Please move these declarations beside delegatesMenuListRendering().

Done

>> Source/WebCore/rendering/RenderThemeChromiumLinux.h:77
>> +        virtual bool popsMenuBySpaceOrReturn() const { return true; }
> 
> Please add 'OVERRIDE' between const and {.

Done

>> Source/WebCore/rendering/RenderThemeMac.h:83
>> +    virtual bool popsMenuByArrowKeys() const { return true; }
> 
> ditto.

Done

>> Source/WebCore/rendering/RenderThemeSafari.h:88
>> +
> 
> You should not modify RenderThemeSafari, which is for Apple Windows port.

Removed back to the original.  Thanks for pointing.
Comment 5 WebKit Review Bot 2012-01-18 01:02:30 PST
Attachment 122885 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/html/HTMLSelectElement.cpp:986:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Kent Tamura 2012-01-18 01:06:08 PST
Comment on attachment 122885 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122885&action=review

> Source/WebCore/rendering/RenderTheme.h:212
> +    // Method for <select> elements.

nit: 'Method' is not a standard term for C++.  This should be 'Member functions', or just 'Functions'.
Comment 7 Jun Mukai 2012-01-18 01:07:16 PST
Created attachment 122887 [details]
Patch
Comment 8 Jun Mukai 2012-01-18 01:08:56 PST
Created attachment 122888 [details]
Patch
Comment 9 Jun Mukai 2012-01-18 01:10:00 PST
Comment on attachment 122885 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122885&action=review

Also fixed a style violation (no need of {} for just return false;).

>> Source/WebCore/rendering/RenderTheme.h:212
>> +    // Method for <select> elements.
> 
> nit: 'Method' is not a standard term for C++.  This should be 'Member functions', or just 'Functions'.

Done
Comment 10 Kent Tamura 2012-01-18 01:20:05 PST
Comment on attachment 122888 [details]
Patch

Looks ok.  Thank you!
Comment 11 WebKit Review Bot 2012-01-18 02:39:47 PST
Comment on attachment 122888 [details]
Patch

Clearing flags on attachment: 122888

Committed r105253: <http://trac.webkit.org/changeset/105253>
Comment 12 WebKit Review Bot 2012-01-18 02:39:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Sergio Villar Senin 2012-01-18 09:25:27 PST
This change provoked a regression in GTK+. It has been addressed in https://bugs.webkit.org/show_bug.cgi?id=76549
Comment 14 Benjamin Poulain 2012-01-20 14:00:53 PST
Could you explain why is that needed? The ChangeLog does not say.

This is adding symbols and dead code to the binary, and complexity to the source.
Comment 15 Kent Tamura 2012-01-22 20:24:34 PST
(In reply to comment #14)
> Could you explain why is that needed? The ChangeLog does not say.
> 
> This is adding symbols and dead code to the binary, and complexity to the source.

The motivation was runtime switching of these behaviors.
Also, using platform-ifdef in platform-neutral code is bad. We have similar approach in WebCore/editing/EditingBehavior*.