Remove InstallTracker's extension load, unload observer

Remove InstallTracker's extension load, unload observer.
Since ExtensionRegistry provide canonical way, use  ExtensionRegistry instead.

BUG=374353
TEST=unit_tests

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

[limasdf, 2014-05-29 17:11:29]

when you have time

[limasdf, 2014-05-29 18:04:16]

Sorry. Hold on please, there's some failed test.
After fixing it i'll re-request

[not at google - send to devlin, 2014-05-29 18:09:29]

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log.cc (left):

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log.cc:700:
DependsOn(InstallTrackerFactory::GetInstance());
strictly speaking you should add ExtensionRegistryFactory here

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:136:
existing_item->Reload();
ugh annoying diff. but it looks right.

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (left):

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:197: void
AppResult::OnShutdown() { StopObservingInstall(); }
hm, I'm a bit worried about losing this; the registry *should* outlive the app
list, but it's being dealt with now.

can you add an OnShutdown() method to ExtensionRegistryObserver? it'll be pretty
trivial to add, might as well do it now in this CL.

[limasdf, 2014-05-29 18:17:50]

Thank you for the review.
I'll fix address both your comments and fix test. and then upload agian.

[limasdf, 2014-05-30 21:00:49]

please take a look. I added OnShutdown to ExtensionRegistryObserver.

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log.cc (left):

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log.cc:700:
DependsOn(InstallTrackerFactory::GetInstance());
On 2014/05/29 18:09:29, kalman wrote:
> strictly speaking you should add ExtensionRegistryFactory here

Done.

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:136:
existing_item->Reload();
On 2014/05/29 18:09:29, kalman wrote:
> ugh annoying diff. but it looks right.

rearranged :)

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (left):

https://codereview.chromium.org/298023009/diff/230001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:197: void
AppResult::OnShutdown() { StopObservingInstall(); }
On 2014/05/29 18:09:29, kalman wrote:
> hm, I'm a bit worried about losing this; the registry *should* outlive the app
> list, but it's being dealt with now.
> 
> can you add an OnShutdown() method to ExtensionRegistryObserver? it'll be
pretty
> trivial to add, might as well do it now in this CL.

Done.
I added only AppResult. perhaps I have to recover OnShutdown where It was used?

[tfarina, 2014-05-30 23:05:15]

Benjamin, my observation below is mostly for you, based on what I'm experiencing
in https://codereview.chromium.org/297293002

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:133:
extension_registry_->RemoveObserver(this);
right, so if ExtensionRegistry's |observers_| was declared as:

ObserverList<ExtensionRegistryObserver, true> observers_; 

Then, if |extension_registry_| is NULL here, it would mean we had registered (in
the ctor), but never unregistered.

And the DCHECK in ObserverList would have been fired:

https://chromium.googlesource.com/chromium/chromium/+/trunk/base/observer_lis...

Is it possible that |extension_registry_| will be NULL at the time AppResult is
destroyed?

