[Webkit-unassigned] [Bug 58505] [Chromium]UI polishes and tweaks to Autofill dropdown menu.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58505





--- Comment #27 from Naoki Takano <takano.naoki at gmail.com>  2011-04-17 23:23:58 PST ---
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,

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.