Bug 58505 - [Chromium]UI polishes and tweaks to Autofill dropdown menu.
Summary: [Chromium]UI polishes and tweaks to Autofill dropdown menu.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-13 19:03 PDT by Naoki Takano
Modified: 2011-04-19 22:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.48 KB, patch)
2011-04-13 19:13 PDT, Naoki Takano
honten: review-
Details | Formatted Diff | Diff
Revised patch. (15.34 KB, patch)
2011-04-14 19:00 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (17.11 KB, patch)
2011-04-14 19:21 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Remove separator padding at the edges. (15.83 KB, patch)
2011-04-14 19:25 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (12.14 KB, patch)
2011-04-15 22:10 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (13.08 KB, patch)
2011-04-17 11:32 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2011-04-18 19:09 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (9.92 KB, patch)
2011-04-19 21:18 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Naoki Takano 2011-04-13 19:03:29 PDT
[Chromium]UI polishes and tweaks to Autofill dropdown menu.
Comment 1 Naoki Takano 2011-04-13 19:13:46 PDT
Created attachment 89517 [details]
Patch
Comment 2 Naoki Takano 2011-04-13 19:14:37 PDT
Comment on attachment 89517 [details]
Patch

Could you review my patch?
Comment 3 Naoki Takano 2011-04-13 19:16:08 PDT
The original bug report is here.
http://code.google.com/p/chromium/issues/detail?id=51077
Comment 4 Ilya Sherman 2011-04-14 02:23:01 PDT
Comment on attachment 89517 [details]
Patch

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

I know that a lot of the code in this file is shared by the <select> popup (except on Mac) and by the Autofill popup.  I believe we only want these changes to affect the Autofill popup.  Have you verified that this is indeed the case?

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:79
> +static const int kLineHeightMargin = 3; // Line height margin put at the top and bottom of each line.

nit: This should probably be called "kLineHeightPadding" -- margins would be allowed to overlap, whereas this is additive, like padding.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:208
> +    // Paint an top and bottom padding.

nit: "an" -> "the"

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:858
> +void PopupListBox::paintTopBottomPadding(GraphicsContext* gc, const IntRect& rect)

nit: Perhaps "paintVerticalPadding"?

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:864
> +    Color backColor = m_popupClient->itemStyle(0).backgroundColor();

nit: Prefer to use unabbreviated names -- in this case, "backgroundColor"

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:868
> +    topRect.move(0, - kPaddingHeight);

nit: Omit the space in "- kPaddingHeight" since this is a unary minus

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:957
> +        gc->fillRect(separatorRect, Color(0xcd, 0xcd, 0xcd), ColorSpaceDeviceRGB);

nit: I wonder if there's an existing named color for this.  It seems a little weird to declare a color constant as a function parameter.  I'm not completely up to speed on WebKit style guidelines, though.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1146
> +        return separatorHeight;

