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


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-12-15 09:08:16 PST
[Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
------- Comment #1 From 2011-12-15 09:47:24 PST -------
Created an attachment (id=119443) [details]
Patch
------- Comment #2 From 2011-12-15 09:55:20 PST -------
(From update of attachment 119443 [details])
Attachment 119443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10903296
------- Comment #3 From 2011-12-15 10:11:17 PST -------
Created an attachment (id=119451) [details]
Patch
------- Comment #4 From 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 From 2012-01-03 08:17:34 PST -------
Ping again..
------- Comment #6 From 2012-01-03 11:51:11 PST -------
(From update of attachment 119451 [details])
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 From 2012-01-04 07:45:11 PST -------
(From update of attachment 119451 [details])
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 From 2012-01-04 07:46:20 PST -------
Created an attachment (id=121110) [details]
Updated patch
------- Comment #9 From 2012-01-04 07:51:31 PST -------
(From update of attachment 121110 [details])
Ok.
------- Comment #10 From 2012-01-05 04:22:23 PST -------
(From update of attachment 121110 [details])
Clearing flags on attachment: 121110

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