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..97ec4d318a3dfb7cd3bd326790c7cb43843046bb 100644
--- a/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
+++ b/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
@@ -128,11 +128,31 @@ void ExtensionLoaderHandler::FileHelper::MultiFilesSelected(
   NOTREACHED();
 }
 
+// Class to hold failure data when NotifyFrontendOfFailure is called before
+// the extensions page is fully loaded.
+class ExtensionLoaderHandler::FailureData {
+  public:

[Devlin, 2014/06/27 22:31:10]

fix indentation.

[gpdavis, 2014/06/28 02:31:18]

Done.

+   explicit FailureData(base::FilePath path, std::string error,

[Devlin, 2014/06/27 22:31:09]

nit: one param per line.

[Devlin, 2014/06/27 22:31:10]

const & on non-simple arguments.

[gpdavis, 2014/06/28 02:31:18]

Done.

[gpdavis, 2014/06/28 02:31:18]

Done.

+                        size_t line, std::string manifest)
+     : file_path(path),
+       error(error),
+       line_number(line),
+       manifest(manifest) {}
+
+   base::FilePath file_path;

[Devlin, 2014/06/27 22:31:09]

entirely public classes are what structs are for ;)

[gpdavis, 2014/06/27 23:42:54]

That was what I started with, but I wasn't sure if I could have a vector of
structs, so I made it a class instead.  I suppose there should be no reason this
wouldn't work with structs.  I'll have to play around with it and see if I can
get that switched over.

[Devlin, 2014/06/28 08:40:02]

A couple of C++ finer points:
- A vector of structs is pretty easy.
- You're not actually storing a vector of structs right now, you're storing a
vector of pointers.
- If you continue to store a vector of pointers (which is not bad!), you should
add DISALLOW_COPY_AND_ASSIGN (even to a struct).

But yeah - structs and classes in C++ are perfectly similar.  The only real
difference is that the default permission level for structs is public, whereas
for classes it's private.

+   std::string error;
+   size_t line_number;
+   std::string manifest;
+};
+
 ExtensionLoaderHandler::ExtensionLoaderHandler(Profile* profile)
     : profile_(profile),
       file_helper_(new FileHelper(this)),
-      weak_ptr_factory_(this) {
+      weak_ptr_factory_(this),

[Devlin, 2014/06/27 22:31:09]

weak_ptr_factory_ should be the final data member.

[gpdavis, 2014/06/28 02:31:18]

Done.

+      extension_error_reporter_observer_(this),
+      display_ready_(false) {
   DCHECK(profile_);
+  extension_error_reporter_observer_.Add(ExtensionErrorReporter::GetInstance());
 }
 
 ExtensionLoaderHandler::~ExtensionLoaderHandler() {
@@ -166,6 +186,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,18 +206,45 @@ 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()) {

[Devlin, 2014/06/27 22:31:09]

instead of:
function {
  if (condition) {
    <lots of lines>
  }
}

prefer:
function {
  if (!condition)
    return
  <lots of lines>
}

[gpdavis, 2014/06/27 23:42:54]

That is indeed much better.  Will fix (y)

[gpdavis, 2014/06/28 02:31:18]

Done.

+    FailureData *failure = failures_[0];

[Devlin, 2014/06/27 22:31:09]

nit: FailureData* failure

[gpdavis, 2014/06/28 02:31:18]

Done.

+    NotifyFrontendOfFailure(failure->file_path, failure->error,

[Devlin, 2014/06/27 22:31:09]

one arg per line.

[gpdavis, 2014/06/28 02:31:18]

Done.

+                            failure->line_number, failure->manifest);
+
+    std::string list("Additional failures:");

[Devlin, 2014/06/27 22:31:09]

This needs to be i18n.

[gpdavis, 2014/06/28 02:31:18]

Could you elaborate a little on this?  I looked up i18n.h and I don't quite
understand what it is.

[Devlin, 2014/06/28 08:40:02]

Ah, sorry :)  i18n = internationalization.  (See why?  It's because there are 18
letters between the beginning 'i' and the final 'n' in the word
'internationalization'.  We're so clever... ;))  Another common one is
localization (l10n).  Basically, this means that the string has to be a
specialized variable that will be made substituted for the proper translation
when displayed in the browser.  For examples, see stuff like
GetLocalizedValues(), l10n_util, etc.

+    for (size_t i = 1; i < failures_.size(); i++) {

[Devlin, 2014/06/27 22:31:09]

++i.

[gpdavis, 2014/06/28 02:31:18]

Done.

+      FailureData *failure = failures_[i];
+      list.append("\n");
+      list.append(failure->file_path.value());

[Devlin, 2014/06/27 22:31:10]

Use one of the display functions in FilePath.

[gpdavis, 2014/06/27 23:42:54]

Sounds good.  LossyDisplayName seem reasonable?

[Devlin, 2014/06/28 08:40:02]

Yep!

+    }
+    failures_.clear();

[Devlin, 2014/06/27 22:31:09]

Gaaah!  Leaaaaaaak!
;)

But really.  Leaks are bad. :)