nit: As mentioned in the Chromium bug, we might want a few pixels of padding around the separator.  I'd need to see more screenshots or play with the code to have a beter idea of what exactly we want here, though.
Comment 5 Naoki Takano 2011-04-14 10:53:52 PDT
(In reply to comment #4)
> (From update of attachment 89517 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89517&action=review
> 
> I know that a lot of the code in this file is shared by the <select> popup (except on Mac) and by the Autofill popup.  I believe we only want these changes to affect the Autofill popup.  Have you verified that this is indeed the case?
I verified them, but I assumed the other popups also need to obey this rule. I'll leave the other popups as it is.

My plan is
1, Add whole padding and margin information into PopupMenuStyle.
2, Add popup window type into PopupMenuStyle and according to the type, change the margin.

I prefer to 1.

> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:957
> > +        gc->fillRect(separatorRect, Color(0xcd, 0xcd, 0xcd), ColorSpaceDeviceRGB);
> 
> nit: I wonder if there's an existing named color for this.  It seems a little weird to declare a color constant as a function parameter.  I'm not completely up to speed on WebKit style guidelines, though.
There is not #cdcdcd color in Webkit as defined. But as I suggested in bug track comment, I will use #c0c0c0 which is Color::lightGray. That would be better.

> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1146
> > +        return separatorHeight;
> 
> nit: As mentioned in the Chromium bug, we might want a few pixels of padding around the separator.  I'd need to see more screenshots or play with the code to have a beter idea of what exactly we want here, though.

As Roma isn't worried about it, so I will leave here.

Thanks,
Comment 6 Naoki Takano 2011-04-14 19:00:42 PDT
Created attachment 89718 [details]
Revised patch.

Please review again.

Once I got ToT, my webkit-patch upload doesn't work anymore because of exception. Now I'm investigating...

So I upload it manually.
Comment 7 Naoki Takano 2011-04-14 19:00:57 PDT
Please review again.
Comment 8 Naoki Takano 2011-04-14 19:21:08 PDT
Created attachment 89722 [details]
Patch
Comment 9 WebKit Review Bot 2011-04-14 19:23:06 PDT
Attachment 89722 [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

Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:326:  at least two spaces before inline comment  [pep8/E261] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Naoki Takano 2011-04-14 19:25:14 PDT
Created attachment 89723 [details]
Remove separator padding at the edges.

Remove separator padding.
Comment 11 Ilya Sherman 2011-04-14 23:41:09 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 89517 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=89517&action=review
> > 
> > I know that a lot of the code in this file is shared by the <select> popup (except on Mac) and by the Autofill popup.  I believe we only want these changes to affect the Autofill popup.  Have you verified that this is indeed the case?
> I verified them, but I assumed the other popups also need to obey this rule. I'll leave the other popups as it is.
> 
> My plan is
> 1, Add whole padding and margin information into PopupMenuStyle.
> 2, Add popup window type into PopupMenuStyle and according to the type, change the margin.
> 
> I prefer to 1.

Long-term, we want to more cleanly separate the Autofill popup code from the <select> popup code -- the current dual-purpose code can be a bit clumsy to update.  I agree that (1) is probably a better short term solution that (2) though -- perhaps some WebKit folks more familiar with this code could comment?

> As Roma isn't worried about it, so I will leave here.

Ok :)
Comment 12 Ilya Sherman 2011-04-14 23:52:36 PDT
Comment on attachment 89723 [details]
Remove separator padding at the edges.

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

Nearly all of these are fairly opinionated nits -- feel free to ignore ones that seem totally bogus to you.  One the whole, this looks pretty good to me, but I am not a WebKit reviewer.

> Source/WebCore/ChangeLog:12
> +        Put PopupMenuStyle::paddingLineHeightk() at the top and bottom of each line.

nit: Stray 'k's in the function names

> Source/WebCore/platform/PopupMenuStyle.h:38
> +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, int paddingHeight, int paddingLineHeight)

nit: "listBoxPaddingHeight" and "linePaddingHeight" might be slightly clearer names for these variables.

> Source/WebCore/platform/PopupMenuStyle.h:72
> +    int m_paddingHeight; // Padding height put at the top and bottom of window.

nit: "window" -> "popup"

> Source/WebCore/platform/PopupMenuStyle.h:73
> +    int m_paddingLineHeight; // Padding line height put at the top and bottom of each line.

nit: "Padding line height" -> "Padding height" (you already mention that this is for each line later in the comment)

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:954
> +        gc->fillRect(separatorRect, Color::lightGray, ColorSpaceDeviceRGB);

Is it possible for a <select> popup to have a separator?  If so, again, I think we only want this change for the Autofill popup.  (I'm not 100% sure about that though.)

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1352
> +    int windowHeight = menuStyle.paddingHeight(); // Top padding heights is added first.

nit: remove "heights"

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1366
> +    windowHeight += menuStyle.paddingHeight(); // Bottom padding heights is added last.

nit: remove "heights"
Comment 13 Naoki Takano 2011-04-15 00:24:58 PDT
Ilya,

Thank you for your great comments every time.

(In reply to comment #12)
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:954
> > +        gc->fillRect(separatorRect, Color::lightGray, ColorSpaceDeviceRGB);
> 
> Is it possible for a <select> popup to have a separator?  If so, again, I think we only want this change for the Autofill popup.  (I'm not 100% sure about that though.)
Hmm... I'm also not sure we will have separator for other popup windows. At least, we don't have any separator tag for select html tag though. We also can drag the padding value with PopupMenuStyle. But the class is getting bigger...
Comment 14 Naoki Takano 2011-04-15 00:27:10 PDT
So is there anybody who can review my patch?

Especially it is very helpful to let me know if the separator padding width should be configurable or not with PopupMenuStyle.
Comment 15 Dimitri Glazkov (Google) 2011-04-15 11:27:20 PDT
Comment on attachment 89723 [details]
Remove separator padding at the edges.

Can you address the nits?
Comment 16 Naoki Takano 2011-04-15 11:28:06 PDT
Sure!!

(In reply to comment #15)
> (From update of attachment 89723 [details])
> Can you address the nits?
Comment 17 Naoki Takano 2011-04-15 11:31:07 PDT
To see the screen shot, please refer to

http://code.google.com/p/chromium/issues/detail?id=51077
Comment 18 Naoki Takano 2011-04-15 11:46:21 PDT
After looking around the source code, I notice itemIsSeparator() might return true even if not AutoFillPopupMenuClient. So it implies other popup window separator might be drawn.

Ok, I add one more parameter to PopupMenuStyle and drag it.

(In reply to comment #13)
> Ilya,
> 
> Thank you for your great comments every time.
> 
> (In reply to comment #12)
> > > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:954
> > > +        gc->fillRect(separatorRect, Color::lightGray, ColorSpaceDeviceRGB);
> > 
> > Is it possible for a <select> popup to have a separator?  If so, again, I think we only want this change for the Autofill popup.  (I'm not 100% sure about that though.)
> Hmm... I'm also not sure we will have separator for other popup windows. At least, we don't have any separator tag for select html tag though. We also can drag the padding value with PopupMenuStyle. But the class is getting bigger...
Comment 19 David Holloway 2011-04-15 18:46:47 PDT
I have some suggestions about layout and separator color.  Please see my comments in http://code.google.com/p/chromium/issues/detail?id=51077.
Comment 20 Naoki Takano 2011-04-15 19:52:19 PDT
Thanks, David.

(In reply to comment #19)
> I have some suggestions about layout and separator color.  Please see my comments in http://code.google.com/p/chromium/issues/detail?id=51077.
Comment 21 Naoki Takano 2011-04-15 22:10:37 PDT
Created attachment 89915 [details]
Patch
Comment 22 Naoki Takano 2011-04-15 22:11:21 PDT
Per David's request, I tweak my patch again.

Please review again, David.

Thanks,
Comment 23 Naoki Takano 2011-04-17 11:32:49 PDT
Created attachment 89952 [details]
Patch
Comment 24 Naoki Takano 2011-04-17 11:33:49 PDT
Comment on attachment 89952 [details]
Patch

Per Roma's request, I added font size change for label if autofill.

Please review.
Comment 25 Kent Tamura 2011-04-17 23:02:54 PDT
(In reply to comment #13)
> > Is it possible for a <select> popup to have a separator?  If so, again, I think we only want this change for the Autofill popup.  (I'm not 100% sure about that though.)
> Hmm... I'm also not sure we will have separator for other popup windows. At least, we don't have any separator tag for select html tag though. We also can drag the padding value with PopupMenuStyle. But the class is getting bigger...

<select> can have an <hr> child, and the <hr> is rendered as a separator though the HTML parser doesn't support this structure.  (HTML5 parser regression??)
Comment 26 Kent Tamura 2011-04-17 23:10:02 PDT
Comment on attachment 89952 [details]
Patch

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

> Source/WebCore/platform/PopupMenuStyle.h:38
> +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, bool isAutofillStyle)

Please avoid new bool parameter.  We prefer introducing enum. Also, we can avoid updating RenderMenuList and RenderTextConstrolSingleLine by a default parameter.

enum PopupMenuType { SelectPopup, AutofillPopup };
PopupMenuStyle(const Color&...., PopupMenuType menuType = SelectPopup)

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:78
> +static const int kLinePaddingHeight = 3; // Padding height put at the top and bottom of each line.

Do not use "k" prefix for constant values.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1000
> +        d.setComputedSize(d.computedSize()*0.8);

Add spaces around "*"
Comment 27 Naoki Takano 2011-04-17 23:23:58 PDT
Kent-san,

Thank you for your review.

(In reply to comment #26)
> (From update of attachment 89952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89952&action=review
> 
> > Source/WebCore/platform/PopupMenuStyle.h:38
> > +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, bool isAutofillStyle)
> 
> Please avoid new bool parameter.  We prefer introducing enum. Also, we can avoid updating RenderMenuList and RenderTextConstrolSingleLine by a default parameter.
I assumed any default parameter is prohibited.
I believe it it true in Chromium project, but is WetKit different?

> enum PopupMenuType { SelectPopup, AutofillPopup };
> PopupMenuStyle(const Color&...., PopupMenuType menuType = SelectPopup)
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:78
> > +static const int kLinePaddingHeight = 3; // Padding height put at the top and bottom of each line.
> 
> Do not use "k" prefix for constant values.
I referred to
 static const int kLabelToIconPadding = 5;
 static const int kMinEndOfLinePadding = 2;

They start with "k", but do I have change "kLinePaddingHeight" to "linePaddingHeight"?

Thanks,
Comment 28 Kent Tamura 2011-04-18 00:25:17 PDT
Comment on attachment 89952 [details]
Patch

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

>>> Source/WebCore/platform/PopupMenuStyle.h:38
>>> +    PopupMenuStyle(const Color& foreground, const Color& background, const Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection textDirection, bool hasTextDirectionOverride, bool isAutofillStyle)
>> 
>> Please avoid new bool parameter.  We prefer introducing enum. Also, we can avoid updating RenderMenuList and RenderTextConstrolSingleLine by a default parameter.
>> 
>> enum PopupMenuType { SelectPopup, AutofillPopup };
>> PopupMenuStyle(const Color&...., PopupMenuType menuType = SelectPopup)
> 
> I assumed any default parameter is prohibited.
> I believe it it true in Chromium project, but is WetKit different?

Yes. WebKit is not Chromium. You can see a lot of default parameter usages in WebKit.

>>> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:78
>>> +static const int kLinePaddingHeight = 3; // Padding height put at the top and bottom of each line.
>> 
>> Do not use "k" prefix for constant values.
> 
> I referred to
>  static const int kLabelToIconPadding = 5;
>  static const int kMinEndOfLinePadding = 2;
> 
> They start with "k", but do I have change "kLinePaddingHeight" to "linePaddingHeight"?
> 
> Thanks,

It's ok to keep "k" if you think consistency in this file is more important than consistency in WebKit.
Anyway, I expect someone makes a patch to remove all of "k" prefixes in PopupMenuChromium.cpp and ScrollbarThemeChromiumWin.cpp later.
Comment 29 Naoki Takano 2011-04-18 19:09:41 PDT
Created attachment 90130 [details]
Patch
Comment 30 Naoki Takano 2011-04-18 19:11:43 PDT
Comment on attachment 90130 [details]
Patch

Please review again.

About 'k' prefix problem, I'll be in charge of it next time...
Comment 31 Kent Tamura 2011-04-19 11:16:44 PDT
Comment on attachment 90130 [details]
Patch

Looks ok.
Comment 32 Naoki Takano 2011-04-19 11:19:28 PDT
Roma requests very tiny parameter tuning, maybe 0.8 to 0.9.

Once the parameter is fixed, I let you know.

Thanks,
Comment 33 Naoki Takano 2011-04-19 21:18:05 PDT
Created attachment 90301 [details]
Patch
Comment 34 Naoki Takano 2011-04-19 21:19:37 PDT
Comment on attachment 90301 [details]
Patch

Kent-san,

Sorry for bothering you again.

But I just changed 0.8 to 0.9 ratio for label font.

Please commit if it's ok.

Thanks,
Comment 35 Kent Tamura 2011-04-19 21:43:54 PDT
Comment on attachment 90301 [details]
Patch

ok
Comment 36 WebKit Commit Bot 2011-04-19 22:16:50 PDT
Comment on attachment 90301 [details]
Patch

Clearing flags on attachment: 90301

Committed r84342: <http://trac.webkit.org/changeset/84342>
Comment 37 WebKit Commit Bot 2011-04-19 22:16:57 PDT
All reviewed patches have been landed.  Closing bug.