Bug 52826 – [Chromium] Use WebThemeEngine for relevant RenderTheme parts for chromium/linux

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[xiyuan]

We have moved/implemented the drawing code into NativeThemeLinux and now need to use WebThemeEngine to hook it up with RenderTheme code.

[xiyuan]

Created attachment 79628 [details]
Proposed code change and new baseline for layout tests.

[Tony Chang]

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

[xiyuan]

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

[Tony Chang]

(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!

[xiyuan]

Created attachment 79678 [details]
Address tony's comments @ 2

[WebKit Review Bot]

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.

[xiyuan]

Note that this change need to land after http://src.chromium.org/viewvc/chrome?view=rev&revision=71969 is rolled in.

[xiyuan]

Created attachment 79679 [details]
Fix style @ 6

[Tony Chang]

Comment on attachment 79679 [details]
Fix style @ 6

static const unsigned kDefaultButtonBackgroundColor = 0xffdddddd;

This should be defaultButtonBackgroundColor.

[xiyuan]

Created attachment 79692 [details]
Address comments @ 9

[xiyuan]

Comment on attachment 79692 [details]
Address comments @ 9

Please take another look. Thanks.

[xiyuan]

Created attachment 79748 [details]
Sync and resolve.

Synced with ToT and adapted to r76340 (ChromiumBridge -> PlatformBridge rename change).

[WebKit Commit Bot]

Comment on attachment 79748 [details]
Sync and resolve.

Clearing flags on attachment: 79748

Committed r76379: <http://trac.webkit.org/changeset/76379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.