Bug 96516

Summary: [Chromium] Move UNIX-specific theme functions out of PlatformSupport
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: WebKit Misc.Assignee: Mark Pilgrim (Google) <pilgrim>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, fishd, haraken, peter+ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82948    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP Patch
none
Patch
none
Patch
none
Patch none

Mark Pilgrim (Google)
Reported 2012-09-12 07:54:03 PDT
[Chromium] Move UNIX-specific theme functions out of PlatformSupport
Attachments
WIP patch (39.78 KB, patch)
2012-09-12 07:54 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP patch (22.61 KB, patch)
2012-09-19 08:51 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
WIP Patch (13.52 KB, patch)
2012-09-26 12:13 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (34.47 KB, patch)
2012-09-26 12:56 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (33.89 KB, patch)
2012-09-26 14:48 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Patch (33.85 KB, patch)
2012-09-27 10:31 PDT, Mark Pilgrim (Google) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Mark Pilgrim (Google)
Comment 1 2012-09-12 07:54:25 PDT
Created attachment 163627 [details] WIP patch
Mark Pilgrim (Google)
Comment 2 2012-09-19 08:51:02 PDT
Created attachment 164744 [details] WIP patch
Mark Pilgrim (Google)
Comment 3 2012-09-26 12:13:28 PDT
Created attachment 165848 [details] WIP Patch
Mark Pilgrim (Google)
Comment 4 2012-09-26 12:56:13 PDT
Created attachment 165858 [details] Patch
Peter Beverloo (cr-android ews)
Comment 5 2012-09-26 13:10:56 PDT
Comment on attachment 165858 [details] Patch Attachment 165858 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14038402
Adam Barth
Comment 6 2012-09-26 13:18:56 PDT
Comment on attachment 165858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165858&action=review > Source/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:37 > This blank line isn't needed. > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:37 > This blank line isn't needed. > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:205 > bool RenderThemeChromiumLinux::paintCheckbox(RenderObject* o, const PaintInfo& i, const IntRect& rect) I would have changed these variable names to be complete words. > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:213 > + WebKit::Platform::current()->themeEngine()->paint( > + canvas, You can merge these two lines (or really just put this all on one line).
Adam Barth
Comment 7 2012-09-26 13:19:28 PDT
Looks like there's also Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp to worry about.
Mark Pilgrim (Google)
Comment 8 2012-09-26 14:48:43 PDT
Created attachment 165883 [details] Patch
Mark Pilgrim (Google)
Comment 9 2012-09-26 14:49:04 PDT
Comment on attachment 165883 [details] Patch Nits addressed, Android build fixed.
Adam Barth
Comment 10 2012-09-27 09:36:15 PDT
Comment on attachment 165883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165883&action=review > Source/WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:112 > + WebKit::Platform::current()->themeEngine()->paint( > + canvas, > + paintPart, > + state, > + WebKit::WebRect(rect), > + 0); I would just put all this stuff on one line. This line breaking style comes from the Chromium style guide and is a result of the 80 col limit. > Source/WebCore/rendering/RenderThemeChromiumAndroid.cpp:39 > No need for a blank line here. > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:61 > + { > + if (!theme->isEnabled(o)) > + return WebKit::WebThemeEngine::StateDisabled; > + if (theme->isPressed(o)) > + return WebKit::WebThemeEngine::StatePressed; Looks like you've got a bad indent here.
Mark Pilgrim (Google)
Comment 11 2012-09-27 10:31:18 PDT
Created attachment 166029 [details] Patch
Mark Pilgrim (Google)
Comment 12 2012-09-27 10:31:38 PDT
Comment on attachment 166029 [details] Patch Nits addressed.
WebKit Review Bot
Comment 13 2012-09-27 12:48:42 PDT
Comment on attachment 166029 [details] Patch Clearing flags on attachment: 166029 Committed r129792: <http://trac.webkit.org/changeset/129792>
WebKit Review Bot
Comment 14 2012-09-27 12:48:46 PDT
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.