Remove deprecated extension notifications from APIs

Remove deprecated extension notifications and
Use ExtensionRegistry instead from file_system, location, management apis.
and Use content:BrowserContext instead of Profile.
and Remove unwanted extensions:: namespace prefix from management api.

TEST=browser_tests --gtest_filter=FileSystemApiTest.*
R=rdevlin.cronin@chromium.org
BUG=354046
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278486

[limasdf, 2014-06-14 18:09:37]

when you have time

[Devlin, 2014-06-17 16:28:31]

Sorry for the delay, and, as always, thanks for the work :)

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:24:
AppLoadObserver(Profile* profile,
nit: This could actually just be a BrowserContext.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:31: const
Extension* extension) OVERRIDE {
nit: indentation is off

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/location/location_manager.h (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/location/location_manager.h:34: // chrome.location
nit: Fix line wrapping.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:22: #include
"chrome/browser/chrome_notification_types.h"
We can get rid of this one, too, right?

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:950: if
(ui_util::ShouldNotBeVisible(extension, browser_context_))
Let's go ahead and move this stuff into one function, BroadcastEvent(const
Extension* extension, const char* event_name) that does all this.  Then each
observer method should just call BroadcastEvent with the extension and the
appropriate event name.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.h:246: virtual void
OnExtensionWillBeInstalled(
For what this is doing, I think maybe OnExtensionInstalled() is a better fit.

[limasdf, 2014-06-17 18:25:48]

Thanks for the review. and I'm sorry about my nits(typo, indentation, wrapping).
I think I lose focus around 3AM :/

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:24:
AppLoadObserver(Profile* profile,
On 2014/06/17 16:28:31, Devlin wrote:
> nit: This could actually just be a BrowserContext.

Done.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:31: const
Extension* extension) OVERRIDE {
On 2014/06/17 16:28:31, Devlin wrote:
> nit: indentation is off

Done.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:197:
AppLoadObserver observer(profile(),
I'm actually like to pass something like a browser_context(). but there's no :/

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/location/location_manager.h (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/location/location_manager.h:34: // chrome.location
On 2014/06/17 16:28:31, Devlin wrote:
> nit: Fix line wrapping.

Sorry.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:22: #include
"chrome/browser/chrome_notification_types.h"
On 2014/06/17 16:28:31, Devlin wrote:
> We can get rid of this one, too, right?

Done.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.cc:950: if
(ui_util::ShouldNotBeVisible(extension, browser_context_))
On 2014/06/17 16:28:31, Devlin wrote:
> Let's go ahead and move this stuff into one function, BroadcastEvent(const
> Extension* extension, const char* event_name) that does all this.  Then each
> observer method should just call BroadcastEvent with the extension and the
> appropriate event name.

Done. Except OnExtensionUninstalled. It has different pattern.

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/management/management_api.h:246: virtual void
OnExtensionWillBeInstalled(
On 2014/06/17 16:28:31, Devlin wrote:
> For what this is doing, I think maybe OnExtensionInstalled() is a better fit.

Done.
Yep, Perhaps, There must be many other places to use OnExtensionInstalled()
instead of OnExtensionWillBeInstalled().

[Devlin, 2014-06-18 16:04:34]

A few more small comments. :)

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:197:
AppLoadObserver observer(profile(),
On 2014/06/17 18:25:43, limasdf wrote:
> I'm actually like to pass something like a browser_context(). but there's no
:/

Yeah, that's kind of annoying.  What we could (and probably should) do in a
followup patch is to add a "browser_context()" method to ExtensionBrowserTest
which returns the profile as a browser context.  But it's not too pressing (and
don't worry about it here).

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.cc:984:
scoped_ptr<management::ExtensionInfo> info =
A few small things:
- You actually don't need to pass in the browser context - it should always be
from this objects |browser_context_|.
- You can inline the event into the EventRouter broadcast.
- I think I'd like to keep all the logic for dispatching in one place, and just
put in the if statement again.  The reason is it makes it easier to modify the
dispatch logic later on.  For instance, if we decided to take out the
early-return on ShouldNotBeVisible(), someone might overlook the dispatch in
OnExtensionUninstalled().

So,

void ManagementEventRouter::BroadcastEvent(const Extension* extension,
                                           const char* event_name) {
  if (ui_util::ShouldNotBeVisible(extension, browser_context_))
    return;  // Don't dispatch events for built-in extenions.
  scoped_ptr<base::ListValue> args(new base::ListValue());
  if (event_name == management::OnUninstalled::kEventName) {
    args->Append(new base::StringValue(extension->id());
  } else {
    scoped_ptr<management::ExtensionInfo> info = CreateExtensionInfo(
        *extension, ExtensionSystem::Get(browser_context_));
    args->Append(info->ToValue().release());
  }

  EventRouter::Get(browser_context_)->BroadcastEvent(
      scoped_ptr<Event>(new Event(event_name, args.Pass())));
}

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:239: private:
nit: // ExtensionRegistryObserver implementation.

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:251: void
BroadcastEvent(const Extension* extension,
nit: comment to the effect of
// Dispatches management api events to listening extensions.

[limasdf, 2014-06-19 01:48:43]

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/330383003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:197:
AppLoadObserver observer(profile(),
On 2014/06/18 16:04:34, Devlin wrote:
> On 2014/06/17 18:25:43, limasdf wrote:
> > I'm actually like to pass something like a browser_context(). but there's no
> :/
> 
> Yeah, that's kind of annoying.  What we could (and probably should) do in a
> followup patch is to add a "browser_context()" method to ExtensionBrowserTest
> which returns the profile as a browser context.  But it's not too pressing
(and
> don't worry about it here).

one day I'll do it!

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.cc:984:
scoped_ptr<management::ExtensionInfo> info =
On 2014/06/18 16:04:34, Devlin wrote:
> A few small things:
> - You actually don't need to pass in the browser context - it should always be
> from this objects |browser_context_|.
> - You can inline the event into the EventRouter broadcast.
> - I think I'd like to keep all the logic for dispatching in one place, and
just
> put in the if statement again.  The reason is it makes it easier to modify the
> dispatch logic later on.  For instance, if we decided to take out the
> early-return on ShouldNotBeVisible(), someone might overlook the dispatch in
> OnExtensionUninstalled().
> 
> So,
> 
> void ManagementEventRouter::BroadcastEvent(const Extension* extension,
>                                            const char* event_name) {
>   if (ui_util::ShouldNotBeVisible(extension, browser_context_))
>     return;  // Don't dispatch events for built-in extenions.
>   scoped_ptr<base::ListValue> args(new base::ListValue());
>   if (event_name == management::OnUninstalled::kEventName) {
>     args->Append(new base::StringValue(extension->id());
>   } else {
>     scoped_ptr<management::ExtensionInfo> info = CreateExtensionInfo(
>         *extension, ExtensionSystem::Get(browser_context_));
>     args->Append(info->ToValue().release());
>   }
> 
>   EventRouter::Get(browser_context_)->BroadcastEvent(
>       scoped_ptr<Event>(new Event(event_name, args.Pass())));
> }

Done.
> The reason is it makes it easier to modify the dispatch logic later on.
Nice. I have learned good point :)

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/management/management_api.h (right):

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:239: private:
On 2014/06/18 16:04:34, Devlin wrote:
> nit: // ExtensionRegistryObserver implementation.

Done.

https://codereview.chromium.org/330383003/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/management/management_api.h:251: void
BroadcastEvent(const Extension* extension,
On 2014/06/18 16:04:34, Devlin wrote:
> nit: comment to the effect of
> // Dispatches management api events to listening extensions.

Done.

[Devlin, 2014-06-19 16:56:21]

lgtm! :)

[limasdf, 2014-06-19 16:56:57]

Yay!!

[limasdf, 2014-06-19 16:57:01]

The CQ bit was checked by limasdf@gmail.com

[commit-bot: I haz the power, 2014-06-19 16:58:50]

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

[commit-bot: I haz the power, 2014-06-19 21:14:22]

Change committed as 278486