[webkit-reviews] review granted: [Bug 42887] [Qt] Search input field doesn't have cancel button : [Attachment 78438] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Robert Hogan
<robert at webkit.org>'s request for review:
Bug 42887: [Qt] Search input field doesn't have cancel button
https://bugs.webkit.org/show_bug.cgi?id=42887

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78438&action=review

It contains new API, but it looks fine with me from a webkit 1 perspective.

> Source/WebCore/platform/qt/RenderThemeQt.cpp:96
> +// These values all match Safari/Win/Chromium

Add dot before landing. Would be good with a "why" they must match :-) because
then I do not see why we all need to define them.

> Source/WebCore/platform/qt/RenderThemeQt.cpp:216
> +// Remove this when SearchFieldPart is style-able in
RenderTheme::isControlStyled

Add "()."

> Source/WebCore/platform/qt/RenderThemeQt.cpp:224
> +	   return (style->border() != border
> +		   || *style->backgroundLayers() != fill
> +		   || style->visitedDependentColor(CSSPropertyBackgroundColor)
!= backgroundColor);

So this part is covered with the layout tests? seems like something that should
be common code

> Source/WebCore/platform/qt/RenderThemeQt.cpp:930
> +    // Taken from RenderThemeChromium.cpp.

Here the comment is inside the method

> Source/WebCore/platform/qt/RenderThemeQt.cpp:939
> +// Taken from RenderThemeChromium.cpp

Here it is outside? on purpose?

> Source/WebCore/platform/qt/RenderThemeQt.cpp:955
> +    // Adapted from RenderThemeChromium.cpp.

the adaption means using qMax? or did you change logic? you should make that
clear