Make ThemeService use the ExtensionSystem's AsyncEvent

Make ThemeService use the ExtensionSystem's AsyncEvent
instead of the READY notification.

BUG=240968

[Jeffrey Yasskin, 2013-05-15 23:30:14]

This depends on https://codereview.chromium.org/14757022/, but I think that's
stable enough that it's worth you reviewing this piece. Let me know what you
think.

[pkotwicz, 2013-05-16 18:18:35]

https://codereview.chromium.org/14895016/diff/2001/chrome/browser/themes/them...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/14895016/diff/2001/chrome/browser/themes/them...
chrome/browser/themes/theme_service.cc:348: NotifyThemeChanged();
Delaying when we call NotifyThemeChanged() gives a lot more code the opportunity
to register to listen to NOTIFICATION_BROWSER_THEME_CHANGED. This subsequently
makes the call to NotifyThemeChanged() significantly more expensive on Windows.
We only go through this code path after the user has upgraded to a new version
of Chrome so I am not sure how much this matters.
Otherwise, the code looks good

[Jeffrey Yasskin, 2013-05-16 23:28:18]

Thanks for taking a look.

https://codereview.chromium.org/14895016/diff/2001/chrome/browser/themes/them...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/14895016/diff/2001/chrome/browser/themes/them...
chrome/browser/themes/theme_service.cc:348: NotifyThemeChanged();
On 2013/05/16 18:18:35, pkotwicz wrote:
> Delaying when we call NotifyThemeChanged() gives a lot more code the
opportunity
> to register to listen to NOTIFICATION_BROWSER_THEME_CHANGED. This subsequently
> makes the call to NotifyThemeChanged() significantly more expensive on
Windows.
> We only go through this code path after the user has upgraded to a new version
> of Chrome so I am not sure how much this matters.
> Otherwise, the code looks good

In the long run, I think we can fix this by making ThemeService::ready_ a
OneShotEvent too, and doing the same cleanup on the users of
NOTIFICATION_BROWSER_THEME_CHANGED. In the short run ...

We could in theory add a ThenRun(..., INLINE_OK) overload, which allows running
the callback without yielding back to the MessageLoop, which I think would
eliminate the extra listeners on NOTIFICATION_BROWSER_THEME_CHANGED in the case
that the ExtensionsSystem is ready when the ThemeService is constructed. (*Is*
the ExtensionSystem generally ready when the ThemeService is constructed?)
However, we're hoping to avoid that INLINE_OK flag because of past bad
experiences with calls that could either run inline or yield.

I don't immediately see a good way to call NotifyThemeChanged only if the task
is delayed, without going back to the duplicated-code situation.

Do you think the extra NotifyThemeChanged cost is enough that I should rethink
this change, or is it acceptable in order to eliminate the duplicated code?
(and, in the long run, all uses of EXTENSIONS_READY)

[pkotwicz, 2013-05-21 19:13:03]

The path that you are modifying runs on average once per stable release. (It is
controlled by the value of kThemePackVersion changing.)

ExtensionService is never ready when ThemeService is constructed. The theme
service is created right when the profile is created.
From doing some digging, delaying NotifyThemeChanged() results in
NotifyThemeChanged() taking ~100ms more on Windows. This seems to be mostly due
to a really expensive call to SetWindowRgn().

A couple options:
- Do some more digging and decide that the runtime change in
NotifyThemeChanged() is not important. (And double check that my measurements
are correct) It may not be important. I am not well acquainted with windows
native calls.
- Move the handling which currently occurs as a result of
NOTIFICATION_EXTENSIONS_READY to occur as a result of
NOTIFICATION_EXTENSION_LOADED instead.

Note: Somewhat related to this, ExtensionService currently depends on
ThemeService which it should not. For instance,
ExtensionService::GarbageCollectExtensions calls into the ThemeService. Fixing
this should be simple.