New post-sideload UI:

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2012 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 <set>
 
 #include "base/basictypes.h"
@@ skipping 42 matching lines @@
 #include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extension_error_ui.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_install_ui.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
 #include "chrome/browser/extensions/extension_sorting.h"
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
+#include "chrome/browser/extensions/external_install_ui.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
 #include "chrome/browser/extensions/external_provider_interface.h"
 #include "chrome/browser/extensions/installed_loader.h"
 #include "chrome/browser/extensions/lazy_background_task_queue.h"
 #include "chrome/browser/extensions/pending_extension_manager.h"
 #include "chrome/browser/extensions/permissions_updater.h"
 #include "chrome/browser/extensions/platform_app_launcher.h"
 #include "chrome/browser/extensions/settings/settings_frontend.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
@@ skipping 76 matching lines @@
 static const int kOmniboxIconPaddingLeft = 2;
 static const int kOmniboxIconPaddingRight = 2;
 #elif defined(OS_MACOSX)
 static const int kOmniboxIconPaddingLeft = 0;
 static const int kOmniboxIconPaddingRight = 2;
 #else
 static const int kOmniboxIconPaddingLeft = 0;
 static const int kOmniboxIconPaddingRight = 0;
 #endif
 
+// Prompt the user this many times before considering an extension acknowledged.
+static const int kMaxExtensionAcknowledgePromptCount = 3;
+
 const char* kNaClPluginMimeType = "application/x-nacl";
 
 static bool IsSyncableExtension(const Extension& extension) {
   return extension.GetSyncType() == Extension::SYNC_TYPE_EXTENSION;
 }
 
 static bool IsSyncableApp(const Extension& extension) {
   return extension.GetSyncType() == Extension::SYNC_TYPE_APP;
 }
 
@@ skipping 674 matching lines @@
   // enabled unless otherwise noted.
   return !extension_prefs_->IsExtensionDisabled(extension_id) &&
       !extension_prefs_->IsExternalExtensionUninstalled(extension_id);
 }
 
 bool ExtensionService::IsExternalExtensionUninstalled(
     const std::string& extension_id) const {
   return extension_prefs_->IsExternalExtensionUninstalled(extension_id);
 }
 
 void ExtensionService::EnableExtension(const std::string& extension_id) {

[Jeffrey Yasskin, 2012/10/15 00:43:59]

Is there a comment somewhere saying that this function is called in response to
user actions instead of some other way of enabling extensions?

[Matt Perry, 2012/10/15 19:13:38]

I'm not sure what you're asking. EnableExtension is called from a variety of
places, whenever we need to enable the extension.

   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (IsExtensionEnabled(extension_id))
     return;
 
   extension_prefs_->SetExtensionState(extension_id, Extension::ENABLED);
   extension_prefs_->ClearDisableReasons(extension_id);
 
   const Extension* extension = GetExtensionByIdInternal(extension_id,
       INCLUDE_DISABLED);
   // This can happen if sync enables an extension that is not
   // installed yet.
   if (!extension)
     return;
 
+  if (Extension::IsExternalLocation(extension->location()))
+    AcknowledgeExternalExtension(extension->id());
+
   // Move it over to the enabled list.
   extensions_.Insert(make_scoped_refptr(extension));
   disabled_extensions_.Remove(extension->id());
 
   // Make sure any browser action contained within it is not hidden.
   extension_prefs_->SetBrowserActionVisibility(extension, true);
 
   NotifyExtensionLoaded(extension);
 
   // Notify listeners that the extension was enabled.
@@ skipping 877 matching lines @@
       CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
       did_show_alert = extension_error_ui_->ShowErrorInBubbleView();
     } else {
       // First run. Just acknowledge all the extensions, silently, by
       // shortcutting the display of the UI and going straight to the
       // callback for pressing the Accept button.
       HandleExtensionAlertAccept();
     }
   }
 
+  UpdateExternalExtensionAlert();
+
   if (!did_show_alert)
     extension_error_ui_.reset();
 }
 
 bool ExtensionService::PopulateExtensionErrorUI(
     ExtensionErrorUI* extension_error_ui) {
   bool needs_alert = false;
   for (ExtensionSet::const_iterator iter = extensions_.begin();
        iter != extensions_.end(); ++iter) {
     const Extension* e = *iter;
-    if (Extension::IsExternalLocation(e->location())) {
-      if (!e->is_hosted_app()) {
-        if (!extension_prefs_->IsExternalExtensionAcknowledged(e->id())) {
-          extension_error_ui->AddExternalExtension(e->id());
-          needs_alert = true;
-        }
-      }
-    }
     if (!extension_prefs_->UserMayLoad(e, NULL)) {
       if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(e->id())) {
         extension_error_ui->AddBlacklistedExtension(e->id());
         needs_alert = true;
       }
     }
     if (extension_prefs_->IsExtensionOrphaned(e->id())) {
       if (!extension_prefs_->IsOrphanedExtensionAcknowledged(e->id())) {
         extension_error_ui->AddOrphanedExtension(e->id());
         needs_alert = true;
       }
     }
   }
   return needs_alert;
 }
 
 void ExtensionService::HandleExtensionAlertClosed() {
   extension_error_ui_.reset();
 }
 
 void ExtensionService::HandleExtensionAlertAccept() {
-  const ExtensionIdSet *extension_ids =
-      extension_error_ui_->get_external_extension_ids();
-  for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
-       iter != extension_ids->end(); ++iter) {
-    AcknowledgeExternalExtension(*iter);
-  }
-  extension_ids = extension_error_ui_->get_blacklisted_extension_ids();
+  const ExtensionIdSet* extension_ids =
+      extension_error_ui_->get_blacklisted_extension_ids();
   for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
        iter != extension_ids->end(); ++iter) {
     extension_prefs_->AcknowledgeBlacklistedExtension(*iter);
   }
   extension_ids = extension_error_ui_->get_orphaned_extension_ids();
   for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
        iter != extension_ids->end(); ++iter) {
     extension_prefs_->AcknowledgeOrphanedExtension(*iter);
   }
 }
 
 void ExtensionService::AcknowledgeExternalExtension(const std::string& id) {
   extension_prefs_->AcknowledgeExternalExtension(id);
+  UpdateExternalExtensionAlert();
 }
 
 void ExtensionService::HandleExtensionAlertDetails() {
   extension_error_ui_->ShowExtensions();
 }
 
