Bug 64897 – Minimum Row Height For PopupListBoxes should platform-dependent

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Fady Samuel]

This defaults to zero for all but certain Chromium Linux platforms.

[Fady Samuel]

Created attachment 101519 [details]
This changes behavior only on certain Chromium Linux platforms.

[WebKit Review Bot]

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.

[Fady Samuel]

Created attachment 101521 [details]
Fixed a small style issue

[James Robinson]

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

[Fady Samuel]

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.

[Fady Samuel]

Created attachment 101630 [details]
Fixed a couple of style issues as suggested by jamesr

[Fady Samuel]

Created attachment 101632 [details]
Uploaded previous file in incorrect format

[James Robinson]

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.

[Fady Samuel]

Created attachment 101640 [details]
Got rid of use of 'get'

[James Robinson]

Comment on attachment 101640 [details]
Got rid of use of 'get'

OK

[WebKit Review Bot]

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

[James Robinson]

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.

[Darin Fisher (:fishd, Google)]

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?

[Darin Fisher (:fishd, Google)]

Examples:
  WebView::setScrollbarColors
  WebView::setSelectionColors

Other examples:
  WebSettings

[Fady Samuel]

(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.

[Fady Samuel]

(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

[Fady Samuel]

Created attachment 101782 [details]
Another approach as suggested by fishd

[WebKit Review Bot]

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.

[Fady Samuel]

Created attachment 101870 [details]
Fixed style

[Darin Fisher (:fishd, Google)]

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.

[Fady Samuel]

Created attachment 101933 [details]
Updated according to fishd

[Darin Fisher (:fishd, Google)]

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.

[Fady Samuel]

Created attachment 102072 [details]
Fixed a couple of nits

[WebKit Review Bot]

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

[Fady Samuel]

Created attachment 102144 [details]
Forgot to upload changelog along with patch

[WebKit Review Bot]

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.

[Fady Samuel]

Created attachment 102145 [details]
Added the bug report link

[WebKit Review Bot]

Comment on attachment 102145 [details]
Added the bug report link

Clearing flags on attachment: 102145

Committed r91896: <http://trac.webkit.org/changeset/91896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.