Allow getting the theme tint as a value so that it can be applied independent...

Allow getting the theme tint as a value so that it can be applied independent of the theme provider. Since the Mac needs it to tint its vector resources it's added to the Mac.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19751

[Avi (use Gerrit), 2009-07-01 16:28:53]

erik: theme code
pink: mac stuff

[pink (ping after 24hrs), 2009-07-01 16:43:25]

mac stuff LGTM

[Erik does not do reviews, 2009-07-01 16:51:05]

LGTM - just comment nits and a question

http://codereview.chromium.org/151153/diff/1/2
File app/theme_provider.h (right):

http://codereview.chromium.org/151153/diff/1/2#newcode75
Line 75: // Gets the NSColor for tinting with the specified |id|. Returns a
pointer to
newline above comment

http://codereview.chromium.org/151153/diff/1/2#newcode77
Line 77: // theme provider and should not be freed.
Why does this need to be explicitly spelled out?  This is the standard pattern
in Foundation/Cocoa (retain if you want to keep the object around yourself,
otherwise expect it to be released by others at some undetermined time in the
future).

http://codereview.chromium.org/151153/diff/1/4
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/151153/diff/1/4#newcode185
Line 185: typedef std::map<int, NSColor*> NSColorMap;
I assume that this is a pattern that the mac team has agreed on, but it looks
weird to me.  Shouldn't we be using Foundation containers?  Without that, we
have to do the retain/release manually, which is more error prone.  This is just
my curiosity, not something I'm asking you to change for this review.

http://codereview.chromium.org/151153/diff/1/5
File chrome/browser/browser_theme_provider_mac.mm (right):

http://codereview.chromium.org/151153/diff/1/5#newcode68
Line 68: // determine how much this matters and fix it if necessary.
file a bug for TODO and reference

[pink (ping after 24hrs), 2009-07-01 16:55:56]

http://codereview.chromium.org/151153/diff/1/4
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/151153/diff/1/4#newcode185
Line 185: typedef std::map<int, NSColor*> NSColorMap;
I thought about this as well, but the foundation containers (NSMutableSet)
require you to use an object as the key. So we'd still have to create an
NSNumber or NSValue object out of the int and manage its lifetime. Six of one.

[Avi (use Gerrit), 2009-07-01 18:41:04]

All items addressed unless otherwise noted.

http://codereview.chromium.org/151153/diff/1/2
File app/theme_provider.h (right):

http://codereview.chromium.org/151153/diff/1/2#newcode77
Line 77: // theme provider and should not be freed.
On 2009/07/01 16:51:06, Erik Kay wrote:
> Why does this need to be explicitly spelled out?

(shrug) It's kinda paralleling the comment above for the other platform. No big
deal; will remove.

http://codereview.chromium.org/151153/diff/1/4
File chrome/browser/browser_theme_provider.h (right):

http://codereview.chromium.org/151153/diff/1/4#newcode185
Line 185: typedef std::map<int, NSColor*> NSColorMap;
It's done to parallel how the other platforms are doing things. Since this is a
shared header, I'm trying to keep the ObjC-isms down to a minimum anyway.