Remove NOTIFICATION_EXTENSION_ENABLED.

Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, there are also 2 other reasons for EventRouter code in b):
b1) We also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.
b2) A component extension reload does not go through
ExtensionRegistry::TriggerOnInstalled, so it doesn't see any
OnExtensionWillBeInstalled. Couple the logic with b1).

TBR=benwells@chromium.org for added test in app_browsertest.cc
BUG=723754, 411569
Test=No visible changes expected.

Review-Url: https://codereview.chromium.org/2893693002
Cr-Commit-Position: refs/heads/master@{#477357}
Committed: https://chromium.googlesource.com/chromium/src/+/75b9def1648c17c0edc926e7ea787519e9c59899

[lazyboy, 2017-05-17 18:11:48]

Description was changed from

==========
Remove/replace NOTIFICATION_EXTENSION_ENABLED.

Add ExtensionRegistryObserver method OnExtensionEnabled instead.
This is one baby step towards cleaning up EventRouter code.

BUG=723754, 411569
Test=No visible changes expected.
==========

to

==========
Remove/replace NOTIFICATION_EXTENSION_ENABLED.

Add ExtensionRegistryObserver method OnExtensionEnabled instead.
This is one baby step towards cleaning up EventRouter code (in terms of code
lines, but not really a logical cleanup).

BUG=723754, 411569
Test=No visible changes expected.
==========

[lazyboy, 2017-05-17 18:14:36]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[lazyboy, 2017-05-17 18:14:47]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-05-17 18:14:48]

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/input_ime/input_ime_api.h (right):

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/input_ime/input_ime_api.h:21: #include
"content/public/browser/notification_registrar.h"
This was transitively being included through EventRouter.

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/signed_in_devices/signed_in_devices_manager.h
(right):

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_manager.h:16:
#include "content/public/browser/notification_registrar.h"
^^^ Same here.

[commit-bot: I haz the power, 2017-05-17 18:15:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-05-17 18:37:49]

I'm wondering if we need an extension enabled notification at all, or if we can
just use OnExtensionLoaded... WDYT?

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:195: void
OnExtensionEnabled(content::BrowserContext* browser_context,
Hmm... does this care about enablement?

OnExtensionLoaded(), I think, can be called from three paths:
1. A new extension is added
2. A disabled extension is enabled
3. A terminated extension is reloaded

The logic in OnExtensionWillbeInstalled + OnExtensionLoaded seems to be saying
"if it's a new theme, use it". (1)

The logic here says "if the user re-enabled a disabled theme, use it". (2)

Themes have no process, so can't be terminated. (3 is moot)

So... can we replace all three of these methods with:
OnExtensionLoaded() {
  if (is_theme)
    SetTheme()
}
? 

WDYT?

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:762: void
EventRouter::OnExtensionEnabled(content::BrowserContext* browser_context,
Why do we do this for enabled, but not loaded?

[commit-bot: I haz the power, 2017-05-17 20:07:11]

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

[commit-bot: I haz the power, 2017-05-17 20:07:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lazyboy, 2017-05-17 22:23:25]

(answering questions)

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:195: void
OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> Hmm... does this care about enablement?
> 
> OnExtensionLoaded(), I think, can be called from three paths:
> 1. A new extension is added
> 2. A disabled extension is enabled
> 3. A terminated extension is reloaded
> 
> The logic in OnExtensionWillbeInstalled + OnExtensionLoaded seems to be saying
> "if it's a new theme, use it". (1)
> 
> The logic here says "if the user re-enabled a disabled theme, use it". (2)
> 
> Themes have no process, so can't be terminated. (3 is moot)
> 
> So... can we replace all three of these methods with:
> OnExtensionLoaded() {
>   if (is_theme)
>     SetTheme()
> }
> ? 
> 
> WDYT?

Is it possible for a theme to be loaded but it stays disabled for some reason?*
Comment in line 180 seems to be hinting about this, vaguely unfortunately.
In that case we would get OnExtensionLoaded call but won't enable it. (so
setting theme in OnExtensionLoaded would be incorrect).
So if * doesn't happen then the you suggestions sound good!

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:762: void
EventRouter::OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> Why do we do this for enabled, but not loaded?

This seems to be fixing
https://bugs.chromium.org/p/chromium/issues/detail?id=163379 by
https://chromiumcodereview.appspot.com/11348301.

The flow I've understood is:
v1 extension w/ event page does not have foo listener.
update to v2 extension with increased permission that has foo listener.
v2 is disabled due to permission increase.
Enable v2, OnExtensionLoaded does not run the event page. So we don't have any
clue that foo listener was ever registered.

[Devlin, 2017-05-18 01:34:45]

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:195: void
OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/17 22:23:25, lazyboy wrote:
> On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> > Hmm... does this care about enablement?
> > 
> > OnExtensionLoaded(), I think, can be called from three paths:
> > 1. A new extension is added
> > 2. A disabled extension is enabled
> > 3. A terminated extension is reloaded
> > 
> > The logic in OnExtensionWillbeInstalled + OnExtensionLoaded seems to be
saying
> > "if it's a new theme, use it". (1)
> > 
> > The logic here says "if the user re-enabled a disabled theme, use it". (2)
> > 
> > Themes have no process, so can't be terminated. (3 is moot)
> > 
> > So... can we replace all three of these methods with:
> > OnExtensionLoaded() {
> >   if (is_theme)
> >     SetTheme()
> > }
> > ? 
> > 
> > WDYT?
> 
> Is it possible for a theme to be loaded but it stays disabled for some
reason?*
> Comment in line 180 seems to be hinting about this, vaguely unfortunately.
> In that case we would get OnExtensionLoaded call but won't enable it. (so
> setting theme in OnExtensionLoaded would be incorrect).
> So if * doesn't happen then the you suggestions sound good!

