Summary: | [Chromium] Upstream the RenderTheme and ScrollbarTheme for Android | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Beverloo <peter> | ||||||||
Component: | New Bugs | Assignee: | 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
Peter Beverloo
2011-12-15 09:08:16 PST
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. |