Show alert failure for reloading unpacked extensions with bad manifest

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_WEBUI_EXTENSIONS_EXTENSION_LOADER_HANDLER_H_
 #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 {
 class ListValue;
 }
 
 namespace content {
 class WebUIDataSource;
 }
 
 class Profile;
 
 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();
 
   // Fetches the localized values for the page and deposits them into |source|.
   void GetLocalizedValues(content::WebUIDataSource* source);
 
   // WebUIMessageHandler implementation.
   virtual void RegisterMessages() OVERRIDE;
 
  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
   // at which the error was found.
   void NotifyFrontendOfFailure(const base::FilePath& file_path,
                                const std::string& error,
                                size_t line_number,
                                const std::string& manifest);
 
   // The profile with which this Handler is associated.
   Profile* profile_;
 
   // A helper to manage file picking.
   scoped_ptr<FileHelper> file_helper_;
 
   // The file path to the extension that failed to load, or empty. This is
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionLoaderHandler);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_UI_WEBUI_EXTENSIONS_EXTENSION_LOADER_HANDLER_H_