Invoke callback from WebstoreInstaller when installation is fully complete

Invoke callback from WebstoreInstaller when installation is fully complete

ExtensionRegistry observers will be notified when an extension install
is fully complete, i.e. after the extension has been loaded or added
to one of the registry extension sets.

This allows the WebstoreInstaller completion to be delayed until the
extension has been loaded. Workarounds in EphemeralAppLauncher for
launching an app can now be cleaned up since we are now assured that
the app has been loaded.

BUG=None
TEST=browser_tests

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

[tmdiep, 2014-06-11 03:42:14]

This patch cleans up a hack in EphemeralAppLauncher. This class inherits from
WebstoreStandaloneInstaller and launches the app when install is complete. The
problem was that CompleteInstall() occurred before the app was loaded in
extension system.  This patch ensures that CompleteInstall() occurs after the
extension has been both installed and loaded.

I believe c/b/e/WebstoreInstaller and implementators of
WebstoreInstaller::Delegate::OnExtensionInstallSuccess() won't break as a result
of this change. But I may have missed something.

Thanks in advance.

[asargent_no_longer_on_chrome, 2014-06-11 18:27:54]

lgtm

Are there already some tests that exercise the install/launch of ephemeral apps?
If not, it would be good to add some.

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_registry.cc (right):

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
extensions/browser/extension_registry.cc:71:
DCHECK(GenerateInstalledExtensionsSet()->Contains(extension->id()));
I forget, do the bodies of DCHECK's get completely elided in the build process
if DCHECK's are turned off? If not, you might want to wrap this in:

#if DCHECK_IS_ON

#endif 

Since GenerateInstalledExtensionsSet does a little bit of work.

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:64: // necessarily enabled.
Would it be helpful to pass the enabled/disabled state as an additional
parameter?

[tmdiep, 2014-06-11 22:56:19]

Thanks for the review.

Tests for ephemeral apps are concentrated in ephemeral_app_browsertest, but
there aren't any specifically for the EphemeralAppLauncher. I'll include tests
for that when implementing the webstore private api functions for launching
ephemeral apps.

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_registry.cc (right):

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
extensions/browser/extension_registry.cc:71:
DCHECK(GenerateInstalledExtensionsSet()->Contains(extension->id()));
On 2014/06/11 18:27:54, Antony Sargent wrote:
> I forget, do the bodies of DCHECK's get completely elided in the build process
> if DCHECK's are turned off? If not, you might want to wrap this in:
> 
> #if DCHECK_IS_ON
> 
> #endif 
> 
> Since GenerateInstalledExtensionsSet does a little bit of work. 

The code doesn't get compiled out completely, but the compiler short circuits
the evaluation so the condition won't execute.

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/326213004/diff/120001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:64: // necessarily enabled.
On 2014/06/11 18:27:54, Antony Sargent wrote:
> Would it be helpful to pass the enabled/disabled state as an additional
> parameter?

I think maybe not for this notification - it's for observers who want to know
when the entire installation process is complete. Observers that do things when
an extension is enabled listen for OnLoaded.

[tmdiep, 2014-06-11 22:56:33]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:58:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/326213004/120001

[asargent_no_longer_on_chrome, 2014-06-11 23:21:05]

> Tests for ephemeral apps are concentrated in ephemeral_app_browsertest, but
> there aren't any specifically for the EphemeralAppLauncher. I'll include tests
> for that when implementing the webstore private api functions for launching
> ephemeral apps.

Ok, sounds good.

[commit-bot: I haz the power, 2014-06-12 00:14:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15755)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18887)

[commit-bot: I haz the power, 2014-06-12 00:15:41]

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

[commit-bot: I haz the power, 2014-06-12 00:15:42]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15809)

[tmdiep, 2014-06-12 11:12:48]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-12 11:17:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/326213004/120001

[commit-bot: I haz the power, 2014-06-12 14:20:06]

Change committed as 276666