Bug 74614

Summary: [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
Product: WebKit Reporter: Peter Beverloo <peter>
Component: New BugsAssignee: Peter Beverloo <peter>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, jrg, levin, tonyg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Attachments:
Description Flags
Patch
none
Patch
abarth: review+
Updated patch none

Peter Beverloo
Reported 2011-12-15 09:08:16 PST
[Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
Attachments
Patch (19.45 KB, patch)
2011-12-15 09:47 PST, Peter Beverloo 		no flags
DetailsFormatted DiffDiff
Patch (19.54 KB, patch)
2011-12-15 10:11 PST, Peter Beverloo 		abarth: review+
DetailsFormatted DiffDiff
Updated patch (18.96 KB, patch)
2012-01-04 07:46 PST, Peter Beverloo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Peter Beverloo
Comment 1 2011-12-15 09:47:24 PST
Created attachment 119443 [details] Patch
WebKit Review Bot
Comment 2 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
Peter Beverloo
Comment 3 2011-12-15 10:11:17 PST
Created attachment 119451 [details] Patch
Peter Beverloo
Comment 4 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?
Peter Beverloo
Comment 5 2012-01-03 08:17:34 PST
Ping again..
Adam Barth
Comment 6 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.
Peter Beverloo
Comment 7 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.
Peter Beverloo
Comment 8 2012-01-04 07:46:20 PST
Created attachment 121110 [details] Updated patch
Adam Barth
Comment 9 2012-01-04 07:51:31 PST
Comment on attachment 121110 [details] Updated patch Ok.
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2012-01-05 04:22:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.