This defaults to zero for all but certain Chromium Linux platforms.
Created attachment 101519 [details] This changes behavior only on certain Chromium Linux platforms.
Attachment 101519 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/chromium/PlatformB..." exit_code: 1 Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:160: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 101521 [details] Fixed a small style issue
Comment on attachment 101521 [details] Fixed a small style issue View in context: https://bugs.webkit.org/attachment.cgi?id=101521&action=review > Source/WebCore/platform/chromium/PlatformBridge.h:401 > + static int getPopupListBoxMinimumRowHeight(); we don't prefix getters with "get" in WebKit > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1179 > + int minimumHeight = RenderTheme::defaultTheme()->popupListBoxMinimumRowHeight(); > + why don't you just ask PlatformBridge for the minimum row height here? > Source/WebCore/rendering/RenderTheme.h:170 > + virtual int popupListBoxMinimumRowHeight() const { return 0; } this doesn't seem necessary
Comment on attachment 101521 [details] Fixed a small style issue View in context: https://bugs.webkit.org/attachment.cgi?id=101521&action=review >> Source/WebCore/platform/chromium/PlatformBridge.h:401 >> + static int getPopupListBoxMinimumRowHeight(); > > we don't prefix getters with "get" in WebKit But using 'get' is consistent with getThemePartSize in the same file. Is this also incorrect? >> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1179 >> + > > why don't you just ask PlatformBridge for the minimum row height here? Done. >> Source/WebCore/rendering/RenderTheme.h:170 >> + virtual int popupListBoxMinimumRowHeight() const { return 0; } > > this doesn't seem necessary Removed.
Created attachment 101630 [details] Fixed a couple of style issues as suggested by jamesr
Created attachment 101632 [details] Uploaded previous file in incorrect format
Comment on attachment 101632 [details] Uploaded previous file in incorrect format View in context: https://bugs.webkit.org/attachment.cgi?id=101632&action=review > Source/WebCore/platform/chromium/PlatformBridge.h:401 > + static int getPopupListBoxMinimumRowHeight(); style nit: we don't use "get" for getters in WebKit.
Created attachment 101640 [details] Got rid of use of 'get'
Comment on attachment 101640 [details] Got rid of use of 'get' OK
Comment on attachment 101640 [details] Got rid of use of 'get' Attachment 101640 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9200814
Comment on attachment 101640 [details] Got rid of use of 'get' Thanks, EWS bot! You need a definition+declaration of WebThemeEngine::popupListBoxMinimumRowHeight() to exist for each platform.
Comment on attachment 101640 [details] Got rid of use of 'get' View in context: https://bugs.webkit.org/attachment.cgi?id=101640&action=review > Source/WebKit/chromium/public/linux/WebThemeEngine.h:150 > + virtual int popupListBoxMinimumRowHeight() const { return 0; } is this really a property of a theme engine? often rendering style is pushed down to webkit. maybe you should just have a static setter on WebPopupMenu for this? put another way, does it really need to be dynamically determined? does it depend on a native theme engine like uxtheme.dll or the gtk theme engine?
Examples: WebView::setScrollbarColors WebView::setSelectionColors Other examples: WebSettings
(In reply to comment #13) > (From update of attachment 101640 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101640&action=review > > > Source/WebKit/chromium/public/linux/WebThemeEngine.h:150 > > + virtual int popupListBoxMinimumRowHeight() const { return 0; } > > is this really a property of a theme engine? often rendering style is pushed down to webkit. > maybe you should just have a static setter on WebPopupMenu for this? put another way, does > it really need to be dynamically determined? does it depend on a native theme engine like > uxtheme.dll or the gtk theme engine? The size of hit targets for popup items can vary on certain Chromium platforms.
(In reply to comment #15) > (In reply to comment #13) > > (From update of attachment 101640 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=101640&action=review > > > > > Source/WebKit/chromium/public/linux/WebThemeEngine.h:150 > > > + virtual int popupListBoxMinimumRowHeight() const { return 0; } > > > > is this really a property of a theme engine? often rendering style is pushed down to webkit. > > maybe you should just have a static setter on WebPopupMenu for this? put another way, does > > it really need to be dynamically determined? does it depend on a native theme engine like > > uxtheme.dll or the gtk theme engine? > > The size of hit targets for popup items can vary on certain Chromium platforms. I will give this another approach. Since this is chromium-specific functionality
Created attachment 101782 [details] Another approach as suggested by fishd
Attachment 101782 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/chromium/PopupMenu..." exit_code: 1 Source/WebKit/chromium/public/WebWidget.h:129: Missing space inside { }. [whitespace/braces] [5] Source/WebKit/chromium/src/WebViewImpl.h:107: The parameter name "minimumRowHeight" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/chromium/PopupMenuChromium.h:178: The parameter name "popupType" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/src/WebPopupMenuImpl.h:76: The parameter name "minimumRowHeight" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 101870 [details] Fixed style
Comment on attachment 101870 [details] Fixed style View in context: https://bugs.webkit.org/attachment.cgi?id=101870&action=review > Source/WebKit/chromium/public/WebWidget.h:129 > + virtual void setPopupListBoxMinimumRowHeight(int minimumRowHeight) { } do you really need to make this widget-specific? it seems like it could just be a static function on WebPopupMenu that is implemented in terms of modifying a global variable stored in PopupMenuChromium.
Created attachment 101933 [details] Updated according to fishd
Comment on attachment 101933 [details] Updated according to fishd View in context: https://bugs.webkit.org/attachment.cgi?id=101933&action=review yeah, this is basically what i had in mind. one more round. > Source/WebKit/chromium/public/WebPopupMenu.h:44 > + // Sets the minimum height of a popup listbox row. nit: new line above this comment. > Source/WebKit/chromium/public/WebPopupMenu.h:45 > + WEBKIT_API static void setPopupListBoxMinimumRowHeight(int); nit: no need to repeat the word Popup. i'd probably just name this method setMinimumRowHeight since it should be obvious that this could only refer to the rows of the popup menu.
Created attachment 102072 [details] Fixed a couple of nits
Comment on attachment 102072 [details] Fixed a couple of nits Rejecting attachment 102072 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: ommit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9249630
Created attachment 102144 [details] Forgot to upload changelog along with patch
Attachment 102144 [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/WebKit/chromium/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 102145 [details] Added the bug report link
Comment on attachment 102145 [details] Added the bug report link Clearing flags on attachment: 102145 Committed r91896: <http://trac.webkit.org/changeset/91896>
All reviewed patches have been landed. Closing bug.