[not at google - send to devlin, 2014-05-30 23:22:17]

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:187: void
ExtensionAppModelBuilder::OnShutdown() {
again strictly speaking you should be listening to the Registry's OnShutdown and
doing the same for that.

i'm wondering whether OnShutdown should actually take an ExtensionRegistry
instance, not a BrowserContext, to avoid any method overloading issues.

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:133:
extension_registry_->RemoveObserver(this);
On 2014/05/30 23:05:15, tfarina wrote:
> right, so if ExtensionRegistry's |observers_| was declared as:
> 
> ObserverList<ExtensionRegistryObserver, true> observers_; 
> 
> Then, if |extension_registry_| is NULL here, it would mean we had registered
(in
> the ctor), but never unregistered.
> 
> And the DCHECK in ObserverList would have been fired:
> 
>
https://chromium.googlesource.com/chromium/chromium/+/trunk/base/observer_lis...
> 
> Is it possible that |extension_registry_| will be NULL at the time AppResult
is
> destroyed?

I think it would mean that StopObservingExtensionRegistry was called twice, e.g.
first from OnShutdown (which gets called as part of Profile shutdown; it's not
clear to me that AppResult is tied to Profile shutdown) and then on destruction.

if it was called in the other order then we have problems.

(Of course, it's not declared as ObserverList<ExtensionRegistryObserver, true>
but rather than ObserverList<ExtensionRegistryObserver>, but including the
"true" there is probably a good idea.)

I'm not sure if that answered your question...

[limasdf, 2014-06-02 11:49:04]

please take a look

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:187: void
ExtensionAppModelBuilder::OnShutdown() {
On 2014/05/30 23:22:17, kalman wrote:
> again strictly speaking you should be listening to the Registry's OnShutdown
and
> doing the same for that.
> 
> i'm wondering whether OnShutdown should actually take an ExtensionRegistry
> instance, not a BrowserContext, to avoid any method overloading issues.

Done. To avoid method overloading issue, I have renamed
ExtensionRegistryObserver::OnShutdown to OnExtensionShutdown.

[not at google - send to devlin, 2014-06-02 19:45:53]

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/390001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:187: void
ExtensionAppModelBuilder::OnShutdown() {
On 2014/06/02 11:49:04, limasdf wrote:
> On 2014/05/30 23:22:17, kalman wrote:
> > again strictly speaking you should be listening to the Registry's OnShutdown
> and
> > doing the same for that.
> > 
> > i'm wondering whether OnShutdown should actually take an ExtensionRegistry
> > instance, not a BrowserContext, to avoid any method overloading issues.
> 
> Done. To avoid method overloading issue, I have renamed
> ExtensionRegistryObserver::OnShutdown to OnExtensionShutdown.

OnExtensionShutdown isn't really right. It's not attached to a specific
extension. Overloading based on passing in an ExtensionREgistry will work just
fine.

[limasdf, 2014-06-03 18:46:44]

kalman, could you take a look at this?
changed OnExtensionShutdown -> OnShutdown

[not at google - send to devlin, 2014-06-03 19:06:35]

https://codereview.chromium.org/298023009/diff/480001/extensions/browser/exte...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/298023009/diff/480001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:67: virtual void OnShutdown()
{}
OnShutdown should take an ExtensionRegistry

virtual void OnShutdown(const ExtensionRegistry* registry) {}

[not at google - send to devlin, 2014-06-03 19:08:02]

https://codereview.chromium.org/298023009/diff/480001/extensions/browser/exte...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/298023009/diff/480001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:67: virtual void OnShutdown()
{}
On 2014/06/03 19:06:35, kalman wrote:
> OnShutdown should take an ExtensionRegistry
> 
> virtual void OnShutdown(const ExtensionRegistry* registry) {}

(but drop the "const")

[limasdf, 2014-06-04 18:29:14]

dear kalman, I am sorry that I wasn't fully understand your previous comments(I
blame my English!)

OnShutdown receive registry. and it works well.

PTAL.

[not at google - send to devlin, 2014-06-04 18:36:35]

lgtm but needs a couple of DCHECKs.

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:200: if
(extension_registry_ == registry) {
this should be a DCHECK since it should always be true.

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:199: if (extension_registry_ ==
registry)
DCHECK

[limasdf, 2014-06-04 18:53:09]

Added @xiyuan as the owner of ui/app_list

Xiyuan, could you take a look?

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/extension_app_model_builder.cc:200: if
(extension_registry_ == registry) {
On 2014/06/04 18:36:35, kalman wrote:
> this should be a DCHECK since it should always be true.

Done.

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/298023009/diff/520001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:199: if (extension_registry_ ==
registry)
On 2014/06/04 18:36:35, kalman wrote:
> DCHECK

Done.

[xiyuan, 2014-06-04 19:39:37]

ui/app_list LGTM

https://codereview.chromium.org/298023009/diff/540001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/app_result.cc (right):

https://codereview.chromium.org/298023009/diff/540001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/app_result.cc:200:
StopObservingExtensionRegistry();
nit: wrong indent.

[limasdf, 2014-06-05 00:53:27]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-06-05 00:55:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/298023009/560001

[commit-bot: I haz the power, 2014-06-05 05:23:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13162)

[commit-bot: I haz the power, 2014-06-05 05:28:21]

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

[commit-bot: I haz the power, 2014-06-05 05:28:22]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[limasdf, 2014-06-05 06:15:34]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-06-05 06:16:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/298023009/560001

[commit-bot: I haz the power, 2014-06-05 07:47:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13214)

[commit-bot: I haz the power, 2014-06-05 07:52:46]

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

[commit-bot: I haz the power, 2014-06-05 07:52:47]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[limasdf, 2014-06-05 13:41:15]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-06-05 13:43:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/298023009/560001

[commit-bot: I haz the power, 2014-06-05 15:26:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13364)

[commit-bot: I haz the power, 2014-06-05 15:58:02]

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

[commit-bot: I haz the power, 2014-06-05 15:58:02]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13391)

[limasdf, 2014-06-05 20:24:55]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-06-05 20:26:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/298023009/600001

[commit-bot: I haz the power, 2014-06-06 00:24:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13599)

[commit-bot: I haz the power, 2014-06-06 00:53:30]

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

[commit-bot: I haz the power, 2014-06-06 00:53:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/13664)

[limasdf, 2014-06-06 11:44:57]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-06-06 11:46:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/limasdf@gmail.com/298023009/620001

[commit-bot: I haz the power, 2014-06-06 14:08:50]

Change committed as 275434