[Chromium] Upstream the RenderTheme and ScrollbarTheme for Android
Created attachment 119443 [details] Patch
Comment on attachment 119443 [details] Patch Attachment 119443 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10903296
Created attachment 119451 [details] Patch
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?
Ping again..
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 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.
Created attachment 121110 [details] Updated patch
Comment on attachment 121110 [details] Updated patch Ok.
Comment on attachment 121110 [details] Updated patch Clearing flags on attachment: 121110 Committed r104139: <http://trac.webkit.org/changeset/104139>
All reviewed patches have been landed. Closing bug.