I don't know how it could be.  If it's disabled, we won't call
OnExtensionLoaded().  There *might* be a way it could be installed disabled, but
then it will never be loaded.

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:762: void
EventRouter::OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/17 22:23:25, lazyboy wrote:
> On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> > Why do we do this for enabled, but not loaded?
> 
> This seems to be fixing
> https://bugs.chromium.org/p/chromium/issues/detail?id=163379 by
> https://chromiumcodereview.appspot.com/11348301.
> 
> The flow I've understood is:
> v1 extension w/ event page does not have foo listener.
> update to v2 extension with increased permission that has foo listener.
> v2 is disabled due to permission increase.
> Enable v2, OnExtensionLoaded does not run the event page. So we don't have any
> clue that foo listener was ever registered.

Ah, thanks for digging that up!  The motivation here now makes sense.  But, I'm
not convinced it's the right thing to do ;)

Why isn't this a problem for the flow of v1 -> v2, with no increased
permissions?  v2 could still have new events that need registering, so when do
we register those?  Why can't we do this in a similar location?  And, does this
mean there might be other underlying bugs with extension upgrade flow?  If we're
not going through the same path, perhaps we're also not dispatching events like
runtime.onInstalled() (which is also called for installing a new version).

[lazyboy, 2017-05-18 18:10:52]

lazyboy@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[lazyboy, 2017-05-18 18:10:52]

@pkotwicz, can you check + review the cleanup/claim in theme_service.cc?
Specifically OnExtensionLoaded(), thanks.

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_service.cc:195: void
OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/18 01:34:45, Devlin (catching up) wrote:
> On 2017/05/17 22:23:25, lazyboy wrote:
> > On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> > > Hmm... does this care about enablement?
> > > 
> > > OnExtensionLoaded(), I think, can be called from three paths:
> > > 1. A new extension is added
> > > 2. A disabled extension is enabled
> > > 3. A terminated extension is reloaded
> > > 
> > > The logic in OnExtensionWillbeInstalled + OnExtensionLoaded seems to be
> saying
> > > "if it's a new theme, use it". (1)
> > > 
> > > The logic here says "if the user re-enabled a disabled theme, use it". (2)
> > > 
> > > Themes have no process, so can't be terminated. (3 is moot)
> > > 
> > > So... can we replace all three of these methods with:
> > > OnExtensionLoaded() {
> > >   if (is_theme)
> > >     SetTheme()
> > > }
> > > ? 
> > > 
> > > WDYT?
> > 
> > Is it possible for a theme to be loaded but it stays disabled for some
> reason?*
> > Comment in line 180 seems to be hinting about this, vaguely unfortunately.
> > In that case we would get OnExtensionLoaded call but won't enable it. (so
> > setting theme in OnExtensionLoaded would be incorrect).
> > So if * doesn't happen then the you suggestions sound good!
> 
> I don't know how it could be.  If it's disabled, we won't call
> OnExtensionLoaded().  There *might* be a way it could be installed disabled, 
I missed the comments over installed_pending_load_id_ member, it sounds like
it's trying to handle updating a disabled extension case.
So OnExtensionLoad should be sufficient, I'll run it by themes/ OWNERS. Added
comments here.
but
> then it will never be loaded.

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:762: void
EventRouter::OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/18 01:34:45, Devlin (catching up) wrote:
> On 2017/05/17 22:23:25, lazyboy wrote:
> > On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> > > Why do we do this for enabled, but not loaded?
> > 
> > This seems to be fixing
> > https://bugs.chromium.org/p/chromium/issues/detail?id=163379 by
> > https://chromiumcodereview.appspot.com/11348301.
> > 
> > The flow I've understood is:
> > v1 extension w/ event page does not have foo listener.
> > update to v2 extension with increased permission that has foo listener.
> > v2 is disabled due to permission increase.
> > Enable v2, OnExtensionLoaded does not run the event page. So we don't have
any
> > clue that foo listener was ever registered.
> 
> Ah, thanks for digging that up!  The motivation here now makes sense.  But,
I'm
> not convinced it's the right thing to do ;)
> 
> Why isn't this a problem for the flow of v1 -> v2, with no increased
> permissions?  v2 could still have new events that need registering, so when do
> we register those?

