Force-load a lazy background page whenever an extension is enabled.

Force-load a lazy background page whenever an extension is enabled.

Additionally this changes the app.runtime.onLaunched event to always be
delivered, even if it isn't known yet if the extension is interested in
that event. This is necesary because trying to launch an app is one of
the ways an extension can be re-enabled, but there isn't an easy way
to wait for the app launching code to wait for the background page to
be loaded before actually trying to launch an app after re-enabling it.

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

[Marijn Kruisselbrink, 2012-11-29 20:10:07]

This works, but I don't really like the changes to the app.runtime.onLaunched
handling. But solving that properly would involve somehow getting the (currently
empty) callback that is passed to LazyBackgroundTaskQueue::AddPendingTask in
ExtensionService::EnableExtension to call some code in AppLauncherHandler (so
passing in a callback to GrantPermissionsAndEnableExtension which is passed on
to EnableExtension? but 99% of the calls to EnableExtension wouldn't really care
about this callback I think).
So not sure if there is a cleaner solution for this.

[Matt Perry, 2012-11-29 20:24:03]

On 2012/11/29 20:10:07, Marijn Kruisselbrink wrote:
> This works, but I don't really like the changes to the app.runtime.onLaunched
> handling. But solving that properly would involve somehow getting the
(currently
> empty) callback that is passed to LazyBackgroundTaskQueue::AddPendingTask in
> ExtensionService::EnableExtension to call some code in AppLauncherHandler (so
> passing in a callback to GrantPermissionsAndEnableExtension which is passed on
> to EnableExtension? but 99% of the calls to EnableExtension wouldn't really
care
> about this callback I think).
> So not sure if there is a cleaner solution for this.

I don't know the app code, but presumably there's some code somewhere that does:
  if (!enabled) EnableApp();
  DispatchOnLaunchEvent();
We could change that to be:
  if (!enabled) {
    EnableApp();
    LazyBgTaskQueue::AddTask(DispatchOnLaunch);
  } else {
    DispatchOnLaunch();
  }

I'm not sure if it's worth the complexity, though. It seems reasonable to force
dispatch of onLaunched.

[Matt Perry, 2012-11-29 20:24:46]

https://codereview.chromium.org/11348301/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/11348301/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:47:
extension->id());
If you go this route, can you move this chunk to a helper function and update
all the call sites?

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

https://codereview.chromium.org/11348301/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:941: // If the extension has a
lazy background page, make sure it gets loaded to
ExtensionService is not the best place for this code. Better to have a system
listen to NOTIFICATION_EXTENSION_ENABLED and do it there. EventRouter seems like
the best fit.

[Marijn Kruisselbrink, 2012-11-29 21:07:19]

On 2012/11/29 20:24:03, Matt Perry wrote:
> I don't know the app code, but presumably there's some code somewhere that
does:
>   if (!enabled) EnableApp();
>   DispatchOnLaunchEvent();
> We could change that to be:
>   if (!enabled) {
>     EnableApp();
>     LazyBgTaskQueue::AddTask(DispatchOnLaunch);
>   } else {
>     DispatchOnLaunch();
>   }
You're right, that would have worked.

> I'm not sure if it's worth the complexity, though. It seems reasonable to
force
> dispatch of onLaunched.
Okay, I'll leave it as is then. 
PTAL

https://codereview.chromium.org/11348301/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/11348301/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:47:
extension->id());
On 2012/11/29 20:24:46, Matt Perry wrote:
> If you go this route, can you move this chunk to a helper function and update
> all the call sites?

Done.

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

https://codereview.chromium.org/11348301/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:941: // If the extension has a
lazy background page, make sure it gets loaded to
On 2012/11/29 20:24:46, Matt Perry wrote:
> ExtensionService is not the best place for this code. Better to have a system
> listen to NOTIFICATION_EXTENSION_ENABLED and do it there. EventRouter seems
like
> the best fit.
Ah yes, of course. Done.

[Matt Perry, 2012-11-29 22:03:21]

lgtm

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:38:
system->event_router()->AddLazyEventListener(kOnLaunchedEvent, extension_id);
add a comment explaining why this is necessary

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/event_router.cc:622: base::Bind(&DoNothing));
Do you actually need to have an empty function? What if you pass
base::Callback()?

[Marijn Kruisselbrink, 2012-11-30 00:51:20]

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:38:
system->event_router()->AddLazyEventListener(kOnLaunchedEvent, extension_id);
On 2012/11/29 22:03:21, Matt Perry wrote:
> add a comment explaining why this is necessary

Done.

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11348301/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/event_router.cc:622: base::Bind(&DoNothing));
On 2012/11/29 22:03:21, Matt Perry wrote:
> Do you actually need to have an empty function? What if you pass
> base::Callback()?
base::Callback() is "null", which means that unless the code invoking it first
checks for is_null, calling Run on it will crash. So yes, unfortunately the
empty function is really needed. (I first tried to just load the background
page, without posting a task at all, but LazyBackgroundTaskQueue crashes if the
background page got loaded with no tasks pending).

[commit-bot: I haz the power, 2012-11-30 00:52:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/11348301/9001

[commit-bot: I haz the power, 2012-11-30 03:54:38]

Change committed as 170389