Make clicking the restart bubble for crashed apps work.

Make clicking the restart bubble for crashed apps work.

Previously AppLoadService listened for
NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING before dispatching the
onRestarted() event after a reload, but in the case of packaged apps, which use
non-persistent background pages, that notification will never get fired. This
is because reloading an app doesn't cause its background page to get loaded -
only a relevant event causes the page to be woken up.

This patch fixes the issue by listening for NOTIFICATION_EXTENSION_LOADED
instead, which is always fired after a reload. It also determines whether an
extension is listening to an event by checking which events it has registered
for, not which ones it currently has a listener for,
NOTIFICATION_EXTENSION_LOADED is the notification that listeners get created
on, and so to avoid raciness we check registered events (ie: the persisted
list of events that an extension is interested in).

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

[tapted, 2013-12-12 06:54:52]

https://codereview.chromium.org/93593003/diff/1/apps/app_load_service.cc
File apps/app_load_service.cc (right):

https://codereview.chromium.org/93593003/diff/1/apps/app_load_service.cc#newc...
apps/app_load_service.cc:50: RestartPlatformApp(profile_,
service->GetExtensionById(extension_id, false));
GetExtensionsById likes to return NULL a lot.. 

I think there's a pathological case for unpacked extensions, where the reload
needs some asynchronous file IO, so GetExtensionById returns NULL immediately
after ReloadExtension.

[koz (OOO until 15th September), 2013-12-13 03:33:56]

https://codereview.chromium.org/93593003/diff/1/apps/app_load_service.cc
File apps/app_load_service.cc (right):

https://codereview.chromium.org/93593003/diff/1/apps/app_load_service.cc#newc...
apps/app_load_service.cc:50: RestartPlatformApp(profile_,
service->GetExtensionById(extension_id, false));
On 2013/12/12 06:54:52, tapted wrote:
> GetExtensionsById likes to return NULL a lot.. 
> 
> I think there's a pathological case for unpacked extensions, where the reload
> needs some asynchronous file IO, so GetExtensionById returns NULL immediately
> after ReloadExtension.

Ah, good spot! I've tried a different approach in the latest patchset - wait for
the NOTIFICATION_EXTENSION_LOADED instead of NOTIFICATION_HOST_DID_STOP_LOADING
because the former always fires for reloads. Doing so means we need to check for
registered events rather than actual listeners. Registered events are events
that an app app should have listeners for, even if the app isn't running, and we
need to check them because they are only converted into real listeners at
NOTIFICATION_EXTENSION_LOADED time, which means we can't assume that the
listeners will be populated in our own LOADED listener.

WDYT?

[tapted, 2013-12-13 04:34:23]

https://codereview.chromium.org/93593003/diff/20001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/93593003/diff/20001/apps/launcher.cc#newcode381
apps/launcher.cc:381: ExtensionHasEventListener(extension->id(),
Should this be updated as well, to be consistent? Although, maybe it's a good
sideffect now that [i think] an extension that has its extension bubble clicked
that has BOTH OnRestarted and OnLaunched will now get an OnLaunched (rather than
OnRestarted only).

But, it's subtle - maybe a comment?

https://codereview.chromium.org/93593003/diff/20001/apps/launcher.cc#newcode393
apps/launcher.cc:393: std::set<std::string> events =
nit: you could avoid the local `events` variable with something like:

bool listening_to_launch = event_router->GetRegisteredEvents(extension->id())
    .count(app_runtime::OnLaunched::kEventName) != 0;

Also maybe a comment like `Read from the events registered in prefs, since
listeners have not yet been created.`. Assuming that's true :) [that's my
reading of the diff anyway].

[koz (OOO until 15th September), 2013-12-16 01:48:59]

https://codereview.chromium.org/93593003/diff/20001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/93593003/diff/20001/apps/launcher.cc#newcode381
apps/launcher.cc:381: ExtensionHasEventListener(extension->id(),
On 2013/12/13 04:34:24, tapted wrote:
> Should this be updated as well, to be consistent? Although, maybe it's a good
> sideffect now that [i think] an extension that has its extension bubble
clicked
> that has BOTH OnRestarted and OnLaunched will now get an OnLaunched (rather
than
> OnRestarted only).
> 
> But, it's subtle - maybe a comment?

Ooh, actually this should be consistent. The effect of it not being consistent
is that this code won't execute at all from the crash bubble, ie: it will only
get onLaunched and not onRestarted, even if it's listening for onRestarted.

https://codereview.chromium.org/93593003/diff/20001/apps/launcher.cc#newcode393
apps/launcher.cc:393: std::set<std::string> events =
On 2013/12/13 04:34:24, tapted wrote:
> nit: you could avoid the local `events` variable with something like:
> 
> bool listening_to_launch = event_router->GetRegisteredEvents(extension->id())
>     .count(app_runtime::OnLaunched::kEventName) != 0;
> 
> Also maybe a comment like `Read from the events registered in prefs, since
> listeners have not yet been created.`. Assuming that's true :) [that's my
> reading of the diff anyway].

Sam has informed me of ContainsKey() which seems like the best of both worlds. I
added the comment above.

[tapted, 2013-12-16 03:23:17]

lgtm with a troll :)

Also, it might be good to note in the CL description why it didn't previously
work, and how this fixes it.

https://codereview.chromium.org/93593003/diff/40001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/93593003/diff/40001/apps/launcher.cc#newcode386
apps/launcher.cc:386: ContainsKey(events,
std::string(app_runtime::OnRestarted::kEventName));
what's wrong with .count()? you should get an implicit conversion to std::string
that way. I think just about every other language has a set::contains(key)
function, but they didn't bother with C++ since it's equivalent to
set::count(key) (with the implicit cast to bool).

Maybe count is too old-school :p.

But also I don't mind either way - I'm really just trolling :p

[koz (OOO until 15th September), 2013-12-16 03:52:35]

https://codereview.chromium.org/93593003/diff/40001/apps/launcher.cc
File apps/launcher.cc (right):

https://codereview.chromium.org/93593003/diff/40001/apps/launcher.cc#newcode386
apps/launcher.cc:386: ContainsKey(events,
std::string(app_runtime::OnRestarted::kEventName));
On 2013/12/16 03:23:17, tapted wrote:
> what's wrong with .count()? you should get an implicit conversion to
std::string
> that way. I think just about every other language has a set::contains(key)
> function, but they didn't bother with C++ since it's equivalent to
> set::count(key) (with the implicit cast to bool).
> 
> Maybe count is too old-school :p.
> 
> But also I don't mind either way - I'm really just trolling :p

Oh, but I'm old-school, too! Sort of. Java is oldschool to me. I like .count(),
especially because we can get the implicit cast on the string, but the implicit
cast with the bool gives a "performance warning", so I've had to compare to
zero.

[commit-bot: I haz the power, 2013-12-16 23:11:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/93593003/60001

[commit-bot: I haz the power, 2013-12-17 06:48:26]

Change committed as 241162