Rejigger ThemeService: move exposure of ThemeProvider interface to a

Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377, 568388
Committed: https://crrev.com/68691b28a76845494f3b51d02e24844a69833032
Cr-Commit-Position: refs/heads/master@{#364812}

[Evan Stade, 2015-12-04 01:16:56]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 01:19:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492423003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492423003/20001

[commit-bot: I haz the power, 2015-12-04 01:40:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 01:40:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2015-12-04 16:07:10]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org, pkotwicz@chromium.org

[Evan Stade, 2015-12-04 16:07:11]

(oops, forgot to add reviewers)

[pkotwicz, 2015-12-04 23:16:50]

Generally looks good. Thank you for making this change.

You probably want to wait for pkasting@ to agree that the general design's good
before proceeding further though

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:129: enum IncognitoThemeProperty {
Nit: Please add a comment that the int value of the properties has no special
meaning

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:459: SkColor color;
CustomThemeProviders do provide some incognito colors.
COLOR_FRAME_INCOGNITO and COLOR_FRAME_INCOGNITO_INACTIVE in particular

I think that this is clearer:

if (theme_supplier_) {
  SkColor color;
  if (theme_supplier_->GetColor(id, &color))
    return color;
  int original_id = Properties::IncognitoPropertyToProperty(id)
  if (id != original_id && theme_supplier_->GetColor(original_id, &color)
    return color;
}

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:541:
GetColor(Properties::COLOR_NTP_BACKGROUND)))
Nit: Alignment looks off

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:713: const ui::ThemeProvider&
ThemeService::GetThemeProviderForProfile(
I kind of like the idea of having
ThemeProviderFactory::GetForProfile()

That would require adding accessors to |ThemeService::original_theme_provider_|
and |ThemeService::otr_theme_provider_| to the ThemeService public API.

I will differ to pkasting@ for his opinion

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:100: // Also overridden for GTK.
How about: "Whether we are using the system theme. On GTK, the system theme is
the GTK theme, not the "Classic" theme."

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:124: static const ui::ThemeProvider&
GetThemeProviderForProfile(Profile* profile);
Is returning a const reference from a static method a new pattern?
Returning a const star seems much more prevalent in the code base.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:173: // incognito (otr) or normal.
How about: "This class implements ui::ThemeProvider on behalf of ThemeService.
It also converts requested property ids to "off the record" (otr) property ids
for incognito profiles."

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:197: const ThemeService& theme_service_;
Nit: Make this a const pointer (const ThemeService*)

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:198: bool otr_;
Might as well name this variable |off_the_record_|.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:216: bool UsingSystemTheme() const;
UsingSystemTheme() should not be present twice in ThemeService.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ui.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ui.cc:18: if (!service->UsingDefaultTheme())
Nit: You only need to fetch the ThemeProvider if the default theme is not used

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc
(right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc:60: 
Nit: Add a comment that |profile_| is not owned.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:106: }
Is OnThemeChanged() necessary? I don't see where this class accesses theme data.

Regardless of whether OnThemeChanged() is necessary, splitting off the changes
to this file into a separate CL sounds like a good idea.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:342:
ThemeService::GetThemeProviderForProfile(profile_);
This looks like |profile_| is always the non-OTR profile. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/theme_source.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/theme_source.cc:210:
ThemeService::GetThemeProviderForProfile(profile_);
This looks like its always the non-OTR profile. See line 70

https://codereview.chromium.org/1492423003/diff/20001/ui/base/theme_provider.h
File ui/base/theme_provider.h (right):

https://codereview.chromium.org/1492423003/diff/20001/ui/base/theme_provider....
ui/base/theme_provider.h:78: // same as the default theme). TODO(estade): this
should probably just be part
Nit: TODO on new line

[Evan Stade, 2015-12-07 19:05:12]

> You probably want to wait for pkasting@ to agree that the general design's
good before proceeding further though

Sounds reasonable. pkasting, WDYT before I start responding to pkotwicz's
comments?

[Peter Kasting, 2015-12-08 22:52:58]

General approach seems OK.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:134: static int
PropertyToIncognitoProperty(int id);
If you follow my suggestion in theme_service.cc, I think the only usage of
IncognitoPropertyToProperty() goes away, and you're left with one usage of
PropertyToIncognitoProperty() inside ThemeService::GetColor().

At that point I think it would be better to eliminate this other function and do
the mapping directly there.  That basically encapsulates all the complication
about how colors are mapped inside ThemeService and leaves this class as a dumb
vendor.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:743: otr_ ?
ThemeProperties::PropertyToIncognitoProperty(id) : id;
Rather than map here, and then map back the other way in
ThemeService::GetColor(), I would make ThemeService::GetColor() take two args, a
color ID and a bool about whether the profile is off the record, and then let it
decide how to map colors based on those params.

[Evan Stade, 2015-12-09 00:57:11]

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:129: enum IncognitoThemeProperty {
On 2015/12/04 23:16:49, pkotwicz wrote:
> Nit: Please add a comment that the int value of the properties has no special
> meaning

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:134: static int
PropertyToIncognitoProperty(int id);
On 2015/12/08 22:52:58, Peter Kasting wrote:
> If you follow my suggestion in theme_service.cc, I think the only usage of
> IncognitoPropertyToProperty() goes away, and you're left with one usage of
> PropertyToIncognitoProperty() inside ThemeService::GetColor().
> 
> At that point I think it would be better to eliminate this other function and
do
> the mapping directly there.  That basically encapsulates all the complication
> about how colors are mapped inside ThemeService and leaves this class as a
dumb
> vendor.

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:459: SkColor color;
On 2015/12/04 23:16:49, pkotwicz wrote:
> CustomThemeProviders do provide some incognito colors.
> COLOR_FRAME_INCOGNITO and COLOR_FRAME_INCOGNITO_INACTIVE in particular
> 
> I think that this is clearer:
> 
> if (theme_supplier_) {
>   SkColor color;
>   if (theme_supplier_->GetColor(id, &color))
>     return color;
>   int original_id = Properties::IncognitoPropertyToProperty(id)
>   if (id != original_id && theme_supplier_->GetColor(original_id, &color)
>     return color;
> }

changed to pkasting's suggestion

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:541:
GetColor(Properties::COLOR_NTP_BACKGROUND)))
On 2015/12/04 23:16:49, pkotwicz wrote:
> Nit: Alignment looks off

in what way? this is the output of git cl format

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:713: const ui::ThemeProvider&
ThemeService::GetThemeProviderForProfile(
On 2015/12/04 23:16:49, pkotwicz wrote:
> I kind of like the idea of having
> ThemeProviderFactory::GetForProfile()
> 
> That would require adding accessors to
|ThemeService::original_theme_provider_|
> and |ThemeService::otr_theme_provider_| to the ThemeService public API.
> 
> I will differ to pkasting@ for his opinion

hmm, pkasting, wdyt? I like having this factory function be part of
ThemeService. It seems a bit heavyweight (and encapsulation-breaking) to
introduce a whole new class that you have to friend just to implement a single
function.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:743: otr_ ?
ThemeProperties::PropertyToIncognitoProperty(id) : id;
On 2015/12/08 22:52:58, Peter Kasting wrote:
> Rather than map here, and then map back the other way in
> ThemeService::GetColor(), I would make ThemeService::GetColor() take two args,
a
> color ID and a bool about whether the profile is off the record, and then let
it
> decide how to map colors based on those params.

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:100: // Also overridden for GTK.
On 2015/12/04 23:16:49, pkotwicz wrote:
> How about: "Whether we are using the system theme. On GTK, the system theme is
> the GTK theme, not the "Classic" theme."

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:124: static const ui::ThemeProvider&
GetThemeProviderForProfile(Profile* profile);
On 2015/12/04 23:16:50, pkotwicz wrote:
> Is returning a const reference from a static method a new pattern?
> Returning a const star seems much more prevalent in the code base.

a new pattern? No, it has existed for a while. The only difference is that a
pointer can be null and a reference implies it will always be non-null. It's
true there are many places in the codebase where we are using const* but should
probably use const&, and places where we check null-ness when we don't need to,
or don't when we do. I don't think there are any places where we return const&
when we should return const* instead (it's hard to make the mistake in that
direction).

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:173: // incognito (otr) or normal.
On 2015/12/04 23:16:50, pkotwicz wrote:
> How about: "This class implements ui::ThemeProvider on behalf of ThemeService.
> It also converts requested property ids to "off the record" (otr) property ids
> for incognito profiles."

adjusted

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:197: const ThemeService& theme_service_;
On 2015/12/04 23:16:49, pkotwicz wrote:
> Nit: Make this a const pointer (const ThemeService*)

why? It should never be null and this makes that clear.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:198: bool otr_;
On 2015/12/04 23:16:50, pkotwicz wrote:
> Might as well name this variable |off_the_record_|.

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:216: bool UsingSystemTheme() const;
On 2015/12/04 23:16:50, pkotwicz wrote:
> UsingSystemTheme() should not be present twice in ThemeService.

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ui.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ui.cc:18: if (!service->UsingDefaultTheme())
On 2015/12/04 23:16:50, pkotwicz wrote:
> Nit: You only need to fetch the ThemeProvider if the default theme is not used

Done.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc
(right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc:60: 
On 2015/12/04 23:16:50, pkotwicz wrote:
> Nit: Add a comment that |profile_| is not owned.

I'm pretty sure that's clear enough that a comment is just added noise. Raw
pointers for owned objects are pretty much banned at this point, and it's a
common pattern to hold onto a pointer to the relevant Profile across many
classes in Chrome so ownership is not implied.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:106: }
On 2015/12/04 23:16:50, pkotwicz wrote:
> Is OnThemeChanged() necessary? I don't see where this class accesses theme
data.
> 
> Regardless of whether OnThemeChanged() is necessary, splitting off the changes
> to this file into a separate CL sounds like a good idea.

you're right, it's not necessary. I forgot to remove this when I changed
UpdateState to not care about the theme (since it doesn't seem to actually be a
themed image).

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:342:
ThemeService::GetThemeProviderForProfile(profile_);
On 2015/12/04 23:16:50, pkotwicz wrote:
> This looks like |profile_| is always the non-OTR profile. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

well, I suppose we might need to change that in the future, but for now there's
no incognito version of IDR_THEME_NTP_BACKGROUND so it doesn't matter.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/theme_source.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/theme_source.cc:210:
ThemeService::GetThemeProviderForProfile(profile_);
On 2015/12/04 23:16:50, pkotwicz wrote:
> This looks like its always the non-OTR profile. See line 70

yes, but GetRawData doesn't change for incognito yet (and maybe never will)

https://codereview.chromium.org/1492423003/diff/20001/ui/base/theme_provider.h
File ui/base/theme_provider.h (right):

https://codereview.chromium.org/1492423003/diff/20001/ui/base/theme_provider....
ui/base/theme_provider.h:78: // same as the default theme). TODO(estade): this
should probably just be part
On 2015/12/04 23:16:50, pkotwicz wrote:
> Nit: TODO on new line

Done.

[Peter Kasting, 2015-12-09 01:32:00]

It seems vaguely unfortunate that a lot of places seem to have to get the
ThemeService only to ask if it's using the system theme, and then do everything
else with the theme provider.  I see why you moved it (since it's per-user
data), but I wonder if it might not be better left as is, or maybe a forwarding
function added to the theme provider?  Doesn't matter much I guess.

LGTM with a couple of comments.

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:713: const ui::ThemeProvider&
ThemeService::GetThemeProviderForProfile(
On 2015/12/09 00:57:11, Evan Stade wrote:
> On 2015/12/04 23:16:49, pkotwicz wrote:
> > I kind of like the idea of having
> > ThemeProviderFactory::GetForProfile()
> > 
> > That would require adding accessors to
> |ThemeService::original_theme_provider_|
> > and |ThemeService::otr_theme_provider_| to the ThemeService public API.
> > 
> > I will differ to pkasting@ for his opinion
> 
> hmm, pkasting, wdyt? I like having this factory function be part of
> ThemeService. It seems a bit heavyweight (and encapsulation-breaking) to
> introduce a whole new class that you have to friend just to implement a single
> function.

I don't feel like I care much either way.  The idea of getting the ThemeProvider
from a "theme provider factory" instead of the theme service is sort of
appealing, but mostly on a superficial level; I don't think it makes anything
easier or clearer, really.  So leaving it as you have it seems OK, especially
since it seems like that will be shorter.

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:135: static int
PropertyToIncognitoProperty(int id);
I thought you were going to eliminate this function and move its functionality
inside ThemeService::GetColor()?  That had been the intent of my suggestion
anyway...

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:170: friend class
theme_service_internal::ThemeServiceTest;
Nit: I would keep the two friend declarations together (below the member class
declaration)

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ui.cc (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ui.cc:15: ThemeService* service =
ThemeServiceFactory::GetForProfile(profile);
Nit: Could inline into next line (2 places)

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:210:
ThemeServiceFactory::GetForProfile(profile_);
Nit: Can just inline into next line

[Evan Stade, 2015-12-09 01:57:06]

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:135: static int
PropertyToIncognitoProperty(int id);
On 2015/12/09 01:31:59, Peter Kasting wrote:
> I thought you were going to eliminate this function and move its functionality
> inside ThemeService::GetColor()?  That had been the intent of my suggestion
> anyway...

ah, ok, I guess we can do that. I thought you just wanted to delete one of the
two new functions. In fact we might be better off making GetDefaultColor take an
otr bool parameter and not convert at all in ThemeService? At least then we
could make the IncognitoThemeProperty enum private. It's hard for me to judge
whether that will be sufficient for all future purposes.

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:170: friend class
theme_service_internal::ThemeServiceTest;
On 2015/12/09 01:31:59, Peter Kasting wrote:
> Nit: I would keep the two friend declarations together (below the member class
> declaration)

Done.

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_ui.cc (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_ui.cc:15: ThemeService* service =
ThemeServiceFactory::GetForProfile(profile);
On 2015/12/09 01:31:59, Peter Kasting wrote:
> Nit: Could inline into next line (2 places)

Done.

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1492423003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:210:
ThemeServiceFactory::GetForProfile(profile_);
On 2015/12/09 01:31:59, Peter Kasting wrote:
> Nit: Can just inline into next line

Done.

[Evan Stade, 2015-12-09 01:59:06]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[pkotwicz, 2015-12-09 02:08:29]

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:342:
ThemeService::GetThemeProviderForProfile(profile_);
This class fetches colors as well using the ThemeProvider for the incorrect
profile. e.g. line 549

[Evan Stade, 2015-12-09 23:38:40]

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:342:
ThemeService::GetThemeProviderForProfile(profile_);
On 2015/12/09 02:08:29, pkotwicz wrote:
> This class fetches colors as well using the ThemeProvider for the incorrect
> profile. e.g. line 549

are you asking me to fix that right now, or pointing out that this will prevent
the approach in this CL from ever working? There is a lot of follow up needed to
this CL, and there are many places that will need to adjust how they get colors.
I agree that L549 is one place that might change (although it technically works
as-is). Changes will be piecemeal, but will only affect MD, so e.g. normal
canary users won't see half-broken stuff.

[Evan Stade, 2015-12-10 00:28:21]

https://codereview.chromium.org/1492423003/diff/100001/chrome/browser/themes/...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/1492423003/diff/100001/chrome/browser/themes/...
chrome/browser/themes/theme_properties.h:159: static SkColor GetDefaultColor(int
id, bool otr);
check it out, new enum is now gone.

[Peter Kasting, 2015-12-10 00:38:27]

On 2015/12/10 00:28:21, Evan Stade wrote:
>
https://codereview.chromium.org/1492423003/diff/100001/chrome/browser/themes/...
> File chrome/browser/themes/theme_properties.h (right):
> 
>
https://codereview.chromium.org/1492423003/diff/100001/chrome/browser/themes/...
> chrome/browser/themes/theme_properties.h:159: static SkColor
GetDefaultColor(int
> id, bool otr);
> check it out, new enum is now gone.

I probably slightly preferred having the logic in ThemeService, but it really
doesn't matter much; if you like this better, fine.

[pkotwicz, 2015-12-10 01:02:02]

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:342:
ThemeService::GetThemeProviderForProfile(profile_);
I am asking you to fix this right now or to commit to fixing this immediately
after this CL lands.

Most likely someone who changes ThemeProperties::GetDefaultColor() will assume
|otr| is correct on all OSes but Mac. This makes it almost guaranteed that if
someone modifies ThemeProperties::GetDefaultColor() that they will introduce
bugs.

If you want to fix this immediately after this CL lands:
1) Please only use the two argument version ThemeProperties::GetDefaultColor()
for COLOR_TOOLBAR
2) Add a comment that |otr| is not appropriate for deciding whether to use an
incognito color except in the very special case of COLOR_TOOLBAR.
Also please file a bug and include a bug number in the comment

[Evan Stade, 2015-12-10 01:31:17]

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1492423003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:342:
ThemeService::GetThemeProviderForProfile(profile_);
On 2015/12/10 01:02:02, pkotwicz wrote:
> I am asking you to fix this right now or to commit to fixing this immediately
> after this CL lands.
> 
> Most likely someone who changes ThemeProperties::GetDefaultColor() will assume
> |otr| is correct on all OSes but Mac. This makes it almost guaranteed that if
> someone modifies ThemeProperties::GetDefaultColor() that they will introduce
> bugs.
> 
> If you want to fix this immediately after this CL lands:
> 1) Please only use the two argument version ThemeProperties::GetDefaultColor()
> for COLOR_TOOLBAR
> 2) Add a comment that |otr| is not appropriate for deciding whether to use an
> incognito color except in the very special case of COLOR_TOOLBAR.
> Also please file a bug and include a bug number in the comment

I don't understand the urgency. There are no special colors for OTR in pre-MD.
Why would someone besides me be adding a color that cared about OTR status?

[Evan Stade, 2015-12-10 02:03:48]

Description was changed from

==========
Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377
==========

to

==========
Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377, 568388
==========

[Evan Stade, 2015-12-10 02:04:44]

> 
> I don't understand the urgency. There are no special colors for OTR in pre-MD.
> Why would someone besides me be adding a color that cared about OTR status?

filed bug and added comment per off-line chat with pko

[pkotwicz, 2015-12-10 17:20:12]

LGTM. Thank you for bearing with me

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.h:125: static const ui::ThemeProvider&
GetThemeProviderForProfile(Profile* profile);
Nit: Add a comment for this function

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.h:174: BrowserThemeProvider(const
ThemeService& theme_service, bool otr);
Nit: Might as well name the parameter |off_the_record| given that the member
variable is called |off_the_record_|

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.h:226: virtual gfx::Image GetImageNamed(int
id) const;
Nit: I don't think that this method needs to be virtual. I believe that it has
not needed to be virtual for a long time (Ever since we started using Aura on
Desktop Linux)

[Evan Stade, 2015-12-10 18:10:54]

estade@chromium.org changed reviewers:
+ thakis@chromium.org

[Evan Stade, 2015-12-10 18:10:54]

+thakis for

    chrome/browser/chromeos/login/ui/simple_web_view_dialog.cc
    chrome/browser/search/*
    ui/base/*

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.h:125: static const ui::ThemeProvider&
GetThemeProviderForProfile(Profile* profile);
On 2015/12/10 17:20:12, pkotwicz wrote:
> Nit: Add a comment for this function

Done.

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.h:174: BrowserThemeProvider(const
ThemeService& theme_service, bool otr);
On 2015/12/10 17:20:12, pkotwicz wrote:
> Nit: Might as well name the parameter |off_the_record| given that the member
> variable is called |off_the_record_|

Done.

https://codereview.chromium.org/1492423003/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.h:226: virtual gfx::Image GetImageNamed(int
id) const;
On 2015/12/10 17:20:12, pkotwicz wrote:
> Nit: I don't think that this method needs to be virtual. I believe that it has
> not needed to be virtual for a long time (Ever since we started using Aura on
> Desktop Linux)

Done.

[Evan Stade, 2015-12-10 18:11:00]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 18:12:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492423003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492423003/140001

[Nico, 2015-12-10 18:14:09]

rs-lgtm

[commit-bot: I haz the power, 2015-12-10 18:30:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 18:30:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2015-12-10 18:57:20]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 18:59:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492423003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492423003/160001

[commit-bot: I haz the power, 2015-12-10 19:20:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 19:20:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Evan Stade, 2015-12-10 21:15:32]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 21:16:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492423003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492423003/200001

[commit-bot: I haz the power, 2015-12-10 21:49:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 21:49:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Evan Stade, 2015-12-10 23:27:07]

estade@chromium.org changed reviewers:
+ groby@chromium.org

[Evan Stade, 2015-12-10 23:27:07]

+groby, can you review the cocoa files? Had to make some changes since last
review to get OSX to work.

+pkasting, can you review new changes to
chrome/browser/ui/views/toolbar/toolbar_button_test.cc ? I removed this file
because it only had one test which apparently never worked and no one owns.
Updating it was going to be a pain because I can't just extend
ChromeRenderViewHostTestHarness.

[Evan Stade, 2015-12-10 23:27:16]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 23:27:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492423003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492423003/240001

[Peter Kasting, 2015-12-10 23:54:10]

LGTM

[commit-bot: I haz the power, 2015-12-11 00:28:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 00:28:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[groby-ooo-7-16, 2015-12-11 02:32:01]

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2459: -
(Profile*)profile {
Do we need this method at all?

The protocol is only implemented by BookmarkBarController, which already exposes
a -browser method...

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:1749: -
(Profile*)profile {
    return profile_;
should do the trick.

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm (right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm:49: Profile*
profile = [controller_ profile];
Continuing the discussion of removal of themeService:

You could just write this as
    Profile* profile = [controller_ browser]->profile();

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button.h (right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_button.h:133: // Return the profile
associated with this browser window.
The implementer of the protocol has a profile available already.

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/gradient_button_cell.mm (right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/gradient_button_cell.mm:607: ui::ThemeProvider*
themeProvider = [[controlView window] themeProvider];
THANK YOU!

[groby-ooo-7-16, 2015-12-11 19:30:06]

Talked offline - can't remove profile just yet. (Some places rely on the
protocol only, not the implementing object. Missed that when scanning
codesearch)

LGTM

[Evan Stade, 2015-12-11 19:36:50]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-12-11 19:36:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, thakis@chromium.org, groby@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1492423003/#ps260001
(title: "fix that unittest")

[Evan Stade, 2015-12-11 19:36:58]

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2459: -
(Profile*)profile {
On 2015/12/11 02:32:00, groby wrote:
> Do we need this method at all?
> 
> The protocol is only implemented by BookmarkBarController, which already
exposes
> a -browser method...

but where it's used, it's cast as a BookmarkBarToolbarViewController, not a
BrowserWindowController

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm (right):

https://codereview.chromium.org/1492423003/diff/240001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_toolbar_view.mm:49: Profile*
profile = [controller_ profile];
On 2015/12/11 02:32:00, groby wrote:
> Continuing the discussion of removal of themeService:
> 
> You could just write this as
>     Profile* profile = [controller_ browser]->profile();

doesn't seem like that should work based on the type of controller_ (?)

[commit-bot: I haz the power, 2015-12-11 19:37:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492423003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492423003/260001

[commit-bot: I haz the power, 2015-12-11 21:50:56]

Description was changed from

==========
Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377, 568388
==========

to

==========
Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377, 568388
==========

[commit-bot: I haz the power, 2015-12-11 21:50:57]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-12-11 21:52:23]

Description was changed from

==========
Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377, 568388
==========

to

==========
Rejigger ThemeService: move exposure of ThemeProvider interface to a
separate class.

ThemeService provides a static way to get a ThemeProvider from a profile.
This allows different ThemeProviders for different profiles (i.e. otr +
original), while the majority of ThemeService remains per original profile.

As a bonus, UsingSystemTheme is moved from ThemeProvider to ThemeService, where
it seems more natural. It's still part of ThemeProvider on OSX, but only because
updating OSX will require more effort (and more care).

One new incognito color is implemented (for MD mode): COLOR_TOOLBAR. This is
ironically not yet visible on the toolbar, but you can see it on the DL shelf.

BUG=501377, 568388

Committed: https://crrev.com/68691b28a76845494f3b51d02e24844a69833032
Cr-Commit-Position: refs/heads/master@{#364812}
==========

[commit-bot: I haz the power, 2015-12-11 21:52:24]

Patchset 14 (id:??) landed as
https://crrev.com/68691b28a76845494f3b51d02e24844a69833032
Cr-Commit-Position: refs/heads/master@{#364812}