We forcefully send an onInstalled in that case via
DispatchEventWithLazyListener:
https://cs.chromium.org/chromium/src/extensions/browser/api/runtime/runtime_a...
Note that it is using a combination of
OnExtensionWillBeInstalled + remember in pref + OnExtensionLoaded

So the event page will run and get all the new events.
  Why can't we do this in a similar location? 
Good point, suggest how that would look like? We definitely don't want to put
this logic
in RuntimeAPI.

 And, does this
> mean there might be other underlying bugs with extension upgrade flow?  If
we're
> not going through the same path, perhaps we're also not dispatching events
like
> runtime.onInstalled() (which is also called for installing a new version).

[Devlin, 2017-05-18 18:39:47]

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2893693002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.cc:762: void
EventRouter::OnExtensionEnabled(content::BrowserContext* browser_context,
On 2017/05/18 18:10:52, lazyboy wrote:
> On 2017/05/18 01:34:45, Devlin (catching up) wrote:
> > On 2017/05/17 22:23:25, lazyboy wrote:
> > > On 2017/05/17 18:37:48, Devlin (catching up) wrote:
> > > > Why do we do this for enabled, but not loaded?
> > > 
> > > This seems to be fixing
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=163379 by
> > > https://chromiumcodereview.appspot.com/11348301.
> > > 
> > > The flow I've understood is:
> > > v1 extension w/ event page does not have foo listener.
> > > update to v2 extension with increased permission that has foo listener.
> > > v2 is disabled due to permission increase.
> > > Enable v2, OnExtensionLoaded does not run the event page. So we don't have
> any
> > > clue that foo listener was ever registered.
> > 
> > Ah, thanks for digging that up!  The motivation here now makes sense.  But,
> I'm
> > not convinced it's the right thing to do ;)
> > 
> > Why isn't this a problem for the flow of v1 -> v2, with no increased
> > permissions?  v2 could still have new events that need registering, so when
do
> > we register those?
> 
> We forcefully send an onInstalled in that case via
> DispatchEventWithLazyListener:
>
https://cs.chromium.org/chromium/src/extensions/browser/api/runtime/runtime_a...
> Note that it is using a combination of
> OnExtensionWillBeInstalled + remember in pref + OnExtensionLoaded
> 
> So the event page will run and get all the new events.
>   Why can't we do this in a similar location? 
> Good point, suggest how that would look like? We definitely don't want to put
> this logic
> in RuntimeAPI.
> 
>  And, does this
> > mean there might be other underlying bugs with extension upgrade flow?  If
> we're
> > not going through the same path, perhaps we're also not dispatching events
> like
> > runtime.onInstalled() (which is also called for installing a new version).
> 

Oof, this is quite the rat's nest.

So, RuntimeAPI uses DispatchEventWithLazyListener(), which
//  Dispatches |event| to the given extension as if the extension has a lazy
// listener for it.

That's very odd.  (What's more, it might render this code unnecessary, since
that will load the background page whenever an update happens...)

At a high level, my proposal would be that when a new version of an extension is
loaded (which could be either initial installation, or update, or
update->disable->enable), we load the background page and register events. 
After that, we should have an accurate accounting of all events that are
registered, and APIs could dispatch normally.

Now, all that said, that will turn this into a very different CL.  I'd like to
do it at some point, and we should add TODOs or a bug for it, but I totally
understand if we want to hold off on that refactor for now (in which case the
general approach of this CL is probably fine).  Lemme know what you'd like to
do.

[pkotwicz, 2017-05-19 18:51:50]

I'll take a look at the CL by Tuesday. Sorry for the delay

[lazyboy, 2017-05-20 03:18:42]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 03:19:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-20 03:36:00]

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

[commit-bot: I haz the power, 2017-05-20 03:36:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lazyboy, 2017-05-23 01:55:35]

Description was changed from

==========
Remove/replace NOTIFICATION_EXTENSION_ENABLED.

Add ExtensionRegistryObserver method OnExtensionEnabled instead.
This is one baby step towards cleaning up EventRouter code (in terms of code
lines, but not really a logical cleanup).

BUG=723754, 411569
Test=No visible changes expected.
==========

to

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, we also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.

BUG=723754, 411569
Test=No visible changes expected.
==========

[lazyboy, 2017-05-23 02:01:18]

Please take another look.
I've discovered a devtools reattachment issue via
ExtensionLoadingTest.KeepAliveWithDevToolsOpenOnReload
test failure, it seems the Enable notification was responsible for reattaching
devtools to an event page... very confusing. I've moved that logic to
extension_service, which already knows about orphaned devtools on reload. Hope
this doesn't break any other things..

