Remove deprecated extension notification from theme_service

Remove deprecated extension notification from theme_service.
Use ExtensionRegistryObserver instead.

R=pkotwicz@chromium.org
BUG=411568
TEST=unit_tests

Committed: https://crrev.com/96f0702b9e5ff97bf1870b7ac7e614c2358a78aa
Cr-Commit-Position: refs/heads/master@{#320587}

[limasdf, 2015-03-04 10:19:01]

Patchset #1 (id:1) has been deleted

[limasdf, 2015-03-04 10:19:06]

Patchset #1 (id:20001) has been deleted

[limasdf, 2015-03-04 10:19:12]

Patchset #1 (id:40001) has been deleted

[limasdf, 2015-03-04 10:19:16]

Patchset #1 (id:60001) has been deleted

[limasdf, 2015-03-04 10:19:23]

Patchset #1 (id:80001) has been deleted

[limasdf, 2015-03-04 15:04:35]

limasdf@gmail.com changed reviewers:
+ pkotwicz@chromium.org

[limasdf, 2015-03-04 15:05:08]

PTAL!

[pkotwicz, 2015-03-04 15:27:49]

lgtm

[limasdf, 2015-03-04 15:28:18]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2015-03-04 15:29:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/965233002/100001

[commit-bot: I haz the power, 2015-03-04 15:39:02]

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

[commit-bot: I haz the power, 2015-03-04 15:39:03]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[limasdf, 2015-03-04 22:35:52]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2015-03-04 22:42:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/965233002/100001

[commit-bot: I haz the power, 2015-03-04 23:23:05]

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

[commit-bot: I haz the power, 2015-03-04 23:23:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Lei Zhang, 2015-03-04 23:50:36]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-03-04 23:50:37]

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

https://codereview.chromium.org/965233002/diff/100001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:63: public
extensions::ExtensionRegistryObserver,
Rather than making ThemeService an ExtensionRegistryObserver, can you try adding
an inner class to ThemeService to be the observer that just calls back into
ThemeService? The goal of which is to avoid putting extensions headers in this
file.

[limasdf, 2015-03-05 06:04:52]

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

https://codereview.chromium.org/965233002/diff/100001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:63: public
extensions::ExtensionRegistryObserver,
On 2015/03/04 23:50:37, Lei Zhang wrote:
> Rather than making ThemeService an ExtensionRegistryObserver, can you try
adding
> an inner class to ThemeService to be the observer that just calls back into
> ThemeService? The goal of which is to avoid putting extensions headers in this
> file.

Sorry about my ignorance. do you mean this?

Add this inner class to ThemeService.

