Pass Reason as a parameter with OnExtensionUnloaded.

Pass Reason as a parameter with OnExtensionUnloaded.

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

[dhnishi (use Chromium), 2014-04-07 17:14:49]

@kalman

I changed OnExtensionUnloaded to send the whole details like the notification
did before, as we discussed in email.

PTAL when you get a moment.

[not at google - send to devlin, 2014-04-07 17:52:32]

https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
(right):

https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:31:
struct UnloadedExtensionInfo;
you shouldn't need to forward declare this since extension_registry_observer.h
already does so.

https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
extensions/browser/extension_registry_observer.h:25: virtual void
OnExtensionUnloaded(const UnloadedExtensionInfo* reason) {}
So the thing is that the UnloadedExtensionInfo struct is only a struct because
of the way that notifications work; you pass a single pointer to notifications.

Now that this is an Observer we can make a more convenient interface by just
unwrapping the arguments, to be

OnExtensionUnloaded(const Extension* extension,
                    ExtensionUnloadedReason unload_reason);

or whatever.

Having the unload reason be in extension.h isn't really right anyway, it should
be in here, but now isn't the actual time to make such a change.

Perhaps you could add a TODO there, though;

TODO(dhnishi): Move this enum to ExtensionRegistryObserver.
enum Reason {
  ...
}

[Lei Zhang, 2014-04-07 19:15:28]

Note: https://codereview.chromium.org/207613002/ will collide with this. It got
stuck in CQ last week, so it has not been committed just yet. :\

[dhnishi (use Chromium), 2014-04-09 01:04:08]

On 2014/04/07 17:52:32, kalman wrote:
>
https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
> (right):
> 
>
https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
>
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:31:
> struct UnloadedExtensionInfo;
> you shouldn't need to forward declare this since extension_registry_observer.h
> already does so.
> 
>
https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
> File extensions/browser/extension_registry_observer.h (right):
> 
>
https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
> extensions/browser/extension_registry_observer.h:25: virtual void
> OnExtensionUnloaded(const UnloadedExtensionInfo* reason) {}
> So the thing is that the UnloadedExtensionInfo struct is only a struct because
> of the way that notifications work; you pass a single pointer to
notifications.
> 
> Now that this is an Observer we can make a more convenient interface by just
> unwrapping the arguments, to be
> 
> OnExtensionUnloaded(const Extension* extension,
>                     ExtensionUnloadedReason unload_reason);
> 
> or whatever.
> 
> Having the unload reason be in extension.h isn't really right anyway, it
should
> be in here, but now isn't the actual time to make such a change.
> 
> Perhaps you could add a TODO there, though;
> 
> TODO(dhnishi): Move this enum to ExtensionRegistryObserver.
> enum Reason {
>   ...
> }

Should I just import the Extension.h file, since I can't forward declare the
enum, or is there a more suitable course of action?

[Lei Zhang, 2014-04-09 01:12:28]

On 2014/04/09 01:04:08, Daniel Nishi wrote:
> On 2014/04/07 17:52:32, kalman wrote:
> >
>
https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
> > File
> >
>
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
> >
>
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:31:
> > struct UnloadedExtensionInfo;
> > you shouldn't need to forward declare this since
extension_registry_observer.h
> > already does so.
> > 
> >
>
https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
> > File extensions/browser/extension_registry_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
> > extensions/browser/extension_registry_observer.h:25: virtual void
> > OnExtensionUnloaded(const UnloadedExtensionInfo* reason) {}
> > So the thing is that the UnloadedExtensionInfo struct is only a struct
because
> > of the way that notifications work; you pass a single pointer to
> notifications.
> > 
> > Now that this is an Observer we can make a more convenient interface by just
> > unwrapping the arguments, to be
> > 
> > OnExtensionUnloaded(const Extension* extension,
> >                     ExtensionUnloadedReason unload_reason);
> > 
> > or whatever.
> > 
> > Having the unload reason be in extension.h isn't really right anyway, it
> should
> > be in here, but now isn't the actual time to make such a change.
> > 
> > Perhaps you could add a TODO there, though;
> > 
> > TODO(dhnishi): Move this enum to ExtensionRegistryObserver.
> > enum Reason {
> >   ...
> > }
> 
> Should I just import the Extension.h file, since I can't forward declare the
> enum, or is there a more suitable course of action?

I think kalman's saying struct UnloadedExtensionInfo should go away. One step in
getting rid of it is to move enum UnloadedExtensionInfo::Reason to where it
needs to be.

[not at google - send to devlin, 2014-04-09 03:07:02]

