Bug 102403 - [chromium] Copy linux theme related files to default
Summary: [chromium] Copy linux theme related files to default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-15 09:29 PST by Scott Violet
Modified: 2012-11-16 10:38 PST (History)
2 users (show)

See Also:


Attachments
Patch (92.01 KB, patch)
2012-11-15 10:09 PST, Scott Violet
no flags Details | Formatted Diff | Diff
Patch (92.03 KB, patch)
2012-11-15 15:17 PST, Scott Violet
no flags Details | Formatted Diff | Diff
Patch for landing (62.70 KB, patch)
2012-11-15 16:52 PST, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Violet 2012-11-15 09:29:29 PST
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.
Comment 1 Scott Violet 2012-11-15 10:09:01 PST
Created attachment 174477 [details]
Patch
Comment 2 Scott Violet 2012-11-15 10:15:11 PST
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);
Comment 3 Scott Violet 2012-11-15 10:37:56 PST
It looks like the bots are having problems applying the patch. Most likely because it svn cp's files. Any suggestions?
Comment 4 Tony Chang 2012-11-15 10:45:32 PST
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.
Comment 5 Scott Violet 2012-11-15 15:17:55 PST
Created attachment 174531 [details]
Patch
Comment 6 Tony Chang 2012-11-15 16:52:52 PST
Created attachment 174559 [details]
Patch for landing
Comment 7 Tony Chang 2012-11-15 16:53:31 PST
I remade the patch from a git repo.  I hope this will work (although it may lose some svn rename history).
Comment 8 WebKit Review Bot 2012-11-15 17:44:02 PST
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
Comment 9 Scott Violet 2012-11-16 07:20:44 PST
Failure looks like flakyness (I see it referenced in this bug https://bugs.webkit.org/show_bug.cgi?id=102451 too.
Comment 10 Scott Violet 2012-11-16 07:21:31 PST
I can't seem to change the patch to CQ+.
Comment 11 WebKit Review Bot 2012-11-16 10:38:33 PST
Comment on attachment 174559 [details]
Patch for landing

Clearing flags on attachment: 174559

Committed r134969: <http://trac.webkit.org/changeset/134969>
Comment 12 WebKit Review Bot 2012-11-16 10:38:36 PST
All reviewed patches have been landed.  Closing bug.