Summary: | [Chromium] Use WebThemeEngine for relevant RenderTheme parts for chromium/linux | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | xiyuan <xiyuan> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | agl, commit-queue, davemoore, eric, evan, tony, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Attachments: |
|
Description
xiyuan
2011-01-20 11:38:31 PST
Created attachment 79628 [details]
Proposed code change and new baseline for layout tests.
Comment on attachment 79628 [details] Proposed code change and new baseline for layout tests. View in context: https://bugs.webkit.org/attachment.cgi?id=79628&action=review r- for style nits. Can you upload some of image diffs? I'm having a hard time seeing the difference. Maybe it's just a few rgb points off? > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:58 > + return ChromiumBridge::StateHover; 4 space indent here and above 2 conditions > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:196 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartCheckbox, > + getWebThemeState(this, o), > + rect, > + &extraParams); I think this should just be one long line (maybe one line of wrapping if it's really long). > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:217 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartRadio, less wrapping > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:238 > + extraParams.button.backgroundColor = SkColorSetRGB(0xdd, 0xdd, 0xdd); Should this color be a constant somewhere? > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:244 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartButton, less wrapping > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:294 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartMenuList, less wrapping > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:308 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartSliderTrack, less wrapping > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:323 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartSliderThumb, less wrapping > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:346 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartInnerSpinButton, less wrapping > Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:372 > + ChromiumBridge::paintThemePart( > + i.context, > + ChromiumBridge::PartProgressBar, less wrapping Created attachment 79676 [details]
Layout test results to show image diff
This is the layout test results of broken image tests. It's pretty hard to tell the difference with naked eyes. :p
(In reply to comment #3) > Created an attachment (id=79676) [details] > Layout test results to show image diff > > This is the layout test results of broken image tests. It's pretty hard to tell the difference with naked eyes. :p I agree, these all look fine. Thanks for uploading your results! Created attachment 79678 [details]
Address tony's comments @ 2
Attachment 79678 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/RenderThemeChromiumLinux.cpp:55: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 1 in 45 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Note that this change need to land after http://src.chromium.org/viewvc/chrome?view=rev&revision=71969 is rolled in. Created attachment 79679 [details]
Fix style @ 6
Comment on attachment 79679 [details]
Fix style @ 6
static const unsigned kDefaultButtonBackgroundColor = 0xffdddddd;
This should be defaultButtonBackgroundColor.
Created attachment 79692 [details]
Address comments @ 9
Comment on attachment 79692 [details]
Address comments @ 9
Please take another look. Thanks.
Created attachment 79748 [details] Sync and resolve. Synced with ToT and adapted to r76340 (ChromiumBridge -> PlatformBridge rename change). Comment on attachment 79748 [details] Sync and resolve. Clearing flags on attachment: 79748 Committed r76379: <http://trac.webkit.org/changeset/76379> All reviewed patches have been landed. Closing bug. |