Fix regression caused by 11968032 in upgrading themes

Fix regression caused by 11968032 in upgrading themes

BUG=177630
Test=Manual
1) Install a theme
2) Close Chrome
3) Change the value of kThemePackVersion to a different number in browser_theme_pack.cc
4) Compile Chrome
5) Reopen Chrome. Ensure that the theme is still present 

R=mpcomplete, erg
TBR=sky (For new includes in chrome_browser_extensions.gypi)

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

[pkotwicz, 2013-02-23 01:04:31]

Matt, can you please take a look?

[Matt Perry, 2013-02-23 01:06:20]

LGTM

+yoz: FYI

[pkotwicz, 2013-02-23 01:06:45]

I think that this is generally a step in the right direction. Some of the code
in chrome/browser/themes would be better suited in
chrome/browser/extensions/api/themes/*
I will add a browser test in a followup CL.
However, I am not volunteering for moving said code quite yet. There are bigger
fish to tackle such as making extensions not depend on ThemeService.

[pkotwicz, 2013-02-23 01:09:12]

Elliot, can you please take a look at the changes in chrome/browser/profiles

[Elliot Glaysher, 2013-02-23 01:15:13]

https://codereview.chromium.org/12326089/diff/1/chrome/browser/themes/theme_s...
File chrome/browser/themes/theme_service_factory.cc (left):

https://codereview.chromium.org/12326089/diff/1/chrome/browser/themes/theme_s...
chrome/browser/themes/theme_service_factory.cc:88: }
So, why are you removing this? The theme service will be brought up almost
instantaneously on startup because its necessary for drawing the UI.

[Matt Perry, 2013-02-23 01:21:35]

https://codereview.chromium.org/12326089/diff/1/chrome/browser/themes/theme_s...
File chrome/browser/themes/theme_service_factory.cc (left):

https://codereview.chromium.org/12326089/diff/1/chrome/browser/themes/theme_s...
chrome/browser/themes/theme_service_factory.cc:88: }
On 2013/02/23 01:15:14, Elliot Glaysher wrote:
> So, why are you removing this? The theme service will be brought up almost
> instantaneously on startup because its necessary for drawing the UI.

The ThemeService depends on ExtensionService. Unfortunately, ExtensionService is
not a ProfileKeyedService, so you can't actually express this dependency. So
with this part here, ThemeService is created too early.

[pkotwicz, 2013-02-23 01:23:02]

The reason is that the ThemeService was being created in between the
ExtensionSystem being created and the ExtensionService being created. ie before
ExtensionSystem::InitForRegularProfile().
So, the call to
"extensions::ExtensionSystem::Get(profile_)->extension_service();" in
ThemeService::LoatThemePrefs() was failing

[Elliot Glaysher, 2013-02-23 01:25:28]

On 2013/02/23 01:23:02, pkotwicz wrote:
> The reason is that the ThemeService was being created in between the
> ExtensionSystem being created and the ExtensionService being created. ie
before
> ExtensionSystem::InitForRegularProfile().
> So, the call to
> "extensions::ExtensionSystem::Get(profile_)->extension_service();" in
> ThemeService::LoatThemePrefs() was failing

Could you solve that (now or later) by changing the dependency structure
instead?

[Matt Perry, 2013-02-23 01:31:28]

On 2013/02/23 01:25:28, Elliot Glaysher wrote:
> On 2013/02/23 01:23:02, pkotwicz wrote:
> > The reason is that the ThemeService was being created in between the
> > ExtensionSystem being created and the ExtensionService being created. ie
> before
> > ExtensionSystem::InitForRegularProfile().
> > So, the call to
> > "extensions::ExtensionSystem::Get(profile_)->extension_service();" in
> > ThemeService::LoatThemePrefs() was failing
> 
> Could you solve that (now or later) by changing the dependency structure
> instead?

Maybe.

ExtensionService is created as a result of
ProfileManager::DoFinalInitForServices - see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

I recommended to Peter that he take this approach because it didn't seem trivial
to unravel the dependencies here. It wasn't clear to me what would happen if we
moved the ExtensionService initialization earlier in Profile creation.

[Elliot Glaysher, 2013-02-23 01:50:59]

On 2013/02/23 01:31:28, Matt Perry wrote:
> On 2013/02/23 01:25:28, Elliot Glaysher wrote:
> > On 2013/02/23 01:23:02, pkotwicz wrote:
> > > The reason is that the ThemeService was being created in between the
> > > ExtensionSystem being created and the ExtensionService being created. ie
> > before
> > > ExtensionSystem::InitForRegularProfile().
> > > So, the call to
> > > "extensions::ExtensionSystem::Get(profile_)->extension_service();" in
> > > ThemeService::LoatThemePrefs() was failing
> > 
> > Could you solve that (now or later) by changing the dependency structure
> > instead?
> 
> Maybe.
> 
> ExtensionService is created as a result of
> ProfileManager::DoFinalInitForServices - see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> 
> I recommended to Peter that he take this approach because it didn't seem
trivial
> to unravel the dependencies here. It wasn't clear to me what would happen if
we
> moved the ExtensionService initialization earlier in Profile creation.

So, pulling on this some more, I don't understand why
ProfileManager::DoFinalInitForServices() is responsible for doing some  sort of
initialization after the normal PKSF loop. Making sure initialization code lived
in the component, instead of the ProfileImpl or ProfileManager was a big part of
why we moved to this system.

It looks like--for now--this works. But it seems coincidental, and requires a
whole lot of non-local stuff that isn't immediately obvious to keep working the
way its working today.

[Matt Perry, 2013-02-23 01:53:24]

On 2013/02/23 01:50:59, Elliot Glaysher wrote:
> On 2013/02/23 01:31:28, Matt Perry wrote:
> > On 2013/02/23 01:25:28, Elliot Glaysher wrote:
> > > On 2013/02/23 01:23:02, pkotwicz wrote:
> > > > The reason is that the ThemeService was being created in between the
> > > > ExtensionSystem being created and the ExtensionService being created. ie
> > > before
> > > > ExtensionSystem::InitForRegularProfile().
> > > > So, the call to
> > > > "extensions::ExtensionSystem::Get(profile_)->extension_service();" in
> > > > ThemeService::LoatThemePrefs() was failing
> > > 
> > > Could you solve that (now or later) by changing the dependency structure
> > > instead?
> > 
> > Maybe.
> > 
> > ExtensionService is created as a result of
> > ProfileManager::DoFinalInitForServices - see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> > 
> > I recommended to Peter that he take this approach because it didn't seem
> trivial
> > to unravel the dependencies here. It wasn't clear to me what would happen if
> we
> > moved the ExtensionService initialization earlier in Profile creation.
> 
> So, pulling on this some more, I don't understand why
> ProfileManager::DoFinalInitForServices() is responsible for doing some  sort
of
> initialization after the normal PKSF loop. Making sure initialization code
lived
> in the component, instead of the ProfileImpl or ProfileManager was a big part
of
> why we moved to this system.
> 
> It looks like--for now--this works. But it seems coincidental, and requires a
> whole lot of non-local stuff that isn't immediately obvious to keep working
the
> way its working today.

Agreed - I don't understand it either. Which is why I suggested that Peter just
address the immediate regression for now.

[Elliot Glaysher, 2013-02-23 02:03:47]

So, I'm willing to give an lgtm now, but in return, there needs to be an owner
to clean this stuff up. Looking at, for example,
ProfileManager::DoFinalInitForServices(), people are now hacking around the PKSF
because ExtensionSystem both claims to init itself and doesn't. Whatever
ManagedUserService is, is going around normal profile startup because of
ExtensionSystem.

Yoz? Matt?

[commit-bot: I haz the power, 2013-02-24 00:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/12326089/1

[commit-bot: I haz the power, 2013-02-24 00:07:50]

Presubmit check for 12326089-1 failed and returned exit status 1.

INFO:root:Found 7 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/browser/extensions/api/themes/theme_api.cc \
  chrome/browser/extensions/api/themes/theme_api.h

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

Presubmit checks took 3.4s to calculate.

[commit-bot: I haz the power, 2013-02-24 00:18:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/12326089/4002

[commit-bot: I haz the power, 2013-02-24 02:02:27]

Change committed as 184340

[pkotwicz, 2013-02-24 23:08:05]

Matt can you please file a bug related to the issue that Elliot mentioned and
assign appropriately? (I think you can describe the problem better than I can)