Bug 96516 – [Chromium] Move UNIX-specific theme functions out of PlatformSupport

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

[Chromium] Move UNIX-specific theme functions out of PlatformSupport

[Mark Pilgrim (Google)]

Created attachment 163627 [details]
WIP patch

[Mark Pilgrim (Google)]

Created attachment 164744 [details]
WIP patch

[Mark Pilgrim (Google)]

Created attachment 165848 [details]
WIP Patch

[Mark Pilgrim (Google)]

Created attachment 165858 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 165858 [details]
Patch

Attachment 165858 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14038402

[Adam Barth]

Comment on attachment 165858 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165858&action=review

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:37
>  

This blank line isn't needed.

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:37
>  

This blank line isn't needed.

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:205
>  bool RenderThemeChromiumLinux::paintCheckbox(RenderObject* o, const PaintInfo& i, const IntRect& rect)

I would have changed these variable names to be complete words.

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:213
> +    WebKit::Platform::current()->themeEngine()->paint(
> +                                                      canvas,

You can merge these two lines (or really just put this all on one line).

[Adam Barth]

Looks like there's also Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp to worry about.

[Mark Pilgrim (Google)]

Created attachment 165883 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 165883 [details]
Patch

Nits addressed, Android build fixed.

[Adam Barth]

Comment on attachment 165883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165883&action=review

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:112
> +    WebKit::Platform::current()->themeEngine()->paint(
> +        canvas,
> +        paintPart,
> +        state,
> +        WebKit::WebRect(rect),
> +        0);

I would just put all this stuff on one line.  This line breaking style comes from the Chromium style guide and is a result of the 80 col limit.

> Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:39
>  

No need for a blank line here.

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:61
> +    {
> +        if (!theme->isEnabled(o))
> +            return WebKit::WebThemeEngine::StateDisabled;
> +        if (theme->isPressed(o))
> +            return WebKit::WebThemeEngine::StatePressed;

Looks like you've got a bad indent here.

[Mark Pilgrim (Google)]

Created attachment 166029 [details]
Patch

[Mark Pilgrim (Google)]

Comment on attachment 166029 [details]
Patch

Nits addressed.

[WebKit Review Bot]

Comment on attachment 166029 [details]
Patch

Clearing flags on attachment: 166029

Committed r129792: <http://trac.webkit.org/changeset/129792>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.