Bug 74614 - [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
: [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Peter Beverloo
:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2011-12-15 09:08 PST by Peter Beverloo
Modified: 2012-01-05 04:22 PST (History)
6 users (show)

See Also:


Attachments
Patch (19.45 KB, patch)
2011-12-15 09:47 PST, Peter Beverloo
no flags Details | Formatted Diff | Diff
Patch (19.54 KB, patch)
2011-12-15 10:11 PST, Peter Beverloo
abarth: review+
Details | Formatted Diff | Diff
Updated patch (18.96 KB, patch)
2012-01-04 07:46 PST, Peter Beverloo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Beverloo 2011-12-15 09:08:16 PST
[Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
Comment 1 Peter Beverloo 2011-12-15 09:47:24 PST
Created attachment 119443 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-15 09:55:20 PST
Comment on attachment 119443 [details]
Patch

Attachment 119443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10903296
Comment 3 Peter Beverloo 2011-12-15 10:11:17 PST
Created attachment 119451 [details]
Patch
Comment 4 Peter Beverloo 2011-12-20 08:54:08 PST
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?
Comment 5 Peter Beverloo 2012-01-03 08:17:34 PST
Ping again..
Comment 6 Adam Barth 2012-01-03 11:51:11 PST
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.
Comment 7 Peter Beverloo 2012-01-04 07:45:11 PST
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.
Comment 8 Peter Beverloo 2012-01-04 07:46:20 PST
Created attachment 121110 [details]
Updated patch
Comment 9 Adam Barth 2012-01-04 07:51:31 PST
Comment on attachment 121110 [details]
Updated patch

Ok.
Comment 10 WebKit Review Bot 2012-01-05 04:22:23 PST
Comment on attachment 121110 [details]
Updated patch

Clearing flags on attachment: 121110

Committed r104139: <http://trac.webkit.org/changeset/104139>
Comment 11 WebKit Review Bot 2012-01-05 04:22:28 PST
All reviewed patches have been landed.  Closing bug.