+void ExtensionService::UpdateExternalExtensionAlert() {
+  const Extension* extension = NULL;
+  if (!extension) {

[Jeffrey Yasskin, 2012/10/13 21:17:35]

extension is definitely NULL here.

[Matt Perry, 2012/10/15 19:13:38]

You can never be too sure... :)

Fixed.

+    for (ExtensionSet::const_iterator iter = disabled_extensions_.begin();
+         iter != disabled_extensions_.end(); ++iter) {
+      const Extension* e = *iter;
+      if (Extension::IsExternalLocation(e->location())) {
+        if (!e->is_hosted_app()) {
+          if (!extension_prefs_->IsExternalExtensionAcknowledged(e->id())) {
+            extension = e;
+            break;
+          }
+        }
+      }
+    }
+  }
+
+  if (extension) {
+    if (extensions::AddExternalInstallError(this, extension)) {
+      // Note: this can sometimes overcount if there are multiple external
+      // extensions installed on a single restart of Chrome (since
+      // UpdateExternalExtensionAlert may get called multiple times). But that

[Jeffrey Yasskin, 2012/10/13 21:17:35]

Won't it get called for different extensions in that case?

[Matt Perry, 2012/10/15 19:13:38]

Not necessarily. This method is called both on startup (1) and each time an
install finishes (2). I can't remember the details now, but there was a
particular order that resulted in this getting called twice: once when only
extensionA is installed, then again with extensionA+extensionB. If we end up
choosing to display extensionB the second time, then the first call for
extensionA was an overcount.

+      // is rare, so for simplicity, we ignore that case.
+      if (extension_prefs_->IncrementAcknowledgePromptCount(extension->id()) >=
+              kMaxExtensionAcknowledgePromptCount) {
+        // This will be the last time we prompt for this extension.
+        extension_prefs_->AcknowledgeExternalExtension(extension->id());
+      }
+    }
+  } else {
+    extensions::RemoveExternalInstallError(this);
+  }
+}
+
 void ExtensionService::UnloadExtension(
     const std::string& extension_id,
     extension_misc::UnloadedExtensionReason reason) {
   // Make sure the extension gets deleted after we return from this function.
   int include_mask = INCLUDE_ENABLED | INCLUDE_DISABLED;
   scoped_refptr<const Extension> extension(
       GetExtensionByIdInternal(extension_id, include_mask));
 
   // This method can be called via PostTask, so the extension may have been
   // unloaded by the time this runs.
@@ skipping 318 matching lines @@
     bool has_requirement_errors) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Ensure extension is deleted unless we transfer ownership.
   scoped_refptr<const Extension> scoped_extension(extension);
   const std::string& id = extension->id();
   // Extensions installed by policy can't be disabled. So even if a previous
   // installation disabled the extension, make sure it is now enabled.
   bool initial_enable =
       !extension_prefs_->IsExtensionDisabled(id) ||
       system_->management_policy()->MustRemainEnabled(extension, NULL);

[Jeffrey Yasskin, 2012/10/15 00:43:59]

Seems odd to override the management policy with the external install check. If
that's correct, could you update the comment to cover the combination of
conditions?

Would it make sense to extract the |initial_enable| calculation into a helper
function?

[Matt Perry, 2012/10/15 19:13:38]

You're right - management policy should win out here.
Done.

+  if (Extension::IsExternalLocation(extension->location()) &&
+      !extension_prefs_->IsExternalExtensionAcknowledged(id)) {
+    initial_enable = false;
+  }
   const extensions::PendingExtensionInfo* pending_extension_info = NULL;
   if ((pending_extension_info = pending_extension_manager()->GetById(id))) {
     if (!pending_extension_info->ShouldAllowInstall(*extension)) {
       pending_extension_manager()->Remove(id);
 
       LOG(WARNING) << "ShouldAllowInstall() returned false for "
                    << id << " of type " << extension->GetType()
                    << " and update URL " << extension->update_url().spec()
                    << "; not installing";
 
@@ skipping 63 matching lines @@
     extension_prefs_->SetAllowFileAccess(id, true);
   }
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_EXTENSION_INSTALLED,
       content::Source<Profile>(profile_),
       content::Details<const Extension>(extension));
 
   // Transfer ownership of |extension| to AddExtension.
   AddExtension(scoped_extension);
+
+  if (Extension::IsExternalLocation(extension->location()))
+    UpdateExternalExtensionAlert();
 }
 
 const Extension* ExtensionService::GetExtensionByIdInternal(
     const std::string& id, int include_mask) const {
   std::string lowercase_id = StringToLowerASCII(id);
   if (include_mask & INCLUDE_ENABLED) {
     const Extension* extension = extensions_.GetByID(lowercase_id);
     if (extension)
       return extension;
   }
@@ skipping 422 matching lines @@
                                 extension_host->extension(),
                                 NULL, FilePath());
 #endif
 }
 
 void ExtensionService::InspectExtensionHost(
     extensions::ExtensionHost* host) {
   if (host)
     DevToolsWindow::OpenDevToolsWindow(host->render_view_host());
 }