[Devlin, 2017-05-23 21:30:37]

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:103: ExtensionService* service() {
There's already an extension_service() accessor on ExtensionBrowserTest

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:188:
IN_PROC_BROWSER_TEST_F(EventsApiTest, NewlyIntroducedListener) {
I think this test would be a bit more interesting if the original version didn't
have any lazy listeners.  That way, we can make sure that we still give the
extension a chance to register new listeners on update no matter what, rather
than leaving the possibility that the new listener only registered when the
extension was woken up for some other reason.

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:201:
InstallExtension(data[0].crx_path, expected_change);
Maybe do InstallExtensionWithPermissionsGranted()?  (Maybe unnecessary with
other comment)

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

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2569: if
(extensions::BackgroundInfo::HasLazyBackgroundPage(extension)) {
Add a comment explaining this.

https://codereview.chromium.org/2893693002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/lazy_events/new_event_in_new_version/v1/manifest.json
(right):

https://codereview.chromium.org/2893693002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/lazy_events/new_event_in_new_version/v1/manifest.json:5:
"permissions": ["tabs"],
You actually don't need the tabs permission to listen to tabs.onCreated.

https://codereview.chromium.org/2893693002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/lazy_events/new_event_in_new_version/v1/manifest.json:6:
"update_url": "http://localhost/autoupdate/updates.xml"
do we need this?

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatch_util.cc (right):

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatch_util.cc:110: new
base::DictionaryValue());
MakeUnique

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/exte...
extensions/browser/extension_registry.h:212:
std::unique_ptr<LazyEventDispatchUtil> lazy_event_dispatch_util_;
I think this would be better placed on EventRouter.

[lazyboy, 2017-05-23 23:19:08]

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:103: ExtensionService* service() {
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> There's already an extension_service() accessor on ExtensionBrowserTest

Removed.

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:188:
IN_PROC_BROWSER_TEST_F(EventsApiTest, NewlyIntroducedListener) {
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> I think this test would be a bit more interesting if the original version
didn't
> have any lazy listeners.  That way, we can make sure that we still give the
> extension a chance to register new listeners on update no matter what, rather
> than leaving the possibility that the new listener only registered when the
> extension was woken up for some other reason.

SG, done.

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:201:
InstallExtension(data[0].crx_path, expected_change);
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> Maybe do InstallExtensionWithPermissionsGranted()?  (Maybe unnecessary with
> other comment)

Unnecessary due to other comment.

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

https://codereview.chromium.org/2893693002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:2569: if
(extensions::BackgroundInfo::HasLazyBackgroundPage(extension)) {
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> Add a comment explaining this.

Done.

https://codereview.chromium.org/2893693002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/lazy_events/new_event_in_new_version/v1/manifest.json
(right):

https://codereview.chromium.org/2893693002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/lazy_events/new_event_in_new_version/v1/manifest.json:5:
"permissions": ["tabs"],
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> You actually don't need the tabs permission to listen to tabs.onCreated.

Done.

https://codereview.chromium.org/2893693002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/lazy_events/new_event_in_new_version/v1/manifest.json:6:
"update_url": "http://localhost/autoupdate/updates.xml"
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> do we need this?

Nope, copied from somewhere that had it. Removed.

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatch_util.cc (right):

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatch_util.cc:110: new
base::DictionaryValue());
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> MakeUnique

Done.

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/2893693002/diff/60001/extensions/browser/exte...
extensions/browser/extension_registry.h:212:
std::unique_ptr<LazyEventDispatchUtil> lazy_event_dispatch_util_;
On 2017/05/23 21:30:36, Devlin (catching up) wrote:
> I think this would be better placed on EventRouter.

Done.

[pkotwicz, 2017-05-25 16:13:37]

I am very sorry for the delay.

There is the notion of enabling/disabling extensions in theme_service.cc because
it is possible for multiple themes to be installed when the "Installed Theme"
infobar is visible. The goal of https://chromiumcodereview.appspot.com/22755003
was to allow a user to switch back to a previously installed theme without
having to refresh the "Chrome Web Store". The "Chrome Web Store" never updated
their "theme install UI" to treat "uninstalled themes" and "disabled themes" the
same though there is an awkward way of "enabling a disabled theme"

It looks like enabling an extension calls OnExtensionLoaded() so things should
continue working with your CL. I need to spend a few hours checking interactions
with various pieces of extensions such as "external extensions"

[pkotwicz, 2017-05-25 20:52:25]

I have not looked through all of the extensions interactions. I think your code
is ok. Your CL does not seem to introduce any bad interactions with externally
loaded themes. (https://developer.chrome.com/extensions/external_extensions)

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

https://codereview.chromium.org/2893693002/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:176: const extensions::Extension*
extension) override {
On startup, OnExtensionLoaded() is not guaranteed to be called prior to the
NOTIFICATION_EXTENSIONS_READY_DEPRECATED notification being sent.

In particular, ThemeService::OnExtensionLoaded() is called with an extension
with id "oedeeodfidgoollimchfdnbmhcpnklnd" after
NOTIFICATION_EXTENSIONS_READY_DEPRECATED is sent on CrOS

I think that this is why I chose to set the theme as a result of
NOTIFICATION_EXTENSION_ENABLED instead of from OnExtensionLoaded()

If we call ThemeService::SetTheme() from OnExtensionLoaded() we should early
return if the theme is already set.

[Devlin, 2017-05-25 21:08:45]

lgtm as long as the bots are happy!

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/api/...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/api/...
extensions/browser/api/runtime/runtime_api.cc:213:
EventRouter::Get(browser_context_)
Add this as a dependency, and remove the observer in the destructor

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatch_util.cc (right):

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatch_util.cc:114:
previous_version.IsValid() ? previous_version.GetString() : "");
nit: prefer std::string() over ""

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatch_util.h (right):

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatch_util.h:41: //
ExtensionRegistryObserver::
s/::/:

[lazyboy, 2017-05-30 23:35:04]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 23:35:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-05-30 23:38:36]

@pkotwicz, can you take another look?

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

https://codereview.chromium.org/2893693002/diff/80001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:176: const extensions::Extension*
extension) override {
On 2017/05/25 20:52:25, pkotwicz wrote:
> On startup, OnExtensionLoaded() is not guaranteed to be called prior to the
> NOTIFICATION_EXTENSIONS_READY_DEPRECATED notification being sent.
> 
> In particular, ThemeService::OnExtensionLoaded() is called with an extension
> with id "oedeeodfidgoollimchfdnbmhcpnklnd" after
> NOTIFICATION_EXTENSIONS_READY_DEPRECATED is sent on CrOS
> 
> I think that this is why I chose to set the theme as a result of
> NOTIFICATION_EXTENSION_ENABLED instead of from OnExtensionLoaded()
> 
> If we call ThemeService::SetTheme() from OnExtensionLoaded() we should early
> return if the theme is already set.

I've added the exception to bail out if extension->id() is the same as current
theme id.

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/api/...
File extensions/browser/api/runtime/runtime_api.cc (right):

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/api/...
extensions/browser/api/runtime/runtime_api.cc:213:
EventRouter::Get(browser_context_)
On 2017/05/25 21:08:44, Devlin wrote:
> Add this as a dependency, and remove the observer in the destructor

Done.

The observer is removed in ::Shutdown as we can't do this in the destructor.
(EventRouter::Get triggers a BrowserContextKeyedAPI get during
DestroyContextServices).

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatch_util.cc (right):

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatch_util.cc:114:
previous_version.IsValid() ? previous_version.GetString() : "");
On 2017/05/25 21:08:44, Devlin wrote:
> nit: prefer std::string() over ""

Done.

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
File extensions/browser/events/lazy_event_dispatch_util.h (right):

https://codereview.chromium.org/2893693002/diff/80001/extensions/browser/even...
extensions/browser/events/lazy_event_dispatch_util.h:41: //
ExtensionRegistryObserver::
On 2017/05/25 21:08:44, Devlin wrote:
> s/::/:

Done.

[commit-bot: I haz the power, 2017-05-31 00:09:19]

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

[commit-bot: I haz the power, 2017-05-31 00:09:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pkotwicz, 2017-05-31 15:46:53]

LGTM with nit

https://codereview.chromium.org/2893693002/diff/100001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/100001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:181: // out.
Remove the comment because it is incorrect. ThemeService::SetTheme() should be
called when a theme installed not each time that Chrome is launched.

[lazyboy, 2017-05-31 21:06:05]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 21:07:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-05-31 21:07:41]

https://codereview.chromium.org/2893693002/diff/100001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/100001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:181: // out.
On 2017/05/31 15:46:53, pkotwicz wrote:
> Remove the comment because it is incorrect. ThemeService::SetTheme() should be
> called when a theme installed not each time that Chrome is launched.

Done.

Also, as discussed offline, I have revived OnExtensionWillBeInstalled +
installed_pending_load_id_ to detect theme update. Now we can detect whether
|extension| is an updated theme or not, then we set the theme if it is an
updated theme. Otherwise, your suggestions applies: do not SetTheme if it is an
existing theme.
PTAL.

[lazyboy, 2017-05-31 21:10:54]

Description was changed from

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, we also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.

BUG=723754, 411569
Test=No visible changes expected.
==========

to

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, we also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.

BUG=723754, 411569
Test=No visible changes expected.
==========

[pkotwicz, 2017-05-31 21:53:36]

LGTM assuming that you have tested an auto updating theme.
https://developer.chrome.com/extensions/autoupdate I don't know whether auto
updating themes exist in the wild

[pkotwicz, 2017-05-31 21:54:06]

LGTM assuming that you have tested an auto updating theme.
https://developer.chrome.com/extensions/autoupdate I don't know whether auto
updating themes exist in the wild

https://codereview.chromium.org/2893693002/diff/120001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:196: // Do not load existing theme.
Perhaps clarify in the comment that SetTheme() should be called when a disabled
theme extension is enabled.

[lazyboy, 2017-05-31 23:31:56]

re: assuming that you have tested an auto updating theme
I've tested auto update path, this is when the webstore has a new version of
theme and it seems to be installing correctly, OnExtensionWillBeInstalled gets
called with correct is_update = true bit.

https://codereview.chromium.org/2893693002/diff/120001/chrome/browser/themes/...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2893693002/diff/120001/chrome/browser/themes/...
chrome/browser/themes/theme_service.cc:196: // Do not load existing theme.
On 2017/05/31 21:54:06, pkotwicz wrote:
> Perhaps clarify in the comment that SetTheme() should be called when a
disabled
> theme extension is enabled.

I've removed the comment on line 190, added comment for disabled theme
enablement, also simplified comments to avoid confusion.

[lazyboy, 2017-05-31 23:32:04]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 23:32:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 01:18:42]

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