class Obvserver {
 public:
  void OnExtensionLoaded(..) {
    parent_theme_->OnExtensionLoaded(..);
  }
  void OnExtensionUnlaoded(..) { ... }
  void OnExtensionWillBeInstalled(..
}

and call
ThemeServiceFactory::GetForProfile(profile_)->Observer()->OnExtensionLoaded(..).

[Lei Zhang, 2015-03-05 06:18:46]

On 2015/03/05 06:04:52, limasdf wrote:
> Sorry about my ignorance. do you mean this?
> 
> Add this inner class to ThemeService.
> 
> class Obvserver {

Yes, I think that should work.

[limasdf, 2015-03-09 11:31:21]

limasdf@gmail.com changed reviewers:
+ kalman@chromium.org

[limasdf, 2015-03-09 11:31:46]

PTAL

+kalman for extension

[not at google - send to devlin, 2015-03-09 16:32:40]

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1079:
ThemeServiceFactory::GetForProfile(profile_)
The point of Observers is so that ExtensionService doesn't need to know about
anything that wants to observer. ThemeServiceFactory should be adding itself as
an Observer to ExtensionRegistry. ExtensionService should not get involved in
any way.

[limasdf, 2015-03-10 16:46:16]

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1079:
ThemeServiceFactory::GetForProfile(profile_)
On 2015/03/09 16:32:40, kalman wrote:
> The point of Observers is so that ExtensionService doesn't need to know about
> anything that wants to observer. ThemeServiceFactory should be adding itself
as
> an Observer to ExtensionRegistry. ExtensionService should not get involved in
> any way.

To make ThemeServiceFactory as ExtensionRegistryObserver, I have to add
"extension_registry_observer.h". this makes android_chromium_gn_compile_dbg
failed.

So my idea is 
create ThemeServiceDelegate and make it as ExtensionRegistryObserver.
and then, ThemeServiceDelegate::OnExtensionLoaded() calls
Themeservice::OnExtensionLoaded().

Does this looks good on you?

[not at google - send to devlin, 2015-03-10 16:50:18]

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1079:
ThemeServiceFactory::GetForProfile(profile_)
On 2015/03/10 16:46:16, limasdf wrote:
> On 2015/03/09 16:32:40, kalman wrote:
> > The point of Observers is so that ExtensionService doesn't need to know
about
> > anything that wants to observer. ThemeServiceFactory should be adding itself
> as
> > an Observer to ExtensionRegistry. ExtensionService should not get involved
in
> > any way.
> 
> To make ThemeServiceFactory as ExtensionRegistryObserver, I have to add
> "extension_registry_observer.h". this makes android_chromium_gn_compile_dbg
> failed.
> 
> So my idea is 
> create ThemeServiceDelegate and make it as ExtensionRegistryObserver.
> and then, ThemeServiceDelegate::OnExtensionLoaded() calls
> Themeservice::OnExtensionLoaded().
> 
> Does this looks good on you?

Seems like overkill. +thestig has a lot of experience with breaking android
dependencies - what is your suggestion?

[Lei Zhang, 2015-03-10 19:25:52]

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

https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:65: class ExtensionObserver {
This class needs to:

- inherit from extensions::ExtensionRegistryObserver
- be forward declared in the private section
- be defined in the .cc file as class ThemeService::ExtensionObserver

And |extension_observer_| needs to be a scoped_ptr

[Lei Zhang, 2015-03-10 19:34:43]

On 2015/03/10 19:25:52, Lei Zhang wrote:
>
https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
> File chrome/browser/themes/theme_service.h (right):
> 
>
https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
> chrome/browser/themes/theme_service.h:65: class ExtensionObserver {
> This class needs to:
> 
> - inherit from extensions::ExtensionRegistryObserver
> - be forward declared in the private section
> - be defined in the .cc file as class ThemeService::ExtensionObserver
> 
> And |extension_observer_| needs to be a scoped_ptr

Well, that's what you would do if you wanted to use ExtensionRegistryObserver
without including it in the header. However, kalman@ and I noticed ThemeService
isn't built on Android, so we probably should just go back to patch set 1, and
see if we can fix the #includes for theme_service.h.

[Lei Zhang, 2015-03-10 20:19:15]

On 2015/03/10 19:34:43, Lei Zhang wrote:
> On 2015/03/10 19:25:52, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
> > File chrome/browser/themes/theme_service.h (right):
> > 
> >
>
https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
> > chrome/browser/themes/theme_service.h:65: class ExtensionObserver {
> > This class needs to:
> > 
> > - inherit from extensions::ExtensionRegistryObserver
> > - be forward declared in the private section
> > - be defined in the .cc file as class ThemeService::ExtensionObserver
> > 
> > And |extension_observer_| needs to be a scoped_ptr
> 
> Well, that's what you would do if you wanted to use ExtensionRegistryObserver
> without including it in the header. However, kalman@ and I noticed
ThemeService
> isn't built on Android, so we probably should just go back to patch set 1, and
> see if we can fix the #includes for theme_service.h.

Thinking about it a little longer, one could in theory have themes enabled, but
not extensions, or vice versa. So I would go down the patch set 2 path, and also
put the extensions bits in theme_service behind ENABLE_EXTENSIONS.

I've separately prepared https://codereview.chromium.org/991353003 to put more
themes code behind defined(ENABLE_THEMES).

[limasdf, 2015-03-12 16:00:44]

On 2015/03/10 20:19:15, Lei Zhang wrote:
> On 2015/03/10 19:34:43, Lei Zhang wrote:
> > On 2015/03/10 19:25:52, Lei Zhang wrote:
> > >
> >
>
https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
> > > File chrome/browser/themes/theme_service.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/965233002/diff/120001/chrome/browser/themes/t...
> > > chrome/browser/themes/theme_service.h:65: class ExtensionObserver {
> > > This class needs to:
> > > 
> > > - inherit from extensions::ExtensionRegistryObserver
> > > - be forward declared in the private section
> > > - be defined in the .cc file as class ThemeService::ExtensionObserver
> > > 
> > > And |extension_observer_| needs to be a scoped_ptr
> > 
> > Well, that's what you would do if you wanted to use
ExtensionRegistryObserver
> > without including it in the header. However, kalman@ and I noticed
> ThemeService
> > isn't built on Android, so we probably should just go back to patch set 1,
and
> > see if we can fix the #includes for theme_service.h.
> 
> Thinking about it a little longer, one could in theory have themes enabled,
but
> not extensions, or vice versa. So I would go down the patch set 2 path, and
also
> put the extensions bits in theme_service behind ENABLE_EXTENSIONS.
> 
> I've separately prepared https://codereview.chromium.org/991353003 to put more
> themes code behind defined(ENABLE_THEMES).

Used patch set 2.
PTAL!

[not at google - send to devlin, 2015-03-12 17:08:34]

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

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:137: ThemeService& theme_service_;
nit: usually you would hold onto a pointer, not a reference (and pass in a
pointer, not a reference, above - to imply mutability)

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:148: extension_registry_(NULL),
nullptr

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:310:
extension_registry_->RemoveObserver(extension_observer_.get());
(see below - you might as well destroy the observer as well)

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:562:
extension_registry_->AddObserver(extension_observer_.get());
If you make ExtensionObserver add/remove itself as an Observer of
ExtensionRegistry itself, then all you need is the reset(...) here and the
reset() above.

That would also make it more natural to remove the awkwardness of holding onto
the ExtensionRegistry* itself, and let you remove even more #ifdefs.

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

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:195: #if defined(ENABLE_EXTENSIONS)
to minimise the number of #if defined(..) calls, you could forward-declare this
down on line 262?

[not at google - send to devlin, 2015-03-12 17:10:55]

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

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:196: class ExtensionObserver;
Also, I think ExtensionObserver could be better named - it doesn't describe its
function. ExtensionRegistryObserverForThemes is a bit of a mouthful, so how
about just ThemeObserver.

[Lei Zhang, 2015-03-12 21:07:23]

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

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:263: extensions::ExtensionRegistry*
extension_registry_;
Maybe encapsulate this within ExtensionObserver?

[limasdf, 2015-03-13 03:01:54]

Thanks much for the review.
PTAL!

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

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:137: ThemeService& theme_service_;
On 2015/03/12 17:08:33, kalman wrote:
> nit: usually you would hold onto a pointer, not a reference (and pass in a
> pointer, not a reference, above - to imply mutability)

Done.

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:148: extension_registry_(NULL),
On 2015/03/12 17:08:33, kalman wrote:
> nullptr

Done. including other place as well for the consistency.

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:310:
extension_registry_->RemoveObserver(extension_observer_.get());
On 2015/03/12 17:08:33, kalman wrote:
> (see below - you might as well destroy the observer as well)

Done.

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:562:
extension_registry_->AddObserver(extension_observer_.get());
On 2015/03/12 17:08:33, kalman wrote:
> If you make ExtensionObserver add/remove itself as an Observer of
> ExtensionRegistry itself, then all you need is the reset(...) here and the
> reset() above.
> 
> That would also make it more natural to remove the awkwardness of holding onto
> the ExtensionRegistry* itself, and let you remove even more #ifdefs.

Done.
Yes. Exactly.

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

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:195: #if defined(ENABLE_EXTENSIONS)
On 2015/03/12 17:08:33, kalman wrote:
> to minimise the number of #if defined(..) calls, you could forward-declare
this
> down on line 262?

Done.

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:196: class ExtensionObserver;
On 2015/03/12 17:10:55, kalman wrote:
> Also, I think ExtensionObserver could be better named - it doesn't describe
its
> function. ExtensionRegistryObserverForThemes is a bit of a mouthful, so how
> about just ThemeObserver.

Done.

https://codereview.chromium.org/965233002/diff/140001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.h:263: extensions::ExtensionRegistry*
extension_registry_;
On 2015/03/12 21:07:23, Lei Zhang wrote:
> Maybe encapsulate this within ExtensionObserver?

Done.

[Lei Zhang, 2015-03-13 03:21:07]

https://codereview.chromium.org/965233002/diff/160001/chrome/browser/themes/t...
File chrome/browser/themes/theme_syncable_service_unittest.cc (right):

https://codereview.chromium.org/965233002/diff/160001/chrome/browser/themes/t...
chrome/browser/themes/theme_syncable_service_unittest.cc:98: void Shutdown()
override {}
Is this needed?

[limasdf, 2015-03-13 04:51:34]

Thanks :)

https://codereview.chromium.org/965233002/diff/160001/chrome/browser/themes/t...
File chrome/browser/themes/theme_syncable_service_unittest.cc (right):

https://codereview.chromium.org/965233002/diff/160001/chrome/browser/themes/t...
chrome/browser/themes/theme_syncable_service_unittest.cc:98: void Shutdown()
override {}
On 2015/03/13 03:21:07, Lei Zhang wrote:
> Is this needed?

Removed. No need now.

[not at google - send to devlin, 2015-03-13 17:01:44]

lgtm. I can see some other things to potentially clean up, but not worth
spending too much time on this.

However: it occurs to me now that all this means if ENABLE_EXTENSIONS is false
then themes won't work. Before - they might work, I'm not sure.

It seems like therefore we should just not include themes in the compilation at
all if extensions are not compiled in. thestig@ any thoughts? Doesn't need to be
addressed right now, but direction needs to be taken into account, and possibly
comments added.

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

https://codereview.chromium.org/965233002/diff/180001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:110: }
Could you separate all of these methods with blank lines? Each are sufficiently
complex.

Also preferably make the event handling methods private: as well (i.e. move
"private:" up to line 115).

[Lei Zhang, 2015-03-13 21:09:53]

On 2015/03/13 17:01:44, kalman wrote:
> lgtm. I can see some other things to potentially clean up, but not worth
> spending too much time on this.
> 
> However: it occurs to me now that all this means if ENABLE_EXTENSIONS is false
> then themes won't work. Before - they might work, I'm not sure.
> 
> It seems like therefore we should just not include themes in the compilation
at
> all if extensions are not compiled in. thestig@ any thoughts? Doesn't need to
be
> addressed right now, but direction needs to be taken into account, and
possibly
> comments added.

I looked a bit more and it looks like themes depends on extensions. We can do
some cleanups later to make this fact clearer, so LGTM.

[Lei Zhang, 2015-03-13 21:09:57]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-03-13 21:09:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/965233002/#ps220001
(title: "move to private")

[commit-bot: I haz the power, 2015-03-13 21:10:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/965233002/220001

[commit-bot: I haz the power, 2015-03-13 21:57:19]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-03-13 21:58:25]

Patchset 7 (id:??) landed as
https://crrev.com/96f0702b9e5ff97bf1870b7ac7e614c2358a78aa
Cr-Commit-Position: refs/heads/master@{#320587}