[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 #12 from Ilya Sherman <isherman at chromium.org>  2011-04-14 23:52:36 PST ---
(From update of attachment 89723)
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"

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