Bug 64897 - Minimum Row Height For PopupListBoxes should platform-dependent
Summary: Minimum Row Height For PopupListBoxes should platform-dependent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-20 15:16 PDT by Fady Samuel
Modified: 2011-07-27 20:56 PDT (History)
5 users (show)

See Also:


Attachments
This changes behavior only on certain Chromium Linux platforms. (5.28 KB, patch)
2011-07-20 15:18 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Fixed a small style issue (5.28 KB, patch)
2011-07-20 15:32 PDT, Fady Samuel
jamesr: review-
Details | Formatted Diff | Diff
Fixed a couple of style issues as suggested by jamesr (3.66 KB, application/octet-stream)
2011-07-21 12:43 PDT, Fady Samuel
no flags Details
Uploaded previous file in incorrect format (3.66 KB, patch)
2011-07-21 12:47 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Got rid of use of 'get' (3.64 KB, patch)
2011-07-21 13:33 PDT, Fady Samuel
jamesr: review-
Details | Formatted Diff | Diff
Another approach as suggested by fishd (13.00 KB, patch)
2011-07-22 15:43 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Fixed style (12.95 KB, patch)
2011-07-25 08:08 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Updated according to fishd (3.94 KB, patch)
2011-07-25 15:58 PDT, Fady Samuel
fishd: review-
Details | Formatted Diff | Diff
Fixed a couple of nits (3.84 KB, patch)
2011-07-26 16:22 PDT, Fady Samuel
fishd: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Forgot to upload changelog along with patch (5.34 KB, patch)
2011-07-27 07:33 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Added the bug report link (5.44 KB, patch)
2011-07-27 07:42 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-07-20 15:16:30 PDT
This defaults to zero for all but certain Chromium Linux platforms.
Comment 1 Fady Samuel 2011-07-20 15:18:04 PDT
Created attachment 101519 [details]
This changes behavior only on certain Chromium Linux platforms.
Comment 2 WebKit Review Bot 2011-07-20 15:21:19 PDT
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.
Comment 3 Fady Samuel 2011-07-20 15:32:11 PDT
Created attachment 101521 [details]
Fixed a small style issue
Comment 4 James Robinson 2011-07-20 15:57:06 PDT
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 5 Fady Samuel 2011-07-21 12:40:50 PDT
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.
Comment 6 Fady Samuel 2011-07-21 12:43:17 PDT
Created attachment 101630 [details]
Fixed a couple of style issues as suggested by jamesr
Comment 7 Fady Samuel 2011-07-21 12:47:07 PDT
Created attachment 101632 [details]
Uploaded previous file in incorrect format
Comment 8 James Robinson 2011-07-21 13:17:03 PDT
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.
Comment 9 Fady Samuel 2011-07-21 13:33:13 PDT
Created attachment 101640 [details]
Got rid of use of 'get'
Comment 10 James Robinson 2011-07-21 13:36:40 PDT
Comment on attachment 101640 [details]
Got rid of use of 'get'

OK
Comment 11 WebKit Review Bot 2011-07-21 14:41:14 PDT
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 12 James Robinson 2011-07-21 14:44:12 PDT
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 13 Darin Fisher (:fishd, Google) 2011-07-22 08:50:15 PDT
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?
Comment 14 Darin Fisher (:fishd, Google) 2011-07-22 08:52:03 PDT
Examples:
  WebView::setScrollbarColors
  WebView::setSelectionColors

Other examples:
  WebSettings
Comment 15 Fady Samuel 2011-07-22 09:01:26 PDT
(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.
Comment 16 Fady Samuel 2011-07-22 11:51:23 PDT
(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
Comment 17 Fady Samuel 2011-07-22 15:43:50 PDT
Created attachment 101782 [details]
Another approach as suggested by fishd
Comment 18 WebKit Review Bot 2011-07-22 15:47:50 PDT
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.
Comment 19 Fady Samuel 2011-07-25 08:08:17 PDT
Created attachment 101870 [details]
Fixed style
Comment 20 Darin Fisher (:fishd, Google) 2011-07-25 12:51:03 PDT
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.
Comment 21 Fady Samuel 2011-07-25 15:58:53 PDT
Created attachment 101933 [details]
Updated according to fishd
Comment 22 Darin Fisher (:fishd, Google) 2011-07-26 15:49:51 PDT
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.
Comment 23 Fady Samuel 2011-07-26 16:22:58 PDT
Created attachment 102072 [details]
Fixed a couple of nits
Comment 24 WebKit Review Bot 2011-07-26 18:50:36 PDT
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
Comment 25 Fady Samuel 2011-07-27 07:33:53 PDT
Created attachment 102144 [details]
Forgot to upload changelog along with patch
Comment 26 WebKit Review Bot 2011-07-27 07:36:11 PDT
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.
Comment 27 Fady Samuel 2011-07-27 07:42:34 PDT
Created attachment 102145 [details]
Added the bug report link
Comment 28 WebKit Review Bot 2011-07-27 20:56:15 PDT
Comment on attachment 102145 [details]
Added the bug report link

Clearing flags on attachment: 102145

Committed r91896: <http://trac.webkit.org/changeset/91896>
Comment 29 WebKit Review Bot 2011-07-27 20:56:24 PDT
All reviewed patches have been landed.  Closing bug.