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..dda416e136e762afa0da4f00974c25fa7943d54a 100644
--- a/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
+++ b/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
@@ -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() {
@@ -166,6 +169,14 @@ void ExtensionLoaderHandler::RegisterMessages() {
       "extensionLoaderRetry",
       base::Bind(&ExtensionLoaderHandler::HandleRetry,
                  weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderSetDisplayLoading",
+      base::Bind(&ExtensionLoaderHandler::HandleSetDisplayLoading,
+                 weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderDisplayFailures",
+      base::Bind(&ExtensionLoaderHandler::HandleDisplayFailures,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ExtensionLoaderHandler::HandleLoadUnpacked(const base::ListValue* args) {
@@ -178,13 +189,43 @@ void ExtensionLoaderHandler::HandleRetry(const base::ListValue* args) {
   LoadUnpackedExtensionImpl(failed_path_);
 }
 
+void ExtensionLoaderHandler::HandleSetDisplayLoading(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+  display_ready_ = false;
+}
+
+void ExtensionLoaderHandler::HandleDisplayFailures(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+  display_ready_ = true;
+
+  if (failures_.empty())
+    return;
+
+  FailureData* failure = failures_[0];
+  NotifyFrontendOfFailure(failure->file_path,
+                          failure->error,
+                          failure->line_number,
+                          failure->manifest);
+
+  base::string16 list = base::UTF8ToUTF16("Additional failures:");

[gpdavis, 2014/06/30 19:42:52]

Is this what you meant by internationalization?

[Devlin, 2014/06/30 21:06:42]

Not quite.  This changes the string from UTF8 to UTF16, which we _do_ want to do
for most/all user-facing strings, but this doesn't actually internationalize it.
 You'll probably want to use something like l10n_util::GetStringUTF16()
(codesearch has a ton of examples on it).

That said, depending on the UI we use for the extra failures, we'll probably
want to just put this in the html in the end.

[gpdavis, 2014/06/30 21:41:19]

Wait-- I thought you had said this needed to be i18n, not l10n.

I'm pretty confused about what exactly the point of this is.  What does
internationalizing a string do?  If this were to remain in the C++ side (though
I know with UI changes it likely won't), would I then need to create a string
constant and an ID to pass into GetStringUTF16?

[Devlin, 2014/06/30 22:32:36]

Yeah, our libraries are funky.  i18n and l10n are usually very similar, and I
don't know if there's any difference in Chrome.  For some reason, we usually
still refer to it as i18n, though.

So the general practice would be to add a new message to
chrome/app/generated_resources.grd, and then use l10n_util::GetStringUTF16() to
retrieve it.  The messages in generated_resources.grd are translated into all
the languages that chrome is shipped in - and l10n_util::GetStringUTF16()
retrieves the string suited for the user's locale.  Otherwise, every user would
see the same message, even if it should be in a different language.

[gpdavis, 2014/06/30 23:35:11]

Ah, okay, that makes sense.  But since we're outsourcing this string population
to the JS, I won't add it in.  It's good to know how that works, though.  In
what particular instances is internationalization a critical thing?  Anything
that's user facing?

[Devlin, 2014/07/01 17:02:55]

_almost_ anything that's user facing.  There are a few exceptions for things
like extension manifest errors, and we did that so that developers could google
the error and get help.  But any part of the UI like this should be.

+  for (size_t i = 1; i < failures_.size(); ++i) {
+    FailureData *failure = failures_[i];

[Devlin, 2014/06/30 21:06:42]

nit: FailureData* failure - note the asterisk position.

[gpdavis, 2014/06/30 21:41:19]

Done.

+    list.append(base::UTF8ToUTF16("\n"));
+    list.append(failure->file_path.LossyDisplayName());
+  }
+  failures_.clear();
+  base::StringValue list_value(list);
+  web_ui()->CallJavascriptFunction(
+      "extensions.ExtensionLoader.notifyAdditional",

[gpdavis, 2014/06/30 19:42:52]

I've been thinking about how to go about condensing this down to one message so
that the UI can sort failures out on its own.  There are only a few instances in
which multiple failures are possible-- refreshing the extensions page, or
explicitly reloading multiple broken extensions without handling the errors (by
reloading or giving up).

In the event of a page refresh, this should be relatively simple since
everything necessary to build a list of broken extensions is set up.  However,
for multiple user triggered reloads, it gets tricky.  I was thinking that we
would need to keep track of all the currently active load failures somewhere,
then we need to be able to send a message to the UI that another load has failed
and that the UI needs to reflect that change without losing the information
about the previous failures.

Do you have any specific input on this?  I think I could throw something
together to make this work, but your feedback is always appreciated.

[Devlin, 2014/06/30 21:06:42]

For this, at least as a starting point, I wouldn't mind only displaying load
errors for the previous "batch" of loads.  That is, if we refresh the page,
display multiple load errors, but if the user does something like try to reload
a second extension when one has already failed, just show the most recent.  We
don't have to persist a full list (and it might even be annoying if we did).

That said, still waiting on an answer to what current behavior for multiple
immediate load failures is (e.g., two load failures on page refresh)?  This
could determine how we want to make the new behavior look.

[gpdavis, 2014/06/30 21:41:19]

Oops-- thought I responded to that.

Currently, multiple popups do show up, and they are overlapping.

So, just to clarify, in this scenario, if the page were refreshed and multiple
load errors were displayed, then a user reloaded one extension, the UI would
revert to just showing the bad manifest of the one they reloaded, with no
additional information about the load failures from the page refresh?

[Devlin, 2014/06/30 22:32:36]

Good point that it'd be somewhat strange for a user to see the errors disappear.
 That said, it's really such an edge case (the chances that multiple extensions
will fail to reload is actually very low), so I think I'd be okay with it. 
Maybe just have the list be "And these other failures: " followed the full error
message we currently give.  In a followup, we can spruce up the UI to support
multiple.

[gpdavis, 2014/06/30 23:35:11]

Yeah, you're right, that is quite an edge case.  It seems to me the UI ought to
behave like a stack; the first load error shows the manifest, then the next
error will display the manifest for the next load failure and push the old one
down to a minimized version (ie, other load failure: |path|).  Then if the
displayed error is dealt with, the one that was pushed down gets popped back up
with its manifest error displayed.

This may be a challenge to implement in the JS, but I'm up for the challenge. 
Let me know if you've got any other input on this.  If not, I'll just take a
stab at it and have you pick apart my solution ;)

[Devlin, 2014/07/01 17:02:55]

I would envision something similar, though with more of an accordion UI.  If you
wanna take it on, that'd be great. :)  But let's get this patch in first, and do
that in a followup.

[gpdavis, 2014/07/01 20:58:55]

I was already mostly done with this last night before you responded, so I
uploaded my changes.  I'm eager to hear what you think!

+      list_value);

[Devlin, 2014/06/30 21:06:42]

nit: inline the string value construction here.

[gpdavis, 2014/06/30 21:41:19]

Done.

+}
+
 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.
@@ -227,6 +268,16 @@ void ExtensionLoaderHandler::NotifyFrontendOfFailure(
     const std::string& error,
     size_t line_number,
     const std::string& manifest) {
+  // If the extensions page is not loaded, delay frontend notification.
+  if (!display_ready_) {
+    FailureData* failure = new FailureData(file_path,
+                                           error,
+                                           line_number,
+                                           manifest);
+    failures_.push_back(failure);

[Devlin, 2014/06/30 21:06:42]

nit: inline the failuredata construction here.

[gpdavis, 2014/06/30 21:41:19]

Done.

+    return;
+  }
+
   base::StringValue file_value(file_path.LossyDisplayName());
   base::StringValue error_value(base::UTF8ToUTF16(error));