Make UserScript non-copyable.

extensions/common/user_script.h

 // Copyright 2013 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 EXTENSIONS_COMMON_USER_SCRIPT_H_
 #define EXTENSIONS_COMMON_USER_SCRIPT_H_
 
 #include <string>
 #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"
 #include "extensions/common/url_pattern_set.h"
 #include "url/gurl.h"
 
 namespace base {
 class Pickle;
 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.
   static const char kFileExtension[];
 
   static int GenerateUserScriptID();
 
   // Check if a URL should be treated as a user script and converted to an
   // extension.
   static bool IsURLUserScript(const GURL& url, const std::string& mime_type);
@@ skipping 29 matching lines @@
                     // is "idle". Currently this uses the simple heuristic of:
                     // min(DOM_CONTENT_LOADED + TIMEOUT, ONLOAD), but no
                     // particular injection point is guaranteed.
     RUN_DEFERRED,  // The user script's injection was deferred for permissions
                    // reasons, and was executed at a later time.
     BROWSER_DRIVEN,  // The user script will be injected when triggered by an
                      // IPC in the browser process.
     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) {
     name_space_ = name_space;
   }
 
   const std::string& name() const { return name_; }
   void set_name(const std::string& name) { name_ = name; }
 
   const std::string& version() const { return version_; }
   void set_version(const std::string& version) {
     version_ = version;
   }
 
   const std::string& description() const { return description_; }
   void set_description(const std::string& description) {
     description_ = description;
   }
 
   // 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_; }
   void set_emulate_greasemonkey(bool val) { emulate_greasemonkey_ = val; }
 
   // Whether to match all frames, or only the top one.
   bool match_all_frames() const { return match_all_frames_; }
   void set_match_all_frames(bool val) { match_all_frames_ = val; }
 
   // Whether to match about:blank and about:srcdoc.
@@ skipping 15 matching lines @@
 
   // The URLPatterns, if any, that determine which pages this script runs
   // against.
   const URLPatternSet& url_patterns() const { return url_set_; }
   void add_url_pattern(const URLPattern& pattern);
   const URLPatternSet& exclude_url_patterns() const {
     return exclude_url_set_;
   }
   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;
   }
 
   int id() const { return user_script_id_; }
   void set_id(int id) { user_script_id_ = id; }
 
   bool is_incognito_enabled() const { return incognito_enabled_; }
   void set_incognito_enabled(bool enabled) { incognito_enabled_ = enabled; }
 
   // Returns true if the script should be applied to the specified URL, false
   // 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,
                    const std::vector<std::string>& globs) const;
   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,
                      base::PickleIterator* iter,
                      std::vector<std::string>* globs);
   void UnpickleHostID(const base::Pickle& pickle,
                       base::PickleIterator* iter,
                       HostID* host_id);
   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.
   std::string name_space_;
 
   // The script's name. Only used when parsing Greasemonkey-style scripts.
   std::string name_;
 
   // A longer description. Only used when parsing Greasemonkey-style scripts.
   std::string description_;
 
   // A version number of the script. Only used when parsing Greasemonkey-style
   // scripts.
   std::string version_;
 
   // Greasemonkey-style globs that determine pages to inject the script into.
   // These are only used with standalone scripts.
   std::vector<std::string> globs_;
   std::vector<std::string> exclude_globs_;
 
   // URLPatterns that determine pages to inject the script into. These are
   // only used with scripts that are part of extensions.
   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.
   int user_script_id_;
 
   // Whether we should try to emulate Greasemonkey's APIs when running this
   // script.
   bool emulate_greasemonkey_;
 
   // Whether the user script should run in all frames, or only just the top one.
   // Defaults to false.
   bool match_all_frames_;
 
   // Whether the user script should run in about:blank and about:srcdoc as well.
   // Defaults to false.
   bool match_about_blank_;
 
   // True if the script should be injected into an incognito tab.
   bool incognito_enabled_;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ExtensionUserScriptTest, Pickle);
 };
 
-// Information we need while removing scripts from a UserScriptLoader.
+// 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);
+};
+
 struct UserScriptIDPair {
   UserScriptIDPair(int id, const HostID& host_id);
   UserScriptIDPair(int id);
 
   int id;
   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
 
 #endif  // EXTENSIONS_COMMON_USER_SCRIPT_H_