[webkit-reviews] review denied: [Bug 52826] [Chromium] Use WebThemeEngine for relevant RenderTheme parts for chromium/linux : [Attachment 79628] Proposed code change and new baseline for layout tests.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied xiyuan <xiyuan at chromium.org>'s
request for review:
Bug 52826: [Chromium] Use WebThemeEngine for relevant RenderTheme parts for
chromium/linux
https://bugs.webkit.org/show_bug.cgi?id=52826

Attachment 79628: Proposed code change and new baseline for layout tests.
https://bugs.webkit.org/attachment.cgi?id=79628&action=review

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

r- for style nits.

Can you upload some of image diffs?  I'm having a hard time seeing the
difference.  Maybe it's just a few rgb points off?

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:58
> +	 return ChromiumBridge::StateHover;

4 space indent here and above 2 conditions

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:196
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartCheckbox,
> +	   getWebThemeState(this, o),
> +	   rect,
> +	   &extraParams);

I think this should just be one long line (maybe one line of wrapping if it's
really long).

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:217
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartRadio,

less wrapping

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:238
> +    extraParams.button.backgroundColor = SkColorSetRGB(0xdd, 0xdd, 0xdd);

Should this color be a constant somewhere?

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:244
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartButton,

less wrapping

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:294
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartMenuList,

less wrapping

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:308
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartSliderTrack,

less wrapping

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:323
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartSliderThumb,

less wrapping

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:346
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartInnerSpinButton,

less wrapping

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:372
> +    ChromiumBridge::paintThemePart(
> +	   i.context,
> +	   ChromiumBridge::PartProgressBar,

less wrapping