Show alert failure for reloading unpacked extensions with bad manifest

chrome/browser/ui/webui/extensions/extension_loader_handler.cc

Index: chrome/browser/ui/webui/extensions/extension_loader_handler.cc
diff --git a/chrome/browser/ui/webui/extensions/extension_loader_handler.cc b/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
index 6e43177a4a3abd3fe57b684c2c362b5012a36f9b..29ee20474863bc3d0519270d11c4f12b9fdefc6a 100644
--- a/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
+++ b/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
@@ -12,7 +12,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/values.h"
+#include "chrome/browser/extensions/path_util.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
@@ -131,8 +131,11 @@ void ExtensionLoaderHandler::FileHelper::MultiFilesSelected(
 ExtensionLoaderHandler::ExtensionLoaderHandler(Profile* profile)
     : profile_(profile),
       file_helper_(new FileHelper(this)),
+      extension_error_reporter_observer_(this),
+      display_ready_(false),
       weak_ptr_factory_(this) {
   DCHECK(profile_);
+  extension_error_reporter_observer_.Add(ExtensionErrorReporter::GetInstance());
 }
 
 ExtensionLoaderHandler::~ExtensionLoaderHandler() {
@@ -155,9 +158,13 @@ void ExtensionLoaderHandler::GetLocalizedValues(
   source->AddString(
       "extensionLoadCouldNotLoadManifest",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_COULD_NOT_LOAD_MANIFEST));
+  source->AddString(
+      "extensionLoadAdditionalFailures",
+      l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_ADDITIONAL_FAILURES));
 }
 
 void ExtensionLoaderHandler::RegisterMessages() {
+  content::WebContentsObserver::Observe(web_ui()->GetWebContents());

[Finnur, 2014/07/14 10:25:58]

Do we need to call Observe(NULL) when we are done?

[gpdavis, 2014/07/14 18:58:19]

Done with what, exactly?  Done with observing for the notification that the UI
is ready to receive failures?

Observe(NULL) would remove this as an observer, right?  This object exists
through page refreshes.  It's constructed when the page is loaded and destructed
when the tab is closed.  So this needs to continue observing web contents,
because this object needs to continue being notified of page refreshes.

[Finnur, 2014/07/15 10:38:45]

Sounds good. Maybe that warrants a comment in the code?

[gpdavis, 2014/07/15 18:33:14]

Will do!

   web_ui()->RegisterMessageCallback(
       "extensionLoaderLoadUnpacked",
       base::Bind(&ExtensionLoaderHandler::HandleLoadUnpacked,
@@ -166,6 +173,14 @@ void ExtensionLoaderHandler::RegisterMessages() {
       "extensionLoaderRetry",
       base::Bind(&ExtensionLoaderHandler::HandleRetry,
                  weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderIgnoreFailure",
+      base::Bind(&ExtensionLoaderHandler::HandleIgnoreFailure,
+                 weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderDisplayFailures",
+      base::Bind(&ExtensionLoaderHandler::HandleDisplayFailures,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ExtensionLoaderHandler::HandleLoadUnpacked(const base::ListValue* args) {
@@ -175,16 +190,29 @@ void ExtensionLoaderHandler::HandleLoadUnpacked(const base::ListValue* args) {
 
 void ExtensionLoaderHandler::HandleRetry(const base::ListValue* args) {
   DCHECK(args->empty());
-  LoadUnpackedExtensionImpl(failed_path_);
+  const base::FilePath file_path = failed_paths_.back();
+  failed_paths_.pop_back();
+  LoadUnpackedExtensionImpl(file_path);

[Finnur, 2014/07/14 10:25:58]

This got me thinking... If I have two failures (extensions A, B -- in that order
on the failed_paths_ list), what happens if I fix A and retry? Will it only try
reloading B?

[gpdavis, 2014/07/14 18:58:19]

Yes, that is the current functionality.  The "Retry" and "Give up" buttons are
for the failure that is currently being displayed.  This is why there's a
separation with "additional failures".  It seemed to me like the user would
likely attempt to fix the failure whose manifest is being displayed.

Suggestions are more than welcome :)

[Finnur, 2014/07/15 10:38:45]

I see. That's good.

+}
+
+void ExtensionLoaderHandler::HandleIgnoreFailure(const base::ListValue* args) {
+  DCHECK(args->empty());
+  failed_paths_.pop_back();

[Finnur, 2014/07/14 10:25:58]

Similar argument here. Two failures: Should Ignore dismiss and remove one or
both?

[gpdavis, 2014/07/14 18:58:19]

I guess that's more of a design question for you guys.  I would make the
argument that "Give up" should only dismiss the failure that's currently being
displayed.  That way it can be used like a stack, and the failures can be dealt
with one by one.

[Finnur, 2014/07/15 10:38:45]

Yeah, that's fine.

[Devlin, 2014/07/15 17:51:44]

Just a reminder: There are only multiple failures in the case where:
a) The user has multiple unpacked extensions
b) Multiple unpacked extensions have load errors
c) The user hits refresh on the chrome:extensions page (we can never see
multiple errors from a reload or load unpacked command).

Since this situation is pretty rare (and since our current mechanism is to pop
up multiple modal dialogs [yuck!]), I think we can have something a bit rougher
here, as long as it makes sense.

(No action necessary - just documenting reasoning for future generations of
CLs).

+}
+
+void ExtensionLoaderHandler::HandleDisplayFailures(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+  display_ready_ = true;
+
+  if (!failures_.empty())
+    NotifyFrontendOfFailure();
 }
 
 void ExtensionLoaderHandler::LoadUnpackedExtensionImpl(
     const base::FilePath& file_path) {
   scoped_refptr<UnpackedInstaller> installer = UnpackedInstaller::Create(
       ExtensionSystem::Get(profile_)->extension_service());
-  installer->set_on_failure_callback(
-      base::Bind(&ExtensionLoaderHandler::OnLoadFailure,
-                 weak_ptr_factory_.GetWeakPtr()));
 
   // We do our own error handling, so we don't want a load failure to trigger
   // a dialog.
@@ -195,7 +223,6 @@ void ExtensionLoaderHandler::LoadUnpackedExtensionImpl(
 
 void ExtensionLoaderHandler::OnLoadFailure(const base::FilePath& file_path,
                                            const std::string& error) {
-  failed_path_ = file_path;
   size_t line = 0u;
   size_t column = 0u;
   std::string regex =
@@ -215,32 +242,51 @@ void ExtensionLoaderHandler::OnLoadFailure(const base::FilePath& file_path,
       content::BrowserThread::GetBlockingPool(),
       FROM_HERE,
       base::Bind(&ReadFileToString, file_path.Append(kManifestFilename)),
-      base::Bind(&ExtensionLoaderHandler::NotifyFrontendOfFailure,
+      base::Bind(&ExtensionLoaderHandler::AddFailure,
                  weak_ptr_factory_.GetWeakPtr(),
                  file_path,
                  error,
                  line));
 }
 
-void ExtensionLoaderHandler::NotifyFrontendOfFailure(
+void ExtensionLoaderHandler::DidStartNavigationToPendingEntry(
+    const GURL& url,
+    content::NavigationController::ReloadType reload_type) {
+  if (reload_type != content::NavigationController::NO_RELOAD)
+    display_ready_ = false;
+}
+
+void ExtensionLoaderHandler::AddFailure(
     const base::FilePath& file_path,
     const std::string& error,
     size_t line_number,
     const std::string& manifest) {
-  base::StringValue file_value(file_path.LossyDisplayName());
-  base::StringValue error_value(base::UTF8ToUTF16(error));
+  failed_paths_.push_back(file_path);
+  base::FilePath prettified_path = path_util::PrettifyPath(file_path);
 
-  base::DictionaryValue manifest_value;
+  scoped_ptr<base::DictionaryValue> manifest_value(new base::DictionaryValue());
   SourceHighlighter highlighter(manifest, line_number);
   // If the line number is 0, this highlights no regions, but still adds the
   // full manifest.
-  highlighter.SetHighlightedRegions(&manifest_value);
+  highlighter.SetHighlightedRegions(manifest_value.get());
+
+  scoped_ptr<base::DictionaryValue> failure(new base::DictionaryValue());
+  failure->Set("path",
+               new base::StringValue(prettified_path.LossyDisplayName()));
+  failure->Set("error", new base::StringValue(base::UTF8ToUTF16(error)));
+  failure->Set("manifest", manifest_value.release());
+  failures_.Append(failure.release());
+
+  // Only notify the frontend if the display is ready.
+  if (display_ready_)
+    NotifyFrontendOfFailure();
+}
 
+void ExtensionLoaderHandler::NotifyFrontendOfFailure() {
   web_ui()->CallJavascriptFunction(
       "extensions.ExtensionLoader.notifyLoadFailed",
-      file_value,
-      error_value,
-      manifest_value);
+      failures_);
+  failures_.Clear();
 }
 
 }  // namespace extensions