Optimize promotion of ephemeral apps

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 1872 matching lines @@
 void ExtensionService::AddNewOrUpdatedExtension(
     const Extension* extension,
     Extension::State initial_state,
     extensions::BlacklistState blacklist_state,
     bool is_ephemeral,
     const syncer::StringOrdinal& page_ordinal,
     const std::string& install_parameter) {
   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,
                                          is_ephemeral,
                                          page_ordinal,
                                          install_parameter);
   delayed_installs_.Remove(extension->id());
   if (InstallVerifier::NeedsVerification(*extension))
     system_->install_verifier()->VerifyExtension(extension->id());
-  FinishInstallation(extension);
+  FinishInstallation(extension, was_ephemeral);
 }
 
 void ExtensionService::MaybeFinishDelayedInstallation(
     const std::string& extension_id) {
   // Check if the extension already got installed.
   if (!delayed_installs_.Contains(extension_id))
     return;
   extensions::ExtensionPrefs::DelayReason reason =
       extension_prefs_->GetDelayedInstallReason(extension_id);
 
@@ skipping 24 matching lines @@
   FinishDelayedInstallation(extension_id);
 }
 
 void ExtensionService::FinishDelayedInstallation(
     const std::string& extension_id) {
   scoped_refptr<const Extension> extension(
       GetPendingExtensionUpdate(extension_id));
   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;
   std::string old_name;
   if (existing_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);
 
   // Unpacked extensions default to allowing file access, but if that has been
   // overridden, don't reset the value.
   if (Manifest::ShouldAlwaysAllowFileAccess(extension->location()) &&
       !extension_prefs_->HasAllowFileAccessSetting(extension->id())) {
     extension_prefs_->SetAllowFileAccess(extension->id(), true);
   }
 
@@ skipping 18 matching lines @@
     }
   }
 
   // Check extensions that may have been delayed only because this shared module
   // was not available.
   if (SharedModuleInfo::IsSharedModule(extension)) {
     MaybeFinishDelayedInstallations();
   }
 }
 
+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);
 }
 
 void ExtensionService::TrackTerminatedExtension(const Extension* extension) {
   // No need to check for duplicates; inserting a duplicate is a no-op.
   registry_->AddTerminated(make_scoped_refptr(extension));
   extensions_being_terminated_.erase(extension->id());
   UnloadExtension(extension->id(), UnloadedExtensionInfo::REASON_TERMINATE);
@@ skipping 454 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }