Bug 24456 - Chromium Unforking: Split ColorChromium.cpp into Windows + Mac variants
Summary: Chromium Unforking: Split ColorChromium.cpp into Windows + Mac variants
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-08 10:39 PDT by Jeremy Moskovich
Modified: 2009-03-19 09:43 PDT (History)
2 users (show)

See Also:


Attachments
patch (12.37 KB, patch)
2009-03-08 10:43 PDT, Jeremy Moskovich
eric: review-
Details | Formatted Diff | Diff
patch1 (9.36 KB, patch)
2009-03-11 06:29 PDT, Jeremy Moskovich
fishd: review-
Details | Formatted Diff | Diff
patch2 (9.47 KB, patch)
2009-03-18 16:26 PDT, Jeremy Moskovich
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2009-03-08 10:39:56 PDT
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.
Comment 1 Jeremy Moskovich 2009-03-08 10:43:06 PDT
Created attachment 28408 [details]
patch
Comment 2 Eric Seidel (no email) 2009-03-08 22:34:14 PDT
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.
Comment 3 Jeremy Moskovich 2009-03-09 01:46:52 PDT
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.
Comment 4 Darin Fisher (:fishd, Google) 2009-03-09 09:22:25 PDT
What is the solution for Chromium Linux?  Wouldn't the current patch break the Linux build?
Comment 5 Adam Langley 2009-03-09 10:25:52 PDT
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.
Comment 6 Jeremy Moskovich 2009-03-11 06:29:56 PDT
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.
Comment 7 Darin Fisher (:fishd, Google) 2009-03-15 08:40:26 PDT
Comment on attachment 28477 [details]
patch1

>+++ b/WebCore/ChangeLog
>+2009-03-11  Jeremy Moskovich  <jeremy@chromium.org>
>+
>+        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?
Comment 8 Jeremy Moskovich 2009-03-18 16:26:01 PDT
Created attachment 28736 [details]
patch2

Fixes for Darines comments (thanks!)
Comment 9 Dimitri Glazkov (Google) 2009-03-19 09:43:51 PDT
Landed as http://trac.webkit.org/changeset/41839.