Make UserScript non-copyable.

extensions/common/user_script.h

Index: extensions/common/user_script.h
diff --git a/extensions/common/user_script.h b/extensions/common/user_script.h
index 44a2d671f08e914bdecbae317562438fe37477e4..d6108d0b9592eb3123fb63122e044ab1b7b6abd2 100644
--- a/extensions/common/user_script.h
+++ b/extensions/common/user_script.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "base/pickle.h"
 #include "base/strings/string_piece.h"
 #include "extensions/common/host_id.h"
 #include "extensions/common/url_pattern.h"
@@ -22,8 +23,6 @@ class PickleIterator;
 
 namespace extensions {
 
-// Represents a user script, either a standalone one, or one that is part of an
-// extension.
 class UserScript {
  public:
   // The file extension for standalone user scripts.
@@ -73,69 +72,21 @@ class UserScript {
     RUN_LOCATION_LAST  // Leave this as the last item.
   };
 
-  // Holds actual script file info.
-  class File {
-   public:
-    File(const base::FilePath& extension_root,
-         const base::FilePath& relative_path,
-         const GURL& url);
-    File();
-    File(const File& other);
-    ~File();
-
-    const base::FilePath& extension_root() const { return extension_root_; }
-    const base::FilePath& relative_path() const { return relative_path_; }
-
-    const GURL& url() const { return url_; }
-    void set_url(const GURL& url) { url_ = url; }
-
-    // If external_content_ is set returns it as content otherwise it returns
-    // content_
-    const base::StringPiece GetContent() const {
-      if (external_content_.data())
-        return external_content_;
-      else
-        return content_;
-    }
-    void set_external_content(const base::StringPiece& content) {
-      external_content_ = content;
-    }
-    void set_content(const base::StringPiece& content) {
-      content_.assign(content.begin(), content.end());
-    }
-
-    // Serialization support. The content and FilePath members will not be
-    // serialized!
-    void Pickle(base::Pickle* pickle) const;
-    void Unpickle(const base::Pickle& pickle, base::PickleIterator* iter);
-
-   private:
-    // Where the script file lives on the disk. We keep the path split so that
-    // it can be localized at will.
-    base::FilePath extension_root_;
-    base::FilePath relative_path_;
-
-    // The url to this scipt file.
-    GURL url_;
-
-    // The script content. It can be set to either loaded_content_ or
-    // externally allocated string.
-    base::StringPiece external_content_;
-
-    // Set when the content is loaded by LoadContent
-    std::string content_;
-  };
-
-  typedef std::vector<File> FileList;
 
   // Type of a API consumer instance that user scripts will be injected on.
   enum ConsumerInstanceType { TAB, WEBVIEW };
+};
 
+// Contains information related to user scripts, either a standalone one, or one
+// that is part of an extension.
+// This class does not contain any script file (JS/CSS) related information.
+class UserScriptInfo {
+ public:
   // Constructor. Default the run location to document end, which is like
   // Greasemonkey and probably more useful for typical scripts.
-  UserScript();
-  UserScript(const UserScript& other);
-  ~UserScript();
+  UserScriptInfo();
+  UserScriptInfo(const UserScriptInfo& other);
+  virtual ~UserScriptInfo();
 
   const std::string& name_space() const { return name_space_; }
   void set_name_space(const std::string& name_space) {
@@ -156,8 +107,10 @@ class UserScript {
   }
 
   // The place in the document to run the script.
-  RunLocation run_location() const { return run_location_; }
-  void set_run_location(RunLocation location) { run_location_ = location; }
+  UserScript::RunLocation run_location() const { return run_location_; }
+  void set_run_location(UserScript::RunLocation location) {
+    run_location_ = location;
+  }
 
   // Whether to emulate greasemonkey when running this script.
   bool emulate_greasemonkey() const { return emulate_greasemonkey_; }
@@ -193,24 +146,16 @@ class UserScript {
   }
   void add_exclude_url_pattern(const URLPattern& pattern);
 
-  // List of js scripts for this user script
-  FileList& js_scripts() { return js_scripts_; }
-  const FileList& js_scripts() const { return js_scripts_; }
-
-  // List of css scripts for this user script
-  FileList& css_scripts() { return css_scripts_; }
-  const FileList& css_scripts() const { return css_scripts_; }
-
   const std::string& extension_id() const { return host_id_.id(); }
 
   const HostID& host_id() const { return host_id_; }
   void set_host_id(const HostID& host_id) { host_id_ = host_id; }
 
-  const ConsumerInstanceType& consumer_instance_type() const {
+  const UserScript::ConsumerInstanceType& consumer_instance_type() const {
     return consumer_instance_type_;
   }
   void set_consumer_instance_type(
-      const ConsumerInstanceType& consumer_instance_type) {
+      const UserScript::ConsumerInstanceType& consumer_instance_type) {
     consumer_instance_type_ = consumer_instance_type;
   }
 
@@ -224,16 +169,16 @@ class UserScript {
   // otherwise.
   bool MatchesURL(const GURL& url) const;
 
-  // Serialize the UserScript into a pickle. The content of the scripts and
+ protected:
+  // Serializes the UserScriptInfo into a pickle. The content of the scripts and
   // paths to UserScript::Files will not be serialized!
   void Pickle(base::Pickle* pickle) const;
 
-  // Deserialize the script from a pickle. Note that this always succeeds
+  // Deserializes the script from a pickle. Note that this always succeeds
   // because presumably we were the one that pickled it, and we did it
   // correctly.
   void Unpickle(const base::Pickle& pickle, base::PickleIterator* iter);
 
- private:
   // base::Pickle helper functions used to pickle the individual types of
   // components.
   void PickleGlobs(base::Pickle* pickle,
@@ -241,7 +186,6 @@ class UserScript {
   void PickleHostID(base::Pickle* pickle, const HostID& host_id) const;
   void PickleURLPatternSet(base::Pickle* pickle,
                            const URLPatternSet& pattern_list) const;
-  void PickleScripts(base::Pickle* pickle, const FileList& scripts) const;
 
   // Unpickle helper functions used to unpickle individual types of components.
   void UnpickleGlobs(const base::Pickle& pickle,
@@ -253,12 +197,9 @@ class UserScript {
   void UnpickleURLPatternSet(const base::Pickle& pickle,
                              base::PickleIterator* iter,
                              URLPatternSet* pattern_list);
-  void UnpickleScripts(const base::Pickle& pickle,
-                       base::PickleIterator* iter,
-                       FileList* scripts);
 
   // The location to run the script inside the document.
-  RunLocation run_location_;
+  UserScript::RunLocation run_location_;
 
   // The namespace of the script. This is used by Greasemonkey in the same way
   // as XML namespaces. Only used when parsing Greasemonkey-style scripts.
@@ -284,18 +225,12 @@ class UserScript {
   URLPatternSet url_set_;
   URLPatternSet exclude_url_set_;
 
-  // List of js scripts defined in content_scripts
-  FileList js_scripts_;
-
-  // List of css scripts defined in content_scripts
-  FileList css_scripts_;
-
   // The ID of the host this script is a part of. The |ID| of the
   // |host_id| can be empty if the script is a "standlone" user script.
   HostID host_id_;
 
   // The type of the consumer instance that the script will be injected.
-  ConsumerInstanceType consumer_instance_type_;
+  UserScript::ConsumerInstanceType consumer_instance_type_;
 
   // The globally-unique id associated with this user script. Defaults to
   // -1 for invalid.
@@ -315,9 +250,112 @@ class UserScript {
 
   // True if the script should be injected into an incognito tab.
   bool incognito_enabled_;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ExtensionUserScriptTest, Pickle);
+};
+
+// Represents a set of JS/CSS files for a user script.
+template <typename FileType>
+class UserScriptFiles {
+ public:
+  UserScriptFiles() {}
+  virtual ~UserScriptFiles() {}
+
+  using FileListType = std::vector<std::unique_ptr<FileType>>;
+
+  FileListType& js_scripts() { return js_scripts_; }
+  const FileListType& js_scripts() const { return js_scripts_; }
+  FileListType& css_scripts() { return css_scripts_; }
+  const FileListType& css_scripts() const { return css_scripts_; }
+
+  void PickleFiles(base::Pickle* pickle) const {
+    PickleFiles(js_scripts_, pickle);
+    PickleFiles(css_scripts_, pickle);
+  }
+
+  void UnpickleFiles(const base::Pickle& pickle, base::PickleIterator* iter) {
+    UnpickleFiles(pickle, iter, &js_scripts_);
+    UnpickleFiles(pickle, iter, &css_scripts_);
+  }
+
+ protected:
+  std::vector<std::unique_ptr<FileType>> js_scripts_;
+  std::vector<std::unique_ptr<FileType>> css_scripts_;
+
+ private:
+  void PickleFiles(const std::vector<std::unique_ptr<FileType>>& scripts,
+                   base::Pickle* pickle) const {
+    pickle->WriteUInt32(scripts.size());
+    for (const std::unique_ptr<FileType>& file : scripts)
+      file->Pickle(pickle);
+  }
+
+  void UnpickleFiles(const base::Pickle& pickle,
+                     base::PickleIterator* iter,
+                     std::vector<std::unique_ptr<FileType>>* scripts) {
+    uint32_t num_files = 0;
+    CHECK(iter->ReadUInt32(&num_files));
+    scripts->clear();
+    scripts->reserve(num_files);
+    for (uint32_t i = 0; i < num_files; ++i) {
+      std::unique_ptr<FileType> file(new FileType());
+      file->Unpickle(pickle, iter);
+      scripts->push_back(std::move(file));
+    }
+  }
+};
+
+// Holds information about a single script file: JS or CSS.
+// Does not contain any file content.
+class UserScriptFileInfo {

[Devlin, 2016/08/11 17:05:54]

(possibly see comment in extensions/browser/browser_user_script.h first.)

So, basically what we have now is:
UserScript - Some static stuff
UserScriptInfo - Info about the script
UserScriptFiles - bulk operations for a list of files

BrowserScriptFile - UserScriptFile with owned content
BrowserUserScript - UserScript + BrowserScriptFiles
RendererScriptFile - UserScriptFile with unowned content
RendererUserScript - UserScript + RendererScriptFiles
UserScriptFileInfo - Metadata about a file
ScriptMetadata - UserScript + UserScriptFileInfo

This seems excessive to me - I think we can cut down on this a lot.

What if we instead had a UserScript object, which still had a list of files, and
made the files themselves the only subclassed thing?  (basically, composition
over inheritance.)  Then we can have file implementations for metadata, owned
content, and external content, but we only have a single implementation of
UserScript, which has a list of those files.  We could then also, I think, get
rid of the UserScriptFiles class, and just keep its minor pickling logic in
UserScript (with the user script file classes having a Pickle() method as they
do now).  I think this would also get rid of all templating here.

So, something like:
UserScript {
  ...
  std::vector<unique_ptr<UserScriptFile>> files;
}

UserScriptFile {
  Pickle() = 0;
  Unpickle() = 0;
}

OwnedContentScriptFile {
  Pickle();
  Unpickle();
  std::string content;
}

UnownedContentScriptFile {
  Pickle();
  Unpickle();
  base::StringPiece content;
}

WDYT?  Happy to talk more over VC if it would help.

+ public:
+  UserScriptFileInfo(const base::FilePath& extension_root,
+                     const base::FilePath& relative_path,
+                     const GURL& url);
+  UserScriptFileInfo(const UserScriptFileInfo& other);
+  UserScriptFileInfo();
+  virtual ~UserScriptFileInfo();
+
+  const base::FilePath& extension_root() const { return extension_root_; }
+  const base::FilePath& relative_path() const { return relative_path_; }
+
+  const GURL& url() const { return url_; }
+  void set_url(const GURL& url) { url_ = url; }
+
+ protected:
+  void Pickle(base::Pickle* pickle) const;
+  void Unpickle(const base::Pickle& pickle, base::PickleIterator* iter);
+
+  // Where the script file lives on the disk. We keep the path split so that
+  // it can be localized at will.
+  base::FilePath extension_root_;
+  base::FilePath relative_path_;
+
+  // The url to this scipt file.
+  GURL url_;
+
+ private:
+  template <typename FileType>
+  friend class UserScriptFiles;
+
+  FRIEND_TEST_ALL_PREFIXES(ExtensionUserScriptTest, Pickle);
+};
+
+// Contains all metadata about extension's user script(s).
+// e.g. it will contain information about the script itself and a list of file
+// urls/paths for that script.
+class ScriptMetadata : public UserScriptInfo,
+                       public UserScriptFiles<UserScriptFileInfo> {

[Devlin, 2016/08/11 17:05:54]

I think style disallows multiple inheritance of implemented classes, which this
violates.

+ public:
+  ScriptMetadata();
+  ScriptMetadata(const UserScriptInfo& meta);
+  ~ScriptMetadata() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScriptMetadata);
 };
 
-// Information we need while removing scripts from a UserScriptLoader.
 struct UserScriptIDPair {
   UserScriptIDPair(int id, const HostID& host_id);
   UserScriptIDPair(int id);
@@ -326,12 +364,9 @@ struct UserScriptIDPair {
   HostID host_id;
 };
 
-// For storing UserScripts with unique IDs in sets.
-bool operator<(const UserScript& script1, const UserScript& script2);
-
 bool operator<(const UserScriptIDPair& a, const UserScriptIDPair& b);
 
-typedef std::vector<UserScript> UserScriptList;
+using ScriptMetadataList = std::vector<std::unique_ptr<ScriptMetadata>>;
 
 }  // namespace extensions