Show alert failure for reloading unpacked extensions with bad manifest

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

Index: chrome/browser/ui/webui/extensions/extension_loader_handler.h
diff --git a/chrome/browser/ui/webui/extensions/extension_loader_handler.h b/chrome/browser/ui/webui/extensions/extension_loader_handler.h
index 5d0351f97a0bfb200b2b64473ed49437a482393f..529175d411e9f05b70b3f5b5d956b44bcb329527 100644
--- a/chrome/browser/ui/webui/extensions/extension_loader_handler.h
+++ b/chrome/browser/ui/webui/extensions/extension_loader_handler.h
@@ -6,12 +6,16 @@
 #define CHROME_BROWSER_UI_WEBUI_EXTENSIONS_EXTENSION_LOADER_HANDLER_H_
 
 #include <string>
+#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
+#include "chrome/browser/extensions/extension_error_reporter.h"
 #include "content/public/browser/web_ui_message_handler.h"
 
 namespace base {
@@ -29,7 +33,8 @@ namespace extensions {
 class Extension;
 
 // The handler page for the Extension Commands UI overlay.
-class ExtensionLoaderHandler : public content::WebUIMessageHandler {
+class ExtensionLoaderHandler : public content::WebUIMessageHandler,
+                               public ExtensionErrorReporter::Observer {
  public:
   explicit ExtensionLoaderHandler(Profile* profile);
   virtual ~ExtensionLoaderHandler();
@@ -43,17 +48,43 @@ class ExtensionLoaderHandler : public content::WebUIMessageHandler {
  private:
   class FileHelper;
 
+  // Struct to hold failure data when NotifyFrontendOfFailure is called before
+  // the extensions page is fully loaded.
+  struct FailureData {
+    explicit FailureData(base::FilePath file_path,

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

no need for explicit with multiple parameters to the constructor (google
"explicit constructor c++").

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

Done.

+                std::string error,

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

nit: indentation error.

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

Done.

+                size_t line_number,
+                std::string manifest)
+        : file_path(file_path),
+          error(error),
+          line_number(line_number),
+          manifest(manifest) {}

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

There should probably be a newline here, right?

How's my struct look? :)

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

Make an out-of-line constructor, and an explicit destructor.

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

An "out-of-line" constructor?  I've never heard this term before.  Can you
explain?

Done on the destructor, though.

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

out-of-line is the (slang) opposite of inline.  Meaning, put the definition
outside of the class definition.

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

Ah, gotcha.  Can do.

+    base::FilePath file_path;
+    std::string error;
+    size_t line_number;
+    std::string manifest;
+
+    DISALLOW_COPY_AND_ASSIGN(FailureData);

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

One note and one problem:
note - we don't actually _need_ disallow copy and assign on this, because it's
all simple stuff (and knows how to copy itself correctly).  That said, it's
always best to avoid it if we can, so just FYI that if you need to copy this,
you can. :)

problem - look at the definition of DISALLOW_COPY_AND_ASSIGN in base/macros.h -
can you find why the way you have it won't work?

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

C++ is a pretty new language to me-- I'm pretty well familiar with C, but I
started learning C++ at this job.

I'm not really sure why that wouldn't work.  I don't think I understand const
and & well enough.  Is there some reason you couldn't create a constructor for a
struct with a reference to another instance of that struct?

In any case, I'll remove it.

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

So, what DISALLOW_COPY_AND_ASSIGN does is just declares the two "missing"
constructors (copy constructor and assignment constructor).  The way this
normally works is by doing:
class Foo {
 public:
  Foo();
  ~Foo();
  void Bar();
 private:
  DISALLOW_COPY_AND_ASSIGN(Foo);
}

This "disallows" the two other constructors because they are in the classes
private namespace - no outside callers can call them, and it will result in a
compile error.  By putting a DISALLOW_COPY_AND_ASSIGN in the public namespace of
the struct, it does not restrict other classes from calling them.

Make sense?

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

Ahh, I see. So by creating private dummies for those constructors, there's no
way for them to be accessed.  That does make sense.  Thanks for that
explanation!

+  };
+
   // Handle the 'extensionLoaderLoadUnpacked' message.
   void HandleLoadUnpacked(const base::ListValue* args);
 
   // Handle the 'extensionLoaderRetry' message.
   void HandleRetry(const base::ListValue* args);
 
+  // Handle the 'extensionLoaderSetDisplayLoading' message.
+  void HandleSetDisplayLoading(const base::ListValue* args);
+
+  // Handle the 'extensionLoaderDisplayFailures' message.
+  void HandleDisplayFailures(const base::ListValue* args);
+
   // Try to load an unpacked extension from the given |file_path|.
   void LoadUnpackedExtensionImpl(const base::FilePath& file_path);
 
-  // Called when an unpacked extension fails to load.
-  void OnLoadFailure(const base::FilePath& file_path, const std::string& error);
+  // ExtensionErrorReporter::Observer implementation.
+  virtual void OnLoadFailure(const base::FilePath& file_path,
+                             const std::string& error) OVERRIDE;
 
   // Notify the frontend of the failure. If it was a manifest error, |manifest|
   // will hold the manifest contents, and |line_number| will point to the line
@@ -73,6 +104,15 @@ class ExtensionLoaderHandler : public content::WebUIMessageHandler {
   // loaded when the user selects "retry".
   base::FilePath failed_path_;
 
+  // Holds information about all unpacked extension install failures that
+  // were reported while the extensions page was loading.
+  ScopedVector<FailureData> failures_;
+
+  ScopedObserver<ExtensionErrorReporter, ExtensionErrorReporter::Observer>
+      extension_error_reporter_observer_;
+
+  bool display_ready_;
+
   // Weak pointer factory for posting background tasks.
   base::WeakPtrFactory<ExtensionLoaderHandler> weak_ptr_factory_;