Bug 102403 – [chromium] Copy linux theme related files to default

• Component: Platform
• Status: RESOLVED FIXED

[Scott Violet]

As part of making it possible for windows to use the linux theme we need to copy the linux theme files and build them with a GYP_DEFINE. Once chrome is updated we can nuke the linux ones.

[Scott Violet]

Created attachment 174477 [details]
Patch

[Scott Violet]

There are a 2 style issues in the patch. One in RenderThemeChromiumDefault:

    static void setSelectionColors(unsigned activeBackgroundColor,
                                   unsigned activeForegroundColor,
                                   unsigned inactiveBackgroundColor,
                                   unsigned inactiveForegroundColor);

The style guide doesn't mention wrapping. Should I convert to a single line, eg:

    static void setSelectionColors(unsigned activeBackgroundColor, unsigned activeForegroundColor, unsigned inactiveBackgroundColor, unsigned inactiveForegroundColor);


And similarly a call site:

    RenderThemeChromiumDefault::setSelectionColors(activeBackgroundColor,
                                                   activeForegroundColor,
                                                   inactiveBackgroundColor,
                                                   inactiveForegroundColor);

[Scott Violet]

It looks like the bots are having problems applying the patch. Most likely because it svn cp's files. Any suggestions?

[Tony Chang]

Comment on attachment 174477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174477&action=review

> Source/Platform/ChangeLog:6
> +        Reviewed by Tony Change.

You should leave this as "NOBODY (OOPS!)" until you get an r+.  webkit-patch or the cq will fill in the reviewer for you.

> Source/WebCore/rendering/RenderThemeChromiumDefault.h:92
> +    static void setSelectionColors(unsigned activeBackgroundColor,
> +                                   unsigned activeForegroundColor,
> +                                   unsigned inactiveBackgroundColor,
> +                                   unsigned inactiveForegroundColor);

You can either unwrap this into a single line or just indent 4 spaces from the previous line.  I would probably just leave it on a single line.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3570
> +    RenderThemeChromiumDefault::setSelectionColors(activeBackgroundColor,
> +                                                   activeForegroundColor,
> +                                                   inactiveBackgroundColor,
> +                                                   inactiveForegroundColor);

Same as above, either unwrap (seems easier) or indent 4 spaces from the previous line.

[Scott Violet]

Created attachment 174531 [details]
Patch

[Tony Chang]

Created attachment 174559 [details]
Patch for landing

[Tony Chang]

I remade the patch from a git repo.  I hope this will work (although it may lose some svn rename history).

[WebKit Review Bot]

Comment on attachment 174559 [details]
Patch for landing

Rejecting attachment 174559 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Full output: http://queues.webkit.org/results/14833930

[Scott Violet]

Failure looks like flakyness (I see it referenced in this bug https://bugs.webkit.org/show_bug.cgi?id=102451 too.

[Scott Violet]

I can't seem to change the patch to CQ+.

[WebKit Review Bot]

Comment on attachment 174559 [details]
Patch for landing

Clearing flags on attachment: 174559

Committed r134969: <http://trac.webkit.org/changeset/134969>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.