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.
Created attachment 122881 [details] Patch
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.
Created attachment 122885 [details] Patch
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.
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 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'.
Created attachment 122887 [details] Patch
Created attachment 122888 [details] Patch
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 on attachment 122888 [details] Patch Looks ok. Thank you!
Comment on attachment 122888 [details] Patch Clearing flags on attachment: 122888 Committed r105253: <http://trac.webkit.org/changeset/105253>
All reviewed patches have been landed. Closing bug.
This change provoked a regression in GTK+. It has been addressed in https://bugs.webkit.org/show_bug.cgi?id=76549
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.
(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*.