Bug 58505 – [Chromium]UI polishes and tweaks to Autofill dropdown menu.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Naoki Takano]

[Chromium]UI polishes and tweaks to Autofill dropdown menu.

[Naoki Takano]

Created attachment 89517 [details]
Patch

[Naoki Takano]

Comment on attachment 89517 [details]
Patch

Could you review my patch?

[Naoki Takano]

The original bug report is here.
http://code.google.com/p/chromium/issues/detail?id=51077

[Ilya Sherman]

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.

[Naoki Takano]

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

[Naoki Takano]

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.

[Naoki Takano]

Please review again.

[Naoki Takano]

Created attachment 89722 [details]
Patch

[WebKit Review Bot]

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.

[Naoki Takano]

Created attachment 89723 [details]
Remove separator padding at the edges.

Remove separator padding.

[Ilya Sherman]

(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 :)

[Ilya Sherman]

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"

[Naoki Takano]

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

[Naoki Takano]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 89723 [details]
Remove separator padding at the edges.

Can you address the nits?

[Naoki Takano]

Sure!!

(In reply to comment #15)
> (From update of attachment 89723 [details])
> Can you address the nits?

[Naoki Takano]

To see the screen shot, please refer to

http://code.google.com/p/chromium/issues/detail?id=51077

[Naoki Takano]

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

[David Holloway]

I have some suggestions about layout and separator color.  Please see my comments in http://code.google.com/p/chromium/issues/detail?id=51077.

[Naoki Takano]

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.

[Naoki Takano]

Created attachment 89915 [details]
Patch

[Naoki Takano]

Per David's request, I tweak my patch again.

Please review again, David.

Thanks,

[Naoki Takano]

Created attachment 89952 [details]
Patch

[Naoki Takano]

Comment on attachment 89952 [details]
Patch

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

Please review.

[Kent Tamura]

(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??)

[Kent Tamura]

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 "*"

[Naoki Takano]

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,

[Kent Tamura]

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.

[Naoki Takano]

Created attachment 90130 [details]
Patch

[Naoki Takano]

Comment on attachment 90130 [details]
Patch

Please review again.

About 'k' prefix problem, I'll be in charge of it next time...

[Kent Tamura]

Comment on attachment 90130 [details]
Patch

Looks ok.

[Naoki Takano]

Roma requests very tiny parameter tuning, maybe 0.8 to 0.9.

Once the parameter is fixed, I let you know.

Thanks,

[Naoki Takano]

Created attachment 90301 [details]
Patch

[Naoki Takano]

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,

[Kent Tamura]

Comment on attachment 90301 [details]
Patch

ok

[WebKit Commit Bot]

Comment on attachment 90301 [details]
Patch

Clearing flags on attachment: 90301

Committed r84342: <http://trac.webkit.org/changeset/84342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.