Optimize promotion of ephemeral apps

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 2390554fa8c0ebb2fe41eab5cf5f01f3bb2e9e48..357f61ad4c0579fc6501ba010ae7a52ca39aabde 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1890,6 +1890,7 @@ void ExtensionService::AddNewOrUpdatedExtension(
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const bool blacklisted_for_malware =
       blacklist_state == extensions::BLACKLISTED_MALWARE;
+  bool was_ephemeral = extension_prefs_->IsEphemeralApp(extension->id());
   extension_prefs_->OnExtensionInstalled(extension,
                                          initial_state,
                                          blacklisted_for_malware,
@@ -1899,7 +1900,7 @@ void ExtensionService::AddNewOrUpdatedExtension(
   delayed_installs_.Remove(extension->id());
   if (InstallVerifier::NeedsVerification(*extension))
     system_->install_verifier()->VerifyExtension(extension->id());
-  FinishInstallation(extension);
+  FinishInstallation(extension, was_ephemeral);
 }
 
 void ExtensionService::MaybeFinishDelayedInstallation(
@@ -1944,13 +1945,15 @@ void ExtensionService::FinishDelayedInstallation(
   CHECK(extension.get());
   delayed_installs_.Remove(extension_id);
 
+  bool was_ephemeral = extension_prefs_->IsEphemeralApp(extension->id());
   if (!extension_prefs_->FinishDelayedInstallInfo(extension_id))

[benwells, 2014/05/27 01:42:56]

This is interesting, looks like for delayed install we save all the new info
separately and then commit it here. Sounds similar to what we want to be doing
for updates.

     NOTREACHED();
 
-  FinishInstallation(extension.get());
+  FinishInstallation(extension.get(), was_ephemeral);
 }
 
-void ExtensionService::FinishInstallation(const Extension* extension) {
+void ExtensionService::FinishInstallation(
+    const Extension* extension, bool was_ephemeral) {
   const extensions::Extension* existing_extension =
       GetInstalledExtension(extension->id());
   bool is_update = false;
@@ -1959,14 +1962,17 @@ void ExtensionService::FinishInstallation(const Extension* extension) {
     is_update = true;
     old_name = existing_extension->name();
   }
-  extensions::InstalledExtensionInfo details(extension, is_update, old_name);
+  bool from_ephemeral =
+      was_ephemeral && !extension_prefs_->IsEphemeralApp(extension->id());
+  extensions::InstalledExtensionInfo details(
+      extension, is_update, from_ephemeral, old_name);
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED,
       content::Source<Profile>(profile_),
       content::Details<const extensions::InstalledExtensionInfo>(&details));
 
-  ExtensionRegistry::Get(profile_)
-      ->TriggerOnWillBeInstalled(extension, is_update, old_name);
+  ExtensionRegistry::Get(profile_)->TriggerOnWillBeInstalled(
+      extension, is_update, from_ephemeral, old_name);
 
   bool unacknowledged_external = IsUnacknowledgedExternalExtension(extension);
 
@@ -2005,6 +2011,67 @@ void ExtensionService::FinishInstallation(const Extension* extension) {
   }
 }
 
+void ExtensionService::InstallEphemeralApp(
+    const extensions::Extension* extension, bool user_acknowledged) {
+  DCHECK(GetInstalledExtension(extension->id()) &&
+         extension_prefs_->IsEphemeralApp(extension->id()));
+
+  if (user_acknowledged) {
+    if (extension->RequiresSortOrdinal()) {

[benwells, 2014/05/27 01:42:56]

Why don't we do this if it wasn't user_acknowledged?

[tmdiep, 2014/05/27 01:58:54]

If the app came from sync, we probably don't want to set any new ordinals, just
wait for it to come from the sync data.

[benwells, 2014/05/27 03:11:11]

The flag is user_acknowledged, not from_sync, so this isn't really clear.

What do you mean by 'probably'? Is there any harm in doing it?

[tmdiep, 2014/05/27 03:21:53]

ChromeAppSorting::Set[Page|AppLaunch]Ordinal() will initiate sync for the
ordinal change. So I suspect that it will end up overwriting the ordinals,
though I didn't manually test to verify.

I'll change 'user_acknowledged' to 'is_from_sync', which is what I'm really
guarding against.

+      // Reset the sort ordinals of the app to ensure it is added to the default
+      // position, like newly installed apps would.
+      extension_prefs_->app_sorting()->ClearOrdinals(extension->id());
+      extension_prefs_->app_sorting()->EnsureValidOrdinals(
+          extension->id(), syncer::StringOrdinal());
+    }
+
+    if (!IsExtensionEnabled(extension->id())) {
+      // If the installation of this app was user acknowledged it can be
+      // re-enabled if a permission increase or user action were the only
+      // reasons.
+      extension_prefs_->RemoveDisableReason(
+          extension->id(),
+          Extension::DISABLE_PERMISSIONS_INCREASE);
+      extension_prefs_->RemoveDisableReason(
+          extension->id(),
+          Extension::DISABLE_USER_ACTION);
+      if (!extension_prefs_->GetDisableReasons(extension->id()))
+        EnableExtension(extension->id());
+    }
+  }
+
+  // Remove the ephemeral flags from the preferences.
+  extension_prefs_->OnEphemeralAppInstalled(extension->id());
+
+  // Fire install-related events to allow observers to handle the promotion
+  // of the ephemeral app.
+  extensions::InstalledExtensionInfo details(
+      extension,
+      true /* is update */,
+      true /* from ephemeral */,
+      extension->name() /* old name */);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_INSTALLED_DEPRECATED,
+      content::Source<Profile>(profile_),
+      content::Details<const extensions::InstalledExtensionInfo>(&details));
+
+  registry_->TriggerOnWillBeInstalled(
+      extension,
+      true /* is update */,
+      true /* from ephemeral */,
+      extension->name() /* old name */);
+
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
+      content::Source<Profile>(profile_),
+      content::Details<const Extension>(extension));
+
+  registry_->TriggerOnLoaded(extension);
+
+  if (user_acknowledged && extension_sync_service_)
+    extension_sync_service_->SyncExtensionChangeIfNeeded(*extension);

[benwells, 2014/05/27 01:42:56]

Have you got any tests to check that it is synced properly?

[tmdiep, 2014/05/27 01:58:54]

I've only tested this manually. I'll look into a browser test for this.

[tmdiep, 2014/05/27 07:46:05]

Done - see EphemeralAppBrowserTests.

+}
+
 const Extension* ExtensionService::GetPendingExtensionUpdate(
     const std::string& id) const {
   return delayed_installs_.GetByID(id);