[webkit-reviews] review denied: [Bug 111873] [Chromium] chromium/linux breaks expectation of select popup background due to bad UA css rules : [Attachment 192518] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied xiyuan <xiyuan at chromium.org>'s
request for review:
Bug 111873: [Chromium] chromium/linux breaks expectation of select popup
background due to bad UA css rules
https://bugs.webkit.org/show_bug.cgi?id=111873

Attachment 192518: Patch
https://bugs.webkit.org/attachment.cgi?id=192518&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=192518&action=review


Looking good.  Just some final style nits.

> Source/WebCore/ChangeLog:19
> +	   No new tests (no change in behaviour).

I would write: No new tests, this tests <select> popups and can be verified by
ManualTests/select-scroll.html.

> Source/WebCore/css/themeChromiumLinux.css:34
> +    /* Selects look like buttons with a drop down triangle on Linux */

Nit: End the sentence with a period.

> Source/WebCore/rendering/RenderMenuList.cpp:469
> +    getItemBackgroundColorAndFlag(listIndex, itemBackgroundColor,
itemHasCustomBackgroundColor);

AndFlag doesn't seem to add much to this function.  I would just call it
getItemBackgroundColor.

> Source/WebCore/rendering/RenderMenuList.cpp:477
> +void RenderMenuList::getItemBackgroundColorAndFlag(unsigned listIndex,
Color& outItemBackgroundColor, bool& outItemHasCustomBackgroundColor) const

I don't think it's WebKit style to prefix parameters with 'out'.  I would drop
that prefix.

> Source/WebCore/rendering/RenderMenuList.h:118
> +    void getItemBackgroundColorAndFlag(unsigned listIndex, Color&
outItemBackgroundColor, bool& outItemHasCustomBackgroundColor) const;

Nit: Color does not need to be named.

> Source/WebKit/chromium/src/AutofillPopupMenuClient.cpp:295
>  

Should this be Custom or Default?  I guess I would use Default since it's the
original value.