[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 #4 from Ilya Sherman <isherman at chromium.org>  2011-04-14 02:23:01 PST ---
(From update of attachment 89517)
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.

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