Bug 24456 – Chromium Unforking: Split ColorChromium.cpp into Windows + Mac variants

• Component: Platform
• Status: RESOLVED FIXED

[Jeremy Moskovich]

Split ColorChromium.cpp into Mac & Windows variants.

Remove Chromium Dependency on platform/graphics/mac/ColorMac.mm since we ultimately need to take a different approach (e.g. in the face of the sandbox we need to ship things over IPC from the browser process).  For now, createCGColor() is copied from ColorMac.mm.

[Jeremy Moskovich]

Created attachment 28408 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 28408 [details]
patch

Why is it so important to remove the dependance on ColorMac.mm?  It seems that it would be better for us to not duplicate code as a general rule.

Splitting out the win version seems fine, but it seems it also needs a comment about Windows listening to the browser process for focus changes.  Probably the "getting the focus ring color from the browser process" could be x-platform code.

[Jeremy Moskovich]

Thanks for looking at this Eric,

This CL puts is into an intermediate state but I think the comments point clearly at where we want to go.

While I'd like to land something finished with everything fully plumed through, splitting the files up and adding comments creates an easier path to move in that direction and allows us to unfork our copy of ColorMac.mm.

The motivation for taking the code from ColorMac is that, we do want to do our own thing [in the face of MP & the Sandbox], and while duplicating code sucks, IMHO the small quantity and comments are cleaner than other potential solutions (upstreaming our ifdefed version of ColorMac.mm).

If you think there's a way we can comment things more clearly, or file bugs to track this more closely I'm all for it.

Good point about the tint color changed notification on the Windows side, I'll ping the relevant people and see what the right thing to do is.

[Darin Fisher (:fishd, Google)]

What is the solution for Chromium Linux?  Wouldn't the current patch break the Linux build?

[Adam Langley]

For Linux, one could either change ColorChromiumWin.* to ColorChromium.*, or duplicate the Windows code to ChromiumLinux.cpp. At the moment, we have no plans to do anything other than pick a static colour.

[Jeremy Moskovich]

Created attachment 28477 [details]
patch1

Sliced stuff a little differently:
ColorChromium.cpp vs ColorChromiumMac.mm.

Added comment to reflect the fact that we are content to use a static focus ring color on Windows/Linux.

[Darin Fisher (:fishd, Google)]

Comment on attachment 28477 [details]
patch1

>+++ b/WebCore/ChangeLog
>[phone]  Jeremy Moskovich  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Split ColorChromium.cpp into Mac & Windows variants.
>+        Remove Chromium Dependency on platform/graphics/mac/ColorMac.mm since we
>+        ultimately need to take a different approach.  For now, createCGColor()
>+        is copied from ColorMac.mm.
>+
>+        No observable change in behavior, so no test.

nit: please add a bug link in here.


>+
>+        * platform/graphics/chromium/ColorChromium.cpp:
>+        (WebCore::focusRingColor):
>+        * platform/graphics/chromium/ColorChromiumMac.mm: Added.
>+        (WebCore::focusRingColor):
>+        (WebCore::nsColor):
>+        (WebCore::CGColorFromNSColor):
>+        (WebCore::createCGColor):

nit: for newly added files, please remove the lines that mention the newly added
functions.  i wish prepare-ChangeLog did that automatically :-/


>+++ b/WebCore/platform/graphics/chromium/ColorChromiumMac.mm
>@@ -0,0 +1,131 @@
>+/*
>+ * Copyright (C) 2008 Google Inc. All rights reserved.

This copyright info is not correct, right?

[Jeremy Moskovich]

Created attachment 28736 [details]
patch2

Fixes for Darines comments (thanks!)

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/41839.