[webkit-reviews] review denied: [Bug 111873] [Chromium] chromium/linux breaks expectation of select popup background due to bad UA css rules : [Attachment 192351] 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 192351: Patch
https://bugs.webkit.org/attachment.cgi?id=192351&action=review

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


> Source/WebCore/platform/PopupMenuStyle.h:40
> +    PopupMenuStyle(const Color& foreground, const Color& background, const
Font& font, bool visible, bool isDisplayNone, Length textIndent, TextDirection
textDirection, bool hasTextDirectionOverride, BackgroundColorType
backgroundColorType, PopupMenuType menuType = SelectPopup)

I would probably make BackgroundColorType an optional argument and make the
default DefaultBackgroundColor since most of the callers don't care about this
parameter.

> Source/WebCore/rendering/RenderMenuList.cpp:162
> +    bool backgroundColorChanged = !oldStyle ||
oldStyle->visitedDependentColor(CSSPropertyBackgroundColor) !=
style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (backgroundColorChanged)
> +	   updateHasCustomBackgroundColor();

I think this is unnecessary and doesn't work.  This code only runs if the style
is changed dynamically.

I would remove m_hasCustomBackgroundColor and instead pass in the <select>
background color to itemStyle as a param.  Then you can use
CustomBackgroundColor if the <select> background color doesn't match the passed
in value and the option is transparent.