[gpdavis, 2014/06/27 23:42:54]

I had a feeling there was going to be a memory leak here.

What would be the preferred way to free up the memory of those references before
clearing the vector?

[Devlin, 2014/06/28 08:40:02]

For vectors, take a look at scoped_vector.h.  Handy thing, that. :)

+    base::StringValue list_value(list);
+    web_ui()->CallJavascriptFunction(
+        "extensions.ExtensionLoader.notifyAdditional",
+        list_value);

[Devlin, 2014/06/27 22:31:09]

If we go with this approach for multi errors, I think it would be better to send
all errors in one big batch, and let the UI work it out.  This has the advantage
of, for instance, allowing us to display the next error if the user choose "I
give up" on the first one.

[gpdavis, 2014/06/27 23:42:54]

This is certainly going to require heavier UI adjustments.  Knowing that UI
modifications need to be specifically reviewed, I would like to have somebody
work with me to figure out the most reasonable solution.  But we can cross that
bridge when we come to it.  It would be a relatively minor adjustment to make
this send off all at once instead of the way it currently is.

How about we make that determination once the primary architectural changes look
good?

[Devlin, 2014/06/28 08:40:02]

Sure, I'm not too worried about it.  But the only parts that need to be UI
reviewed are the visible changes - the suggestion was for mostly a logical
change.

+  }
+}
+
 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.
   installer->set_be_noisy_on_failure(false);
 
+  display_ready_ = true;

[Devlin, 2014/06/27 22:31:10]

This seems like an odd time to set |display_ready_|...

[gpdavis, 2014/06/28 02:31:18]

Done.

   installer->Load(file_path);
 }
 
@@ -227,6 +282,15 @@ 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_) {
+    failures_.push_back(new FailureData(file_path,
+                                        error,
+                                        line_number,
+                                        manifest));
+    return;
+  }
+
   base::StringValue file_value(file_path.LossyDisplayName());
   base::StringValue error_value(base::UTF8ToUTF16(error));
 
@@ -241,6 +305,7 @@ void ExtensionLoaderHandler::NotifyFrontendOfFailure(
       file_value,
       error_value,
       manifest_value);
+  display_ready_ = false;

[Devlin, 2014/06/27 22:31:09]

This too seems like an odd time to set display ready.

I assume these are just so that you know when to notify of additional failures
vs the primary failure?  There should be a better way of doing this.

[gpdavis, 2014/06/27 23:42:54]

This was initially done because I started off implementing this so that we never
assume the display is ready unless the flag was explicitly set before the call,
so I reset it beforehand.  This is also the reason why it's set to true in
LoadUnpackedExtensionImple; if the user clicked the button to load the
extension, it follows that the UI must be ready.

But since I added the events to the JS that notified this class when the UI was
ready or not ready, this is no longer necessary.  I'll look into pulling these
out safely.

[gpdavis, 2014/06/28 02:31:18]

Done.

 }
 
 }  // namespace extensions