Bug 74614 – [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android

• Component: New Bugs
• Status: RESOLVED FIXED

[Peter Beverloo]

[Chromium] Upstream the RenderTheme and ScrollbarTheme for Android

[Peter Beverloo]

Created attachment 119443 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119443 [details]
Patch

Attachment 119443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10903296

[Peter Beverloo]

Created attachment 119451 [details]
Patch

[Peter Beverloo]

Ping. I cc'ed some people who previously did reviews on RenderThemeChromiumLinux.cpp or showed up in git blame themselves, is there another person I should ask?

[Peter Beverloo]

Ping again..

[Adam Barth]

Comment on attachment 119451 [details]
Patch

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.

[Peter Beverloo]

Comment on attachment 119451 [details]
Patch

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

Thank you for the review. Revised patch upcoming.

>> 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.

Done.

>> Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:78
>> +    // HorizontalScrollbar
> 
> Should this be an ASSERT ?

No, this was here to emphasize that the following IntSize() return applies to horizontal scrollbars. Given that the ScrollbarOrientation enum only has two values I do not think an assert would be beneficial here.

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

Done.

>> Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:49
>> +    return rt;
> 
> So, every Page as a common render theme.

Correct.

>> Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:77
>> +#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.

Ok. I added a comment pointing towards RTCA.cpp.

[Peter Beverloo]

Created attachment 121110 [details]
Updated patch

[Adam Barth]

Comment on attachment 121110 [details]
Updated patch

Ok.

[WebKit Review Bot]

Comment on attachment 121110 [details]
Updated patch

Clearing flags on attachment: 121110

Committed r104139: <http://trac.webkit.org/changeset/104139>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.