On 2014/04/09 01:12:28, Lei Zhang wrote:
> On 2014/04/09 01:04:08, Daniel Nishi wrote:
> > On 2014/04/07 17:52:32, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
> > > File
> > >
> >
>
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
> > >
> >
>
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:31:
> > > struct UnloadedExtensionInfo;
> > > you shouldn't need to forward declare this since
> extension_registry_observer.h
> > > already does so.
> > > 
> > >
> >
>
https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
> > > File extensions/browser/extension_registry_observer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
> > > extensions/browser/extension_registry_observer.h:25: virtual void
> > > OnExtensionUnloaded(const UnloadedExtensionInfo* reason) {}
> > > So the thing is that the UnloadedExtensionInfo struct is only a struct
> because
> > > of the way that notifications work; you pass a single pointer to
> > notifications.
> > > 
> > > Now that this is an Observer we can make a more convenient interface by
just
> > > unwrapping the arguments, to be
> > > 
> > > OnExtensionUnloaded(const Extension* extension,
> > >                     ExtensionUnloadedReason unload_reason);
> > > 
> > > or whatever.
> > > 
> > > Having the unload reason be in extension.h isn't really right anyway, it
> > should
> > > be in here, but now isn't the actual time to make such a change.
> > > 
> > > Perhaps you could add a TODO there, though;
> > > 
> > > TODO(dhnishi): Move this enum to ExtensionRegistryObserver.
> > > enum Reason {
> > >   ...
> > > }
> > 
> > Should I just import the Extension.h file, since I can't forward declare the
> > enum, or is there a more suitable course of action?
> 
> I think kalman's saying struct UnloadedExtensionInfo should go away. One step
in
> getting rid of it is to move enum UnloadedExtensionInfo::Reason to where it
> needs to be.

Yep, and I'm happy for you to #include extension.h so we can submit this.

[dhnishi (use Chromium), 2014-04-09 18:50:58]

https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h
(right):

https://codereview.chromium.org/222903002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries_private/gallery_watch_state_tracker.h:31:
struct UnloadedExtensionInfo;
On 2014/04/07 17:52:32, kalman wrote:
> you shouldn't need to forward declare this since extension_registry_observer.h
> already does so.

Done.

https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/222903002/diff/20001/extensions/browser/exten...
extensions/browser/extension_registry_observer.h:25: virtual void
OnExtensionUnloaded(const UnloadedExtensionInfo* reason) {}
On 2014/04/07 17:52:32, kalman wrote:
> So the thing is that the UnloadedExtensionInfo struct is only a struct because
> of the way that notifications work; you pass a single pointer to
notifications.
> 
> Now that this is an Observer we can make a more convenient interface by just
> unwrapping the arguments, to be
> 
> OnExtensionUnloaded(const Extension* extension,
>                     ExtensionUnloadedReason unload_reason);
> 
> or whatever.
> 
> Having the unload reason be in extension.h isn't really right anyway, it
should
> be in here, but now isn't the actual time to make such a change.
> 
> Perhaps you could add a TODO there, though;
> 
> TODO(dhnishi): Move this enum to ExtensionRegistryObserver.
> enum Reason {
>   ...
> }

TODO added.

[not at google - send to devlin, 2014-04-09 19:51:02]

lgtm

[Lei Zhang, 2014-04-09 20:05:07]

lgtm with a couple nits

https://codereview.chromium.org/222903002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_registry_unittest.cc (right):

https://codereview.chromium.org/222903002/diff/80001/extensions/browser/exten...
extensions/browser/extension_registry_unittest.cc:53: const
UnloadedExtensionInfo::Reason reason)
This probably doesn't need to be const, just like you rarely have a const int
parameter.

https://codereview.chromium.org/222903002/diff/80001/extensions/browser/runti...
File extensions/browser/runtime_data_unittest.cc (right):

https://codereview.chromium.org/222903002/diff/80001/extensions/browser/runti...
extensions/browser/runtime_data_unittest.cc:103: UnloadedExtensionInfo
details(extension,
why bother creating a UnloadedExtensionInfo?

runtime_data_.OnExtensionUnloaded(NULL, extension,
UnloadedExtensionInfo::REASON_DISABLE);

[Lei Zhang, 2014-04-10 21:45:55]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-10 21:47:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/222903002/100001

[commit-bot: I haz the power, 2014-04-10 23:40:17]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-10 23:40:18]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[Lei Zhang, 2014-04-10 23:48:19]

Please fix chrome/browser/media_galleries/media_scan_manager.* too.

[dhnishi (use Chromium), 2014-04-14 19:18:01]

On 2014/04/10 23:48:19, Lei Zhang wrote:
> Please fix chrome/browser/media_galleries/media_scan_manager.* too.

Done.

[Lei Zhang, 2014-04-14 20:49:33]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-14 20:49:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/222903002/120001

[commit-bot: I haz the power, 2014-04-14 21:54:53]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-14 21:54:54]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[limasdf, 2014-04-15 05:17:21]

On 2014/04/14 21:54:54, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on mac_chromium_compile_dbg

dear daniel. you might want to rebase.

[dhnishi (use Chromium), 2014-04-16 18:36:38]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-16 18:37:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/222903002/200001

[commit-bot: I haz the power, 2014-04-16 18:40:22]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-16 18:40:22]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[dhnishi (use Chromium), 2014-04-18 18:42:34]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-18 18:42:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/222903002/200001

[commit-bot: I haz the power, 2014-04-18 20:05:51]

Change committed as 264837