[webkit-reviews] review granted: [Bug 74614] [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android : [Attachment 119451] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Peter Beverloo
<peter at chromium.org>'s request for review:
Bug 74614: [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
https://bugs.webkit.org/show_bug.cgi?id=74614

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119451&action=review


> Source/WebCore/ChangeLog:17
> +	   No new tests. (OOPS!)

We can't land a patch with this line.  You should replace it with an
explanation of how this code is tested.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:78
> +    // HorizontalScrollbar

Should this be an ASSERT ?

> Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:48
> +    static RenderTheme* rt = RenderThemeChromiumAndroid::create().leakRef();


rt => renderTheme

> Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:49
> +    return rt;

So, every Page as a common render theme.

> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:77
> +#if !OS(ANDROID)
>  PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page* page)
>  {
>      static RenderTheme* rt = RenderThemeChromiumLinux::create().leakRef();
>      return rt;
>  }
> +#endif

Another option is to break this out into a separate file, but I think this is
ok.  I would have added a comment referring the reader to
RenderThemeChromiumAndroid.