Change ThemeService to handle uninstalled themes.

Change ThemeService to handle uninstalled themes.

ThemeService now reverts to the default theme if the current theme is
unloaded.

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

[Andrew T Wilson (Slow), 2012-11-28 17:51:23]

PTAL

[Andrew T Wilson (Slow), 2012-11-29 20:33:07]

PTAL

[akalin, 2012-11-29 20:40:12]

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1175: ThemeService* theme_service
= ThemeServiceFactory::GetForProfile(profile_);
can you use ThemeService::GetThemeIDForProfile()? (I'm supposed to change all
callers to use that anyway)  Something like:

if (... extension->is_theme() && extension->id() ==
ThemeService::GetThemeIDForProfile(profile_)) {
  ThemeServiceFactory::GetForProfile(profile_)->UseDefaultTheme();
}

I was gonna suggest adding something like TSF::UseDefaultThemeForProfile, but I
guess at this point you know that the ThemeService is already loaded.

[pkotwicz, 2012-11-29 22:06:01]

themes/ LGTM with nits. Please address akalin@'s nit as well.

[pkotwicz, 2012-11-29 22:06:09]

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

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:15: namespace {
Nit: Add new line

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:95: // Let ThemeService finish
creating the ThemePack.
nit: change "ThemePack" to "theme pack"

[Andrew T Wilson (Slow), 2012-11-30 12:44:55]

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1175: ThemeService* theme_service
= ThemeServiceFactory::GetForProfile(profile_);
On 2012/11/29 20:40:12, akalin wrote:
> can you use ThemeService::GetThemeIDForProfile()? (I'm supposed to change all
> callers to use that anyway)  Something like:
> 
> if (... extension->is_theme() && extension->id() ==
> ThemeService::GetThemeIDForProfile(profile_)) {
>   ThemeServiceFactory::GetForProfile(profile_)->UseDefaultTheme();
> }
> 
> I was gonna suggest adding something like TSF::UseDefaultThemeForProfile, but
I
> guess at this point you know that the ThemeService is already loaded.

Done. If you are concerned about startup regressions, I don't think it's an
issue since at worst case this would force ThemeService to be loaded in the case
that the theme was uninstalled somehow while the browser was shutdown.

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

https://codereview.chromium.org/11434002/diff/13001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:15: namespace {
On 2012/11/29 22:06:09, pkotwicz wrote:
> Nit: Add new line

Done.

[akalin, 2012-11-30 18:25:24]

lgtm after nits

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

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:88: // Installs then uninstalls
a theme and makes sure that the ThemeService
comment above TEST_F line?

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:101: EXPECT_EQ(extension->id(),
theme_service->GetThemeID());
GetThemeIDForProfile

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:111: // default theme.
here too

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:123: EXPECT_EQ(extension->id(),
theme_service->GetThemeID());
GetThemeIDForProfile

[commit-bot: I haz the power, 2012-12-03 12:13:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/11434002/10008

[commit-bot: I haz the power, 2012-12-03 12:13:59]

Presubmit check for 11434002-10008 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/extensions

Presubmit checks took 4.0s to calculate.

[Andrew T Wilson (Slow), 2012-12-03 12:34:58]

+asargent for OWNERS rubberstamp

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

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:88: // Installs then uninstalls
a theme and makes sure that the ThemeService
On 2012/11/30 18:25:25, akalin wrote:
> comment above TEST_F line?

Done.

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:101: EXPECT_EQ(extension->id(),
theme_service->GetThemeID());
On 2012/11/30 18:25:25, akalin wrote:
> GetThemeIDForProfile

Done.

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:111: // default theme.
On 2012/11/30 18:25:25, akalin wrote:
> here too

Done.

https://codereview.chromium.org/11434002/diff/10008/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:123: EXPECT_EQ(extension->id(),
theme_service->GetThemeID());
On 2012/11/30 18:25:25, akalin wrote:
> GetThemeIDForProfile

Done.

[asargent_no_longer_on_chrome, 2012-12-03 18:44:06]

LGTM

[commit-bot: I haz the power, 2012-12-04 08:00:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/11434002/3004

[akalin, 2012-12-04 08:03:50]

https://chromiumcodereview.appspot.com/11434002/diff/3004/chrome/browser/exte...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/11434002/diff/3004/chrome/browser/exte...
chrome/browser/extensions/extension_service.cc:1175: extension->id() ==
ThemeService::GetThemeIDForProfile(profile_)) {
i actually had to revert my patch that changed this.  so now it's back to
theme_service->GetThemeID().  Sorry!

[commit-bot: I haz the power, 2012-12-04 08:27:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 09:41:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/11434002/6009

[commit-bot: I haz the power, 2012-12-04 13:00:19]

Change committed as 170946