Add custom default theme support and create a managed user default theme.

Add custom default theme support and create a managed user default theme.

Add a new interface called CustomThemeProvider. The ThemeService
will have a CustomThemeProvider to access the theme assets.
One subclass of the CustomThemeProvider is the BrowserThemePack,
which is used to install themes represented as extensions.
Other subclasses can be custom default themes, like the Linux
X11 native theme, or the Managed User Theme.

BUG=241377, 251225
TEST=unit_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213866

[Adrian Kuegel, 2013-07-18 16:50:42]

Peter, can you please review this CL?
Compared to your suggestion I did the following differently:

- I didn't make the interface an abstract class; this makes it easier for
subclasses if they don't want to change the default theme values.
- I didn't use a RegisterCustomTheme and UnregisterCustomTheme method, because I
didn't know what should be done there.

Also, I didn't rewrite the ThemeServiceGtk to use the custom default theme
mechanism, because as far as I know that theme service will become obsolete,
anyway. But I can also do that rewrite if you want.

[pkotwicz, 2013-07-19 05:54:13]

Sorry I could not get through the entire CL today. I will continue my review
tomorrow morning

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_provider.h (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_provider.h:12: #include
"ui/gfx/image/image.h"
gfx::Image could be forward declared as well?

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_provider.h:22: class CustomThemeProvider :
public base::RefCountedThreadSafe<
I do not like the name CustomThemeProvider because it leads me to believe that
CustomThemeProvider implements ui::ThemeProvider which it does not.
How about
CustomThemeProperyProvider /  CustomTheme / CustomThemeSupplier

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_provider.h:37: // Called when the theme is
started being used.
Nit: starts being used

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:241: SaveThemeID(kDefaultThemeID);
The call to SaveThemeID(kDefaultThemeID) is done for you by ClearAllThemeData().
Same goes for StopUsingTheme().

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:318: RemoveUnusedThemes();
You want to check |ready_| before calling RemoveUnusedThemes(). That way we
don't end up doing garbage collection on startup.
(See ExtensionService::GarbageCollectExtensions)

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:326: set_ready();
I would rather call UseDefaultTheme() / SetNativeTheme() as appropriate here
(right before set_ready()).

I think you can do it if you introduce bool
ThemeService::ShouldInitWithNativeTheme()

This change should also allow you to make SetCustomDefaultTheme() protected.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:431: if (theme_provider_.get())
Optional: It may be worth putting this code in its own function.
ThemeService::SwapTheme() may be a valid name if you end up renaming
CustomThemeProvider to CustomTheme.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (left):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:25: use_system_theme_ =
profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
kUsesSystemTheme confuses me. I will dig deeper tomorrow morning. It confuses me
because I am unclear as to whether kUsesSystemTheme is syncable.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:74: bool
ThemeServiceAuraX11::UsingDefaultTheme() const {
You still need to override UsingDefaultTheme().

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:31: // These are just reference
pointers.
Nit: comment that the pointers are not owned instead.

[pkotwicz, 2013-07-19 05:54:44]

On 2013/07/18 16:50:42, Adrian Kuegel wrote:
> Peter, can you please review this CL?
> Compared to your suggestion I did the following differently:
> 
> - I didn't make the interface an abstract class; this makes it easier for
> subclasses if they don't want to change the default theme values.
> - I didn't use a RegisterCustomTheme and UnregisterCustomTheme method, because
I
> didn't know what should be done there.
> 
> Also, I didn't rewrite the ThemeServiceGtk to use the custom default theme
> mechanism, because as far as I know that theme service will become obsolete,
> anyway. But I can also do that rewrite if you want.

That's prefectly fine on all counts

[Adrian Kuegel, 2013-07-19 13:54:39]

Thanks for the review. I tried to fix the things you mentioned.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_provider.h (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_provider.h:12: #include
"ui/gfx/image/image.h"
On 2013/07/19 05:54:13, pkotwicz wrote:
> gfx::Image could be forward declared as well?

Done. I also changed this in browser_theme_pack.h

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_provider.h:22: class CustomThemeProvider :
public base::RefCountedThreadSafe<
On 2013/07/19 05:54:13, pkotwicz wrote:
> I do not like the name CustomThemeProvider because it leads me to believe that
> CustomThemeProvider implements ui::ThemeProvider which it does not.
> How about
> CustomThemeProperyProvider /  CustomTheme / CustomThemeSupplier

I have chosen the name CustomThemeSupplier now. CustomThemePropertyProvider
would probably better cover it, but it seems awfully long.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_provider.h:37: // Called when the theme is
started being used.
On 2013/07/19 05:54:13, pkotwicz wrote:
> Nit: starts being used

Done.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:241: SaveThemeID(kDefaultThemeID);
On 2013/07/19 05:54:13, pkotwicz wrote:
> The call to SaveThemeID(kDefaultThemeID) is done for you by
ClearAllThemeData().
> Same goes for StopUsingTheme().

Right. I removed those lines.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:318: RemoveUnusedThemes();
On 2013/07/19 05:54:13, pkotwicz wrote:
> You want to check |ready_| before calling RemoveUnusedThemes(). That way we
> don't end up doing garbage collection on startup.
> (See ExtensionService::GarbageCollectExtensions)

But I guess I should check that then in the function RemoveUnusedThemes? Or does
that break something?

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:326: set_ready();
On 2013/07/19 05:54:13, pkotwicz wrote:
> I would rather call UseDefaultTheme() / SetNativeTheme() as appropriate here
> (right before set_ready()).
> 
> I think you can do it if you introduce bool
> ThemeService::ShouldInitWithNativeTheme()
> 
> This change should also allow you to make SetCustomDefaultTheme() protected.

I do it now almost like that. Since UseDefaultTheme also records UMA statistics
for resetting the theme, I think I should not call it from here. So I introduced
a SetManagedUserTheme function which I call if it is a managed user.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:431: if (theme_provider_.get())
On 2013/07/19 05:54:13, pkotwicz wrote:
> Optional: It may be worth putting this code in its own function.
> ThemeService::SwapTheme() may be a valid name if you end up renaming
> CustomThemeProvider to CustomTheme.

Done.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (left):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:25: use_system_theme_ =
profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
On 2013/07/19 05:54:13, pkotwicz wrote:
> kUsesSystemTheme confuses me. I will dig deeper tomorrow morning. It confuses
me
> because I am unclear as to whether kUsesSystemTheme is syncable.

I hadn't looked into it, I just tried to replicate the current state (and from
what I can tell, it should behave exactly as before).
Anyway, I checked now, and yes, it is synced if the native theme is used or not:
ThemeSyncableService calls UsingNativeTheme to figure out if to set this flag in
the sync data or not. But the preference itself is used only to save the state
for a browser restart.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:74: bool
ThemeServiceAuraX11::UsingDefaultTheme() const {
On 2013/07/19 05:54:13, pkotwicz wrote:
> You still need to override UsingDefaultTheme().

Done.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:31: // These are just reference
pointers.
On 2013/07/19 05:54:13, pkotwicz wrote:
> Nit: comment that the pointers are not owned instead.

Done.

[pkotwicz, 2013-07-19 18:33:20]

Looks really good. Mostly nits

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:318: RemoveUnusedThemes();
Putting the check there's fine.

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:326: set_ready();
Cool!

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (left):

https://codereview.chromium.org/19471005/diff/16001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:25: use_system_theme_ =
profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
I understand now.
For some context, I was confused because of
TwoClientPreferencesSyncTest.kUsesSystemTheme. Upon closer inspection, the test
tests that the pref is not synced. (Though as you mentioned, whether the native
theme is used is synced via ThemeSyncableService)

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:15: }
Nit: }  // namespace

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:32: if (id ==
ThemeProperties::COLOR_MANAGED_USER_LABEL) {
I would have expected for COLOR_MANAGED_USER_LABEL and
COLOR_MANAGED_USER_LABEL_BACKGROUND to be handled either here or in ThemeService
but not both.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:50: return
rb_.GetNativeImageNamed(id);
Return an empty gfx::Image if HasCustomImage() == false.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.h:27: ui::ResourceBundle& rb_;
Request the ResourceBundle where needed

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
chrome/browser/themes/browser_theme_pack.cc:895: void
BrowserThemePack::GetThemeableImageIDRs(std::set<int>* result) {
Reorder this method to match reordering in the header file.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
File chrome/browser/themes/browser_theme_pack.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
chrome/browser/themes/browser_theme_pack.h:20: #include "ui/gfx/image/image.h"
Nit: You should not need to include image.h now that you are forward declaring
it.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_supplier.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:46: // If the theme specifies data
for the corresponding |id|, return true and
Nit: returns true

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:61: // Returns the raw PNG encoded
data for IDR_THEME_NTP_*. This method is only
Nit: This method only works

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:241: SwapThemeSupplier(theme_supplier);
Nit: Comment that NotifyThemeChange() is not necessary if |ready_| == false.
(For the sake of someone reading the code and wondering why LoadThemePrefs()
does not always result in a call to NotifyThemeChanged()).

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:250: if (!profile_ || !ready_)
Comment that we do not want to garbage collect unused themes immediately on
startup and that themes will get garbage collected once 
ExtensionService::GarbageCollectExtensions() runs.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:308: theme_supplier_->StopUsingTheme();
Optional nit: Use SwapThemeSupplier(NULL) here?

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:150: // If the native theme should be used
as default.
How about: "Returns true if the ThemeService should use the native theme on
startup."
The native theme is technically not the default because UsingDefaultTheme()
returns false if we are using the GTK+ native theme.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:178: const CustomThemeSupplier*
GetThemeSupplier() const {
I think that get_theme_supplier() is the preferred style.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:186: // or StartUsingTheme as appropriate.
Nit: StartUsingTheme(), StopUsingTheme().

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:206: void SetManagedUserTheme();
Add a comment here.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:24: virtual gfx::Image
GetImageNamed(int id) OVERRIDE;
GetImageNamed() should be below GetColor() to match order in
custom_theme_supplier.h

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:43:
NativeThemeX11::~NativeThemeX11() {}
Nit: Put the destructor last to match order defined above.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:76:
ThemeService::SetCustomDefaultTheme(
Nit: You do not need to prepend SetCustomDefaultTheme() with ThemeService.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:296: void
GtkThemeService::Init(Profile* profile) {
The current code will sometimes use the GTK+ theme instead of the managed user
theme for managed users.
|use_gtk_| does not take into account IsManagedUser().
Note: GtkThemeService::DefaultUsesSystemTheme() sometimes returns true.

You could fix this by overriding ShouldInitWithNativeTheme() and modifying
LoadThemePrefs().

[Adrian Kuegel, 2013-07-22 12:58:08]

Hi Peter,

I have now also added a small unit test which tests that the managed user theme
is used as default. I needed to create a namespace for the test class though in
order to make it possible to declare it as friend class.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:15: }
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: }  // namespace

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:32: if (id ==
ThemeProperties::COLOR_MANAGED_USER_LABEL) {
On 2013/07/19 18:33:20, pkotwicz wrote:
> I would have expected for COLOR_MANAGED_USER_LABEL and
> COLOR_MANAGED_USER_LABEL_BACKGROUND to be handled either here or in
ThemeService
> but not both.

The reason I do it here and in the ThemeService is: I want the managed user
theme have these specific values, but if some other theme is installed (what we
now want to support in M30), we need to derive the colors from other properties.
So the code in ThemeService is there to support other themes, and the code here
is for the special case of our theme. Unfortunately I don't see a way how to
calculate the colors we want for our theme from the other color properties which
would work in general.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:50: return
rb_.GetNativeImageNamed(id);
On 2013/07/19 18:33:20, pkotwicz wrote:
> Return an empty gfx::Image if HasCustomImage() == false.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.h:27: ui::ResourceBundle& rb_;
On 2013/07/19 18:33:20, pkotwicz wrote:
> Request the ResourceBundle where needed

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
chrome/browser/themes/browser_theme_pack.cc:895: void
BrowserThemePack::GetThemeableImageIDRs(std::set<int>* result) {
On 2013/07/19 18:33:20, pkotwicz wrote:
> Reorder this method to match reordering in the header file.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
File chrome/browser/themes/browser_theme_pack.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/bro...
chrome/browser/themes/browser_theme_pack.h:20: #include "ui/gfx/image/image.h"
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: You should not need to include image.h now that you are forward declaring
> it.

Right. I forgot to remove the include here. Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_supplier.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:46: // If the theme specifies data
for the corresponding |id|, return true and
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: returns true

I also changed "write" to "writes", and replaced "functions" with "methods" to
keep it consistent with the next phrase.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:61: // Returns the raw PNG encoded
data for IDR_THEME_NTP_*. This method is only
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: This method only works

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:241: SwapThemeSupplier(theme_supplier);
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: Comment that NotifyThemeChange() is not necessary if |ready_| == false.
> (For the sake of someone reading the code and wondering why LoadThemePrefs()
> does not always result in a call to NotifyThemeChanged()).

I guess it makes sense then if I also check this before calling
NotifyThemeChanged?

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:250: if (!profile_ || !ready_)
On 2013/07/19 18:33:20, pkotwicz wrote:
> Comment that we do not want to garbage collect unused themes immediately on
> startup and that themes will get garbage collected once 
> ExtensionService::GarbageCollectExtensions() runs.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:308: theme_supplier_->StopUsingTheme();
On 2013/07/19 18:33:20, pkotwicz wrote:
> Optional nit: Use SwapThemeSupplier(NULL) here?

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:150: // If the native theme should be used
as default.
On 2013/07/19 18:33:20, pkotwicz wrote:
> How about: "Returns true if the ThemeService should use the native theme on
> startup."
> The native theme is technically not the default because UsingDefaultTheme()
> returns false if we are using the GTK+ native theme.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:178: const CustomThemeSupplier*
GetThemeSupplier() const {
On 2013/07/19 18:33:20, pkotwicz wrote:
> I think that get_theme_supplier() is the preferred style.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:186: // or StartUsingTheme as appropriate.
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: StartUsingTheme(), StopUsingTheme().

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:206: void SetManagedUserTheme();
On 2013/07/19 18:33:20, pkotwicz wrote:
> Add a comment here.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:24: virtual gfx::Image
GetImageNamed(int id) OVERRIDE;
On 2013/07/19 18:33:20, pkotwicz wrote:
> GetImageNamed() should be below GetColor() to match order in
> custom_theme_supplier.h

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:43:
NativeThemeX11::~NativeThemeX11() {}
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: Put the destructor last to match order defined above.

Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.cc:76:
ThemeService::SetCustomDefaultTheme(
On 2013/07/19 18:33:20, pkotwicz wrote:
> Nit: You do not need to prepend SetCustomDefaultTheme() with ThemeService.

Right. Done.

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/24001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:296: void
GtkThemeService::Init(Profile* profile) {
On 2013/07/19 18:33:20, pkotwicz wrote:
> The current code will sometimes use the GTK+ theme instead of the managed user
> theme for managed users.
> |use_gtk_| does not take into account IsManagedUser().
> Note: GtkThemeService::DefaultUsesSystemTheme() sometimes returns true.
> 
> You could fix this by overriding ShouldInitWithNativeTheme() and modifying
> LoadThemePrefs().

Good catch. Done.

[pkotwicz, 2013-07-22 20:17:28]

LGTM with nits

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.h (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.h:18: // Overridden from
CustomThemeProvider:
Nit: CustomThemeSupplier

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_supplier.cc (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.cc:40: base::RefCountedMemory*
CustomThemeSupplier::GetRawData(
Nit: Move GetRawData() just below GetImageNamed() to match order in
BrowserThemePack.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:246: if (ready_)
Actually, you can skip checking |ready_| here. And do not need a comment.
No one should be listening for NOTIFICATION_BROWSER_THEME_CHANGED before the
ThemeService object has finished being constructed.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:257: //
ExtensionService::GarbageCollectExtensions() runs
Nit '.' at the end of the sentence

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:292: std::string id = GetThemeID();
Optional: You may not want to check |kDefaultThemeGalleryID| for managed users.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:320: theme_supplier_ = NULL;
The line above should be done for us by SwapThemeSupplier().

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:333: // Managed users should always have
their own default theme.
How about: "Managed users have a different default theme".

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:337: SetNativeTheme();
I would rather call UseDefaultTheme() in the else case. And check is_ready()
before recording "Themes_Reset".
This allows you to avoid calling LoadDefaultValues() in
GtkThemeService::LoadThemePrefs().
This will also fix GtkThemeService::UsingNativeTheme() from sometimes returning
true when you are using the managed user theme.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:43: namespace theme_service_test_internal
{
namespace theme_internal should be sufficient.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:103:
CustomThemeSupplier::MANAGED_USER_THEME);
Nice test.

Can you also add a test for the behavior on systems which use the system/native
theme by default?

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.h (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.h:78: virtual bool
ShouldInitWithNativeTheme() OVERRIDE;
The new method should be right below UsingNativeTheme() to match the order in
the ThemeService header file

[Adrian Kuegel, 2013-07-23 08:13:29]

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.h (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.h:18: // Overridden from
CustomThemeProvider:
On 2013/07/22 20:17:28, pkotwicz wrote:
> Nit: CustomThemeSupplier

Thanks for finding this, I forgot to change that. Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_supplier.cc (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.cc:40: base::RefCountedMemory*
CustomThemeSupplier::GetRawData(
On 2013/07/22 20:17:28, pkotwicz wrote:
> Nit: Move GetRawData() just below GetImageNamed() to match order in
> BrowserThemePack.

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:246: if (ready_)
On 2013/07/22 20:17:28, pkotwicz wrote:
> Actually, you can skip checking |ready_| here. And do not need a comment.
> No one should be listening for NOTIFICATION_BROWSER_THEME_CHANGED before the
> ThemeService object has finished being constructed.

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:257: //
ExtensionService::GarbageCollectExtensions() runs
On 2013/07/22 20:17:28, pkotwicz wrote:
> Nit '.' at the end of the sentence

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:292: std::string id = GetThemeID();
On 2013/07/22 20:17:28, pkotwicz wrote:
> Optional: You may not want to check |kDefaultThemeGalleryID| for managed
users.

Right, good point. Changed.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:320: theme_supplier_ = NULL;
On 2013/07/22 20:17:28, pkotwicz wrote:
> The line above should be done for us by SwapThemeSupplier().

Right. I have removed this line now.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:333: // Managed users should always have
their own default theme.
On 2013/07/22 20:17:28, pkotwicz wrote:
> How about: "Managed users have a different default theme".

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:337: SetNativeTheme();
On 2013/07/22 20:17:28, pkotwicz wrote:
> I would rather call UseDefaultTheme() in the else case. And check is_ready()
> before recording "Themes_Reset".
> This allows you to avoid calling LoadDefaultValues() in
> GtkThemeService::LoadThemePrefs().
> This will also fix GtkThemeService::UsingNativeTheme() from sometimes
returning
> true when you are using the managed user theme.

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.h (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.h:43: namespace theme_service_test_internal
{
On 2013/07/22 20:17:28, pkotwicz wrote:
> namespace theme_internal should be sufficient.

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:103:
CustomThemeSupplier::MANAGED_USER_THEME);
On 2013/07/22 20:17:28, pkotwicz wrote:
> Nice test.
> 
> Can you also add a test for the behavior on systems which use the
system/native
> theme by default?

Done.

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.h (right):

https://codereview.chromium.org/19471005/diff/43001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.h:78: virtual bool
ShouldInitWithNativeTheme() OVERRIDE;
On 2013/07/22 20:17:28, pkotwicz wrote:
> The new method should be right below UsingNativeTheme() to match the order in
> the ThemeService header file

Done.

[Adrian Kuegel, 2013-07-23 08:15:29]

Pam, can I please get your approval for managed_user_theme.* ?
Elliot, can I please get your approval for gtk_theme_service.* ?

[Pam (message me for reviews), 2013-07-23 10:14:17]

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.cc (right):

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:24: if (id ==
ThemeProperties::COLOR_FRAME) {
How about a switch statement?

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:57: return (id ==
IDR_THEME_FRAME || id == IDR_THEME_FRAME_INACTIVE ||
Please split this at each || for clarity.

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/themes/bro...
File chrome/browser/themes/browser_theme_pack.h (right):

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/themes/bro...
chrome/browser/themes/browser_theme_pack.h:70: // Returns the set of image idrs
which can be overwritten by a user provided
nit: IDRs

[Adrian Kuegel, 2013-07-23 10:26:36]

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.cc (right):

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:24: if (id ==
ThemeProperties::COLOR_FRAME) {
On 2013/07/23 10:14:17, Pam (also send IM for reviews) wrote:
> How about a switch statement?

Done.

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.cc:57: return (id ==
IDR_THEME_FRAME || id == IDR_THEME_FRAME_INACTIVE ||
On 2013/07/23 10:14:17, Pam (also send IM for reviews) wrote:
> Please split this at each || for clarity.

Done.

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/themes/bro...
File chrome/browser/themes/browser_theme_pack.h (right):

https://codereview.chromium.org/19471005/diff/60001/chrome/browser/themes/bro...
chrome/browser/themes/browser_theme_pack.h:70: // Returns the set of image idrs
which can be overwritten by a user provided
On 2013/07/23 10:14:17, Pam (also send IM for reviews) wrote:
> nit: IDRs

Done.

[Pam (message me for reviews), 2013-07-23 11:59:57]

managed_mode LGTM

[Elliot Glaysher, 2013-07-23 19:57:31]

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.h (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.h:14: class ManagedUserTheme :
public CustomThemeSupplier {
Add class comment. A description of what the ManagedUserTheme looks like
(compared to default Chrome Blue Theme) would be nice.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_supplier.h (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:25: class CustomThemeSupplier :
public base::RefCountedThreadSafe<
Again, class documentation. Imagine I'm coming into this not knowing anything
about themes and I see this piece of abstraction.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:36: ThemeType GetThemeType() const
{
inlined methods should be unix_hacker_cased().

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.h (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.h:11: // theme data.
This comment should be updated. It now sets/manages the CustomThemeSupplier.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:45: const CustomThemeSupplier*
GetThemeSupplier(ThemeService* theme_service) {
unix_hacker_case().

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (left):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:655: set_ready();
How does |ready_| get set? What are the implications of not setting ready? Just
reading through the code, it looks like it'll clear the theme data and screw up
UMA data for Themes.Gone. (via notification, then ThemeService::MigrateTheme()).

(The first time, but not subsequent times after patching this in locally, a
profile previously with the GTK theme switched to the default theme. Possible
race?)

[pkotwicz, 2013-07-23 20:59:52]

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:301: use_gtk_ =
profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
Leave |use_gtk_| as false.
ThemeService::SetManagedUserTheme() does not affect |use_gtk_|

[Adrian Kuegel, 2013-07-24 12:31:35]

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_theme.h (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_theme.h:14: class ManagedUserTheme :
public CustomThemeSupplier {
On 2013/07/23 19:57:31, Elliot Glaysher wrote:
> Add class comment. A description of what the ManagedUserTheme looks like
> (compared to default Chrome Blue Theme) would be nice.

Done.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/cus...
File chrome/browser/themes/custom_theme_supplier.h (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:25: class CustomThemeSupplier :
public base::RefCountedThreadSafe<
On 2013/07/23 19:57:31, Elliot Glaysher wrote:
> Again, class documentation. Imagine I'm coming into this not knowing anything
> about themes and I see this piece of abstraction.

Done.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/cus...
chrome/browser/themes/custom_theme_supplier.h:36: ThemeType GetThemeType() const
{
On 2013/07/23 19:57:31, Elliot Glaysher wrote:
> inlined methods should be unix_hacker_cased().

Done.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_aurax11.h (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_aurax11.h:11: // theme data.
On 2013/07/23 19:57:31, Elliot Glaysher wrote:
> This comment should be updated. It now sets/manages the CustomThemeSupplier.

Done.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:45: const CustomThemeSupplier*
GetThemeSupplier(ThemeService* theme_service) {
On 2013/07/23 19:57:31, Elliot Glaysher wrote:
> unix_hacker_case().

Done.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (left):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:655: set_ready();
On 2013/07/23 19:57:31, Elliot Glaysher wrote:
> How does |ready_| get set? What are the implications of not setting ready?
Just
> reading through the code, it looks like it'll clear the theme data and screw
up
> UMA data for Themes.Gone. (via notification, then
ThemeService::MigrateTheme()).

Thanks for looking so closely at this. I have missed that. I agree, looks like
this is what would happen. But we can check in NotifyThemeChanged if |ready_| is
set, and if not, return early. That should fix this problem.

> (The first time, but not subsequent times after patching this in locally, a
> profile previously with the GTK theme switched to the default theme. Possible
> race?)

I suppose this has to do with |use_gtk_| being set in OnUsesSystemThemeChanged()
instead of changing it alongside the pref. The way I could always reproduce this
race was if I remove line 301 which initializes |use_gtk_| to the pref value.
Therefore I changed it such that |use_gtk_| also gets set whenever we change the
system theme pref.

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

https://codereview.chromium.org/19471005/diff/79001/chrome/browser/ui/gtk/gtk...
chrome/browser/ui/gtk/gtk_theme_service.cc:301: use_gtk_ =
profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme);
On 2013/07/23 20:59:52, pkotwicz wrote:
> Leave |use_gtk_| as false.
> ThemeService::SetManagedUserTheme() does not affect |use_gtk_|

Done.

[Elliot Glaysher, 2013-07-25 19:04:25]

lgtm

[commit-bot: I haz the power, 2013-07-26 08:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/19471005/103001

[commit-bot: I haz the power, 2013-07-26 10:10:36]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-07-26 10:30:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/19471005/143001

[commit-bot: I haz the power, 2013-07-26 12:41:13]

Change committed as 213866