Add "UninstallReason" parameter to ExtensionRegistryObserver::OnExtensionUninstalled.

Add "UninstallReason" parameter to ExtensionRegistryObserver::OnExtensionUninstalled.

BUG=84556
TBR=sky@chromium.org

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

[rpaquay, 2014-07-16 18:08:04]

This is a follow up to https://codereview.chromium.org/284103002/. There should
be no behavior change (next CL will contain the actual bug fix).

[Devlin, 2014-07-16 20:05:43]

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
chrome/browser/chrome_notification_types.h:422: // UnloadedExtensionInfo, and
the source is a Profile.
Is there a reason we need to include the type with the notification?  It doesn't
look like any of the current clients need it...

https://codereview.chromium.org/398083002/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:721:
UninstalledExtensionInfo::REASON_ORPHANED_EXTERNAL_EXTENSION) ||
nit: please extra-indent this line.

https://codereview.chromium.org/398083002/diff/1/chrome/browser/ui/app_list/e...
File chrome/browser/ui/app_list/extension_uninstaller.cc (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/ui/app_list/e...
chrome/browser/ui/app_list/extension_uninstaller.cc:13: using
extensions::UninstalledExtensionInfo;
for non-extensions code, let's limit the using statements (maybe only do it if
there are >= 3 occurrences).

[rpaquay, 2014-07-16 21:16:45]

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
chrome/browser/chrome_notification_types.h:422: // UnloadedExtensionInfo, and
the source is a Profile.
On 2014/07/16 20:05:43, Devlin wrote:
> Is there a reason we need to include the type with the notification?  It
doesn't
> look like any of the current clients need it...

The next CL will have a change to use "reason" from within one of the observers
(in extensions/browser/api/runtime_api.cc).

That said, there is a typo in the comment above (should be
"UninstalledExtensionInfo") that I am fixing in the next patchset.

https://codereview.chromium.org/398083002/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:721:
UninstalledExtensionInfo::REASON_ORPHANED_EXTERNAL_EXTENSION) ||
On 2014/07/16 20:05:43, Devlin wrote:
> nit: please extra-indent this line.

Done.

https://codereview.chromium.org/398083002/diff/1/chrome/browser/ui/app_list/e...
File chrome/browser/ui/app_list/extension_uninstaller.cc (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/ui/app_list/e...
chrome/browser/ui/app_list/extension_uninstaller.cc:13: using
extensions::UninstalledExtensionInfo;
On 2014/07/16 20:05:43, Devlin wrote:
> for non-extensions code, let's limit the using statements (maybe only do it if
> there are >= 3 occurrences).

Done.

[Devlin, 2014-07-16 21:29:38]

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
chrome/browser/chrome_notification_types.h:422: // UnloadedExtensionInfo, and
the source is a Profile.
On 2014/07/16 21:16:45, rpaquay wrote:
> On 2014/07/16 20:05:43, Devlin wrote:
> > Is there a reason we need to include the type with the notification?  It
> doesn't
> > look like any of the current clients need it...
> 
> The next CL will have a change to use "reason" from within one of the
observers
> (in extensions/browser/api/runtime_api.cc).
> 
> That said, there is a typo in the comment above (should be
> "UninstalledExtensionInfo") that I am fixing in the next patchset.

Hmm... I'm concerned about updating a deprecated notification.  It'll just make
the transition to ExtensionRegistryObserver that much harder.  Perhaps it would
be better to modify ExtensionRegistryObserver::OnExtensionUninstalled, and
update RuntimeAPI to use that instead?

[rpaquay, 2014-07-16 22:14:35]

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
chrome/browser/chrome_notification_types.h:422: // UnloadedExtensionInfo, and
the source is a Profile.
On 2014/07/16 21:29:38, Devlin wrote:
> On 2014/07/16 21:16:45, rpaquay wrote:
> > On 2014/07/16 20:05:43, Devlin wrote:
> > > Is there a reason we need to include the type with the notification?  It
> > doesn't
> > > look like any of the current clients need it...
> > 
> > The next CL will have a change to use "reason" from within one of the
> observers
> > (in extensions/browser/api/runtime_api.cc).
> > 
> > That said, there is a typo in the comment above (should be
> > "UninstalledExtensionInfo") that I am fixing in the next patchset.
> 
> Hmm... I'm concerned about updating a deprecated notification.  It'll just
make
> the transition to ExtensionRegistryObserver that much harder.  Perhaps it
would
> be better to modify ExtensionRegistryObserver::OnExtensionUninstalled, and
> update RuntimeAPI to use that instead?

hmmkay, that can be done. Where do you suggest I move the "UninstallReason" enum
definition (I was trying to follow the same pattern as UnloadedExtensionInfo)?

* If I leave in ExtensionService, it creates a strong dependency with
ExtensionRegistry
* If I create UninstalledExtensionInfo similar to UnloadeExtensionInfo, that new
type is not used anywhere (I am assuming
ExtensionRegistryObserver::OnExtensionUninstalled will follow the same pattern
as ExtensionRegistryObserver::OnExtensionUnloaded).

