New post-sideload UI:

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 15c9535ef502aa0af6f7546325221de0d0fdfcbc..3ad205ec0caea2ae55a490cec81efbee69fa9735 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -60,6 +60,7 @@
 #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"
@@ -156,6 +157,9 @@ 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) {
@@ -866,6 +870,9 @@ void ExtensionService::EnableExtension(const std::string& extension_id) {
   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());
@@ -1763,6 +1770,8 @@ void ExtensionService::IdentifyAlertableExtensions() {
     }
   }
 
+  UpdateExternalExtensionAlert();
+
   if (!did_show_alert)
     extension_error_ui_.reset();
 }
@@ -1773,14 +1782,6 @@ bool ExtensionService::PopulateExtensionErrorUI(
   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());
@@ -1802,13 +1803,8 @@ void ExtensionService::HandleExtensionAlertClosed() {
 }
 
 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);
@@ -1822,12 +1818,47 @@ void ExtensionService::HandleExtensionAlertAccept() {
 
 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) {
@@ -2167,6 +2198,10 @@ void ExtensionService::OnExtensionInstalled(
   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)) {
@@ -2250,6 +2285,9 @@ void ExtensionService::OnExtensionInstalled(
 
   // Transfer ownership of |extension| to AddExtension.
   AddExtension(scoped_extension);
+
+  if (Extension::IsExternalLocation(extension->location()))
+    UpdateExternalExtensionAlert();
 }
 
 const Extension* ExtensionService::GetExtensionByIdInternal(