[commit-bot: I haz the power, 2017-06-01 01:18:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2017-06-01 18:44:40]

I just tested the auto update code path and it works!

Thank you for clarifying the comment

[lazyboy, 2017-06-01 21:47:19]

Description was changed from

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, we also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.

BUG=723754, 411569
Test=No visible changes expected.
==========

to

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, there are also 2 other reasons for EventRouter code in b):
b1) We also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.
b2) A component extension reload does not go through
ExtensionRegistry::TriggerOnInstalled, so it doesn't see any
OnExtensionWillBeInstalled. Couple the logic with b1).

BUG=723754, 411569
Test=No visible changes expected.
==========

[lazyboy, 2017-06-01 21:50:05]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 21:50:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-06-01 23:32:54]

@Devlin, can you take another look?

Changes required further review: Patch Set 8 -> 9
1. See comment in extension_service.cc below
2. Changed app_browsertest.cc to add the test reloading component app in
chrome/test/data/extensions/platform_apps/component_reload/*

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
This turns out to be interesting :)
Reloading component extension goes through a different code path and it won't
see OnExtensionWillBeInstalled when the extension is reloaded. I've tied the
logic of orphaned devtools with this.

[commit-bot: I haz the power, 2017-06-01 23:53:41]

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

[commit-bot: I haz the power, 2017-06-01 23:53:42]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-02 01:57:28]

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/apps/ap...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/apps/ap...
chrome/browser/apps/app_browsertest.cc:958: // Now tell the app to reload itself
nittiest of nits: end in a '.'

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
On 2017/06/01 23:32:54, lazyboy wrote:
> This turns out to be interesting :)
> Reloading component extension goes through a different code path and it won't
> see OnExtensionWillBeInstalled when the extension is reloaded. I've tied the
> logic of orphaned devtools with this.

Fun!  So was this also a bug before?

This makes me wonder if this should be part of the LazyEventDispatchUtil, though
- wouldn't this be a problem for anything else that wants to run when an
extension is updated?  Is there a complication with moving this bit there?

https://codereview.chromium.org/2893693002/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/component_reload/background.js
(right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/component_reload/background.js:6: if
(response == "reload")
nit: do we expect any other response?  If not, maybe chrome.test.assertEq()?
nit: single-quotes in JS.

[lazyboy, 2017-06-02 18:35:37]

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/apps/ap...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/apps/ap...
chrome/browser/apps/app_browsertest.cc:958: // Now tell the app to reload itself
On 2017/06/02 01:57:28, Devlin wrote:
> nittiest of nits: end in a '.'

Done.

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
On 2017/06/02 01:57:28, Devlin wrote:
> On 2017/06/01 23:32:54, lazyboy wrote:
> > This turns out to be interesting :)
> > Reloading component extension goes through a different code path and it
won't
> > see OnExtensionWillBeInstalled when the extension is reloaded. I've tied the
> > logic of orphaned devtools with this.
> 
> Fun!  So was this also a bug before?
It wasn't, because we used to call AddPendingTask on
NOTIFICATION_EXTENSION_ENABLED notification from EventRouter.
DriveFileSystemExtensionApiTest.RetainEntry luckily caught this.
> 
> This makes me wonder if this should be part of the LazyEventDispatchUtil,
though
> - wouldn't this be a problem for anything else that wants to run when an
> extension is updated?  Is there a complication with moving this bit there?
I thought of putting the component extension logic in LazyEventDispatchUtil,
though we need to expose a public method in LazyEventDispatchUtil and call that
from here [1].
However, combining orphaned devtools isn't trivial, so I kept the change to
component extension here along with devtools logic. I can certainly do [1]
(which means we might call additional AddPendingTask if devtools is open, not a
big deal), let me know.

https://codereview.chromium.org/2893693002/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/component_reload/background.js
(right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/component_reload/background.js:6: if
(response == "reload")
On 2017/06/02 01:57:28, Devlin wrote:
> nit: do we expect any other response?  If not, maybe chrome.test.assertEq()?
> nit: single-quotes in JS.

Done.

[lazyboy, 2017-06-02 22:38:29]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 22:39:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 22:42:15]

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

[commit-bot: I haz the power, 2017-06-02 22:42:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lazyboy, 2017-06-02 23:42:40]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 23:43:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-03 01:41:02]

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

[commit-bot: I haz the power, 2017-06-03 01:41:04]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-03 02:10:12]

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
On 2017/06/02 18:35:37, lazyboy wrote:
> On 2017/06/02 01:57:28, Devlin wrote:
> > On 2017/06/01 23:32:54, lazyboy wrote:
> > > This turns out to be interesting :)
> > > Reloading component extension goes through a different code path and it
> won't
> > > see OnExtensionWillBeInstalled when the extension is reloaded. I've tied
the
> > > logic of orphaned devtools with this.
> > 
> > Fun!  So was this also a bug before?
> It wasn't, because we used to call AddPendingTask on
> NOTIFICATION_EXTENSION_ENABLED notification from EventRouter.
> DriveFileSystemExtensionApiTest.RetainEntry luckily caught this.
> > 
> > This makes me wonder if this should be part of the LazyEventDispatchUtil,
> though
> > - wouldn't this be a problem for anything else that wants to run when an
> > extension is updated?  Is there a complication with moving this bit there?
> I thought of putting the component extension logic in LazyEventDispatchUtil,
> though we need to expose a public method in LazyEventDispatchUtil and call
that
> from here [1].
Hmm, would we need to call it from here?  Couldn't the lazy event dispatch util
listen for OnExtensionLoaded +
Manifest::IsComponentLocation(extension->location())?

> However, combining orphaned devtools isn't trivial, so I kept the change to
> component extension here along with devtools logic. I can certainly do [1]
> (which means we might call additional AddPendingTask if devtools is open, not
a
> big deal), let me know.

Correct me if I'm wrong, but calling multiple times should essentially be a
no-op, right?  Since the background page will already be active, and we don't do
anything with it.

[lazyboy, 2017-06-03 02:18:52]

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
On 2017/06/03 02:10:12, Devlin wrote:
> On 2017/06/02 18:35:37, lazyboy wrote:
> > On 2017/06/02 01:57:28, Devlin wrote:
> > > On 2017/06/01 23:32:54, lazyboy wrote:
> > > > This turns out to be interesting :)
> > > > Reloading component extension goes through a different code path and it
> > won't
> > > > see OnExtensionWillBeInstalled when the extension is reloaded. I've tied
> the
> > > > logic of orphaned devtools with this.
> > > 
> > > Fun!  So was this also a bug before?
> > It wasn't, because we used to call AddPendingTask on
> > NOTIFICATION_EXTENSION_ENABLED notification from EventRouter.
> > DriveFileSystemExtensionApiTest.RetainEntry luckily caught this.
> > > 
> > > This makes me wonder if this should be part of the LazyEventDispatchUtil,
> > though
> > > - wouldn't this be a problem for anything else that wants to run when an
> > > extension is updated?  Is there a complication with moving this bit there?
> > I thought of putting the component extension logic in LazyEventDispatchUtil,
> > though we need to expose a public method in LazyEventDispatchUtil and call
> that
> > from here [1].
> Hmm, would we need to call it from here?  Couldn't the lazy event dispatch
util
> listen for OnExtensionLoaded +
> Manifest::IsComponentLocation(extension->location())?
OnExtensionLoaded can result from line 1583 as well (AddExtension), this one is
from EnableExtension. So just looking at OnExtensionLoaded wouldn't give us the
exact same behavior.
> 
> > However, combining orphaned devtools isn't trivial, so I kept the change to
> > component extension here along with devtools logic. I can certainly do [1]
> > (which means we might call additional AddPendingTask if devtools is open,
not
> a
> > big deal), let me know.
> 
> Correct me if I'm wrong, but calling multiple times should essentially be a
> no-op, right?  
That's right.
Since the background page will already be active, and we don't do
> anything with it.

[Devlin, 2017-06-05 15:38:38]

lgtm

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
On 2017/06/03 02:18:52, lazyboy wrote:
> On 2017/06/03 02:10:12, Devlin wrote:
> > Hmm, would we need to call it from here?  Couldn't the lazy event dispatch
> util
> > listen for OnExtensionLoaded +
> > Manifest::IsComponentLocation(extension->location())?
> OnExtensionLoaded can result from line 1583 as well (AddExtension), this one
is
> from EnableExtension. So just looking at OnExtensionLoaded wouldn't give us
the
> exact same behavior.

Oh, I see.  Drat.  This is fine, then.

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/api...
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/api...
extensions/browser/api/runtime/runtime_api.h:105: //
LazyEventDispatchUtil::Observer::
nit: s/Observer::/Observer:

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/eve...
File extensions/browser/events/lazy_event_dispatch_util.h (right):

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/eve...
extensions/browser/events/lazy_event_dispatch_util.h:19: class
LazyEventDispatchUtil : public ExtensionRegistryObserver {
class comments.  In particular, it might be nice to note how this tracks
updates/installs/etc (with pref entries)

[lazyboy, 2017-06-05 21:53:49]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[lazyboy, 2017-06-05 21:54:01]

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/2893693002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service.cc:2570: // Reloading component
extension does not trigger install, so RuntimeAPI won't
On 2017/06/05 15:38:38, Devlin wrote:
> On 2017/06/03 02:18:52, lazyboy wrote:
> > On 2017/06/03 02:10:12, Devlin wrote:
> > > Hmm, would we need to call it from here?  Couldn't the lazy event dispatch
> > util
> > > listen for OnExtensionLoaded +
> > > Manifest::IsComponentLocation(extension->location())?
> > OnExtensionLoaded can result from line 1583 as well (AddExtension), this one
> is
> > from EnableExtension. So just looking at OnExtensionLoaded wouldn't give us
> the
> > exact same behavior.
> 
> Oh, I see.  Drat.  This is fine, then.

Acknowledged.

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/api...
File extensions/browser/api/runtime/runtime_api.h (right):

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/api...
extensions/browser/api/runtime/runtime_api.h:105: //
LazyEventDispatchUtil::Observer::
On 2017/06/05 15:38:38, Devlin wrote:
> nit: s/Observer::/Observer:

Done.

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/eve...
File extensions/browser/events/lazy_event_dispatch_util.h (right):

https://codereview.chromium.org/2893693002/diff/200001/extensions/browser/eve...
extensions/browser/events/lazy_event_dispatch_util.h:19: class
LazyEventDispatchUtil : public ExtensionRegistryObserver {
On 2017/06/05 15:38:38, Devlin wrote:
> class comments.  In particular, it might be nice to note how this tracks
> updates/installs/etc (with pref entries)

Done.

[commit-bot: I haz the power, 2017-06-05 21:54:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 02:49:39]

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

[commit-bot: I haz the power, 2017-06-06 02:49:40]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-06 17:52:14]

Description was changed from

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, there are also 2 other reasons for EventRouter code in b):
b1) We also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.
b2) A component extension reload does not go through
ExtensionRegistry::TriggerOnInstalled, so it doesn't see any
OnExtensionWillBeInstalled. Couple the logic with b1).

BUG=723754, 411569
Test=No visible changes expected.
==========

to

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, there are also 2 other reasons for EventRouter code in b):
b1) We also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.
b2) A component extension reload does not go through
ExtensionRegistry::TriggerOnInstalled, so it doesn't see any
OnExtensionWillBeInstalled. Couple the logic with b1).

TBR=benwells@chromium.org for added test in app_browsertest.cc
BUG=723754, 411569
Test=No visible changes expected.
==========

[lazyboy, 2017-06-06 17:53:12]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 17:53:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 17:58:47]

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

[commit-bot: I haz the power, 2017-06-06 17:58:48]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-06 18:50:55]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-06-06 18:50:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2893693002/#ps220001
(title: "address comments")

[commit-bot: I haz the power, 2017-06-06 18:52:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 18:57:01]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496775055614690,
"parent_rev": "3a7d55be3d2245f52c60b892c83ea17f62766b7c", "commit_rev":
"75b9def1648c17c0edc926e7ea787519e9c59899"}

[commit-bot: I haz the power, 2017-06-06 18:57:12]

Description was changed from

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, there are also 2 other reasons for EventRouter code in b):
b1) We also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.
b2) A component extension reload does not go through
ExtensionRegistry::TriggerOnInstalled, so it doesn't see any
OnExtensionWillBeInstalled. Couple the logic with b1).

TBR=benwells@chromium.org for added test in app_browsertest.cc
BUG=723754, 411569
Test=No visible changes expected.
==========

to

==========
Remove NOTIFICATION_EXTENSION_ENABLED.

Enablement can happen for the following 3 cases:
1. New extension is added
2. User enables a disabled extension
3. An extension is reloaded or a terminated extension is reloaded

It seems we used this in two classes in
a) theme_service.cc and
b) event_router.cc

a) For themes, this can be simplified to load the theme on
OnExtensionLoaded. Themes don't have process and it can't be
terminated. We still have to remember whether the load is due
to a theme update or not (OnExtensionWillBeInstalled). It is
possible to avoid this by remembering both current theme's
id (already done) and version in the prefs. This CL does not do
that.

b) The primary reason of EventRouter code is to make sure we
are aware of new lazy events on extension upgrade, and to dispatch
onInstalled event. The CL wraps the logic to use
OnExtensionWillBeInstalled + OnExtensionLoaded into a separate
class: LazyEventDispatchUtil.

However, there are also 2 other reasons for EventRouter code in b):
b1) We also need to reconnect devtools to a reloaded event
page. That was happening through event_router. Pull that logic into
extension_service as extension_service is already responsible
for remembering orphaned devtools.
b2) A component extension reload does not go through
ExtensionRegistry::TriggerOnInstalled, so it doesn't see any
OnExtensionWillBeInstalled. Couple the logic with b1).

TBR=benwells@chromium.org for added test in app_browsertest.cc
BUG=723754, 411569
Test=No visible changes expected.

Review-Url: https://codereview.chromium.org/2893693002
Cr-Commit-Position: refs/heads/master@{#477357}
Committed:
https://chromium.googlesource.com/chromium/src/+/75b9def1648c17c0edc926e7ea78...
==========

[commit-bot: I haz the power, 2017-06-06 18:57:13]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/75b9def1648c17c0edc926e7ea78...