[Devlin, 2014-07-16 22:17:30]

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
chrome/browser/chrome_notification_types.h:422: // UnloadedExtensionInfo, and
the source is a Profile.
On 2014/07/16 22:14:35, rpaquay wrote:
> On 2014/07/16 21:29:38, Devlin wrote:
> > On 2014/07/16 21:16:45, rpaquay wrote:
> > > On 2014/07/16 20:05:43, Devlin wrote:
> > > > Is there a reason we need to include the type with the notification?  It
> > > doesn't
> > > > look like any of the current clients need it...
> > > 
> > > The next CL will have a change to use "reason" from within one of the
> > observers
> > > (in extensions/browser/api/runtime_api.cc).
> > > 
> > > That said, there is a typo in the comment above (should be
> > > "UninstalledExtensionInfo") that I am fixing in the next patchset.
> > 
> > Hmm... I'm concerned about updating a deprecated notification.  It'll just
> make
> > the transition to ExtensionRegistryObserver that much harder.  Perhaps it
> would
> > be better to modify ExtensionRegistryObserver::OnExtensionUninstalled, and
> > update RuntimeAPI to use that instead?
> 
> hmmkay, that can be done. Where do you suggest I move the "UninstallReason"
enum
> definition (I was trying to follow the same pattern as UnloadedExtensionInfo)?
> 
> * If I leave in ExtensionService, it creates a strong dependency with
> ExtensionRegistry
> * If I create UninstalledExtensionInfo similar to UnloadeExtensionInfo, that
new
> type is not used anywhere (I am assuming
> ExtensionRegistryObserver::OnExtensionUninstalled will follow the same pattern
> as ExtensionRegistryObserver::OnExtensionUnloaded).
> 

I would actually say let's put it in its own file
(extensions/common/uninstall_reason.h).  It'll be used enough that it makes
sense, and that way people know where to look. ;)

[rpaquay, 2014-07-16 23:45:16]

PTAL. Note that this CL should still be a refactoring with no behavior change,
i.e. no consumer is using the new "UninstallReason" parameter yet.

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/398083002/diff/1/chrome/browser/chrome_notifi...
chrome/browser/chrome_notification_types.h:422: // UnloadedExtensionInfo, and
the source is a Profile.
On 2014/07/16 22:17:30, Devlin wrote:
> On 2014/07/16 22:14:35, rpaquay wrote:
> > On 2014/07/16 21:29:38, Devlin wrote:
> > > On 2014/07/16 21:16:45, rpaquay wrote:
> > > > On 2014/07/16 20:05:43, Devlin wrote:
> > > > > Is there a reason we need to include the type with the notification? 
It
> > > > doesn't
> > > > > look like any of the current clients need it...
> > > > 
> > > > The next CL will have a change to use "reason" from within one of the
> > > observers
> > > > (in extensions/browser/api/runtime_api.cc).
> > > > 
> > > > That said, there is a typo in the comment above (should be
> > > > "UninstalledExtensionInfo") that I am fixing in the next patchset.
> > > 
> > > Hmm... I'm concerned about updating a deprecated notification.  It'll just
> > make
> > > the transition to ExtensionRegistryObserver that much harder.  Perhaps it
> > would
> > > be better to modify ExtensionRegistryObserver::OnExtensionUninstalled, and
> > > update RuntimeAPI to use that instead?
> > 
> > hmmkay, that can be done. Where do you suggest I move the "UninstallReason"
> enum
> > definition (I was trying to follow the same pattern as
UnloadedExtensionInfo)?
> > 
> > * If I leave in ExtensionService, it creates a strong dependency with
> > ExtensionRegistry
> > * If I create UninstalledExtensionInfo similar to UnloadeExtensionInfo, that
> new
> > type is not used anywhere (I am assuming
> > ExtensionRegistryObserver::OnExtensionUninstalled will follow the same
pattern
> > as ExtensionRegistryObserver::OnExtensionUnloaded).
> > 
> 
> I would actually say let's put it in its own file
> (extensions/common/uninstall_reason.h).  It'll be used enough that it makes
> sense, and that way people know where to look. ;)

Done.

[Devlin, 2014-07-17 20:22:05]

Two things:
- It looks like most of the objects uninstalling the extensions don't include
the file - they should (you don't have to worry about all the subclasses of
ExtensionRegistryObserver, though).
- It looks like the uninstall_reason.h file wasn't added. :)  And on that note,
let's actually move it to extensions/browser for now (until we have a reason to
have it in common).

Otherwise, this change lg.

[rpaquay, 2014-07-17 21:37:08]

PTAL: Both issues addressed.

[Devlin, 2014-07-17 21:48:41]

On 2014/07/17 21:37:08, rpaquay wrote:
> PTAL: Both issues addressed.

Awesome, thanks :)

LGTM

[rpaquay, 2014-07-20 17:22:56]

sky@: TBR for trivial changes under chrome/browser, except
chrome/browser/extensions (already lgtm'ed).

[rpaquay, 2014-07-20 17:23:09]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-07-20 17:23:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rpaquay@chromium.org/398083002/100001

[commit-bot: I haz the power, 2014-07-20 21:46:42]

Change committed as 284381