[Extensions] Use rvalue references for SimpleFeature setters

extensions/common/features/simple_feature.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 EXTENSIONS_COMMON_FEATURES_SIMPLE_FEATURE_H_
 #define EXTENSIONS_COMMON_FEATURES_SIMPLE_FEATURE_H_
 
 #include <stddef.h>
 
 #include <memory>
@@ skipping 81 matching lines @@
   // supported in feature files. These should only be used in this class and in
   // generated files.
   enum Location {
     UNSPECIFIED_LOCATION,
     COMPONENT_LOCATION,
     EXTERNAL_COMPONENT_LOCATION,
     POLICY_LOCATION,
   };
 
   // Setters used by generated code to create the feature.
-  void set_blacklist(const std::vector<std::string>& blacklist) {
-    blacklist_ = blacklist;
-  }
+  // NOTE: These setters use pass-by-value DELIBERATELY. Since these setters
+  // should only be used in generated code, we can ensure that in each call, the
+  // new value is constructed in place, thus allowing us to std::move() it
+  // directly into the feature. This saves a copy, and results in noticable
+  // improvements in both speed and binary size.
+  void set_blacklist(std::vector<std::string> blacklist);
   void set_channel(version_info::Channel channel) {
     channel_.reset(new version_info::Channel(channel));
   }
-  void set_command_line_switch(const std::string& command_line_switch) {
-    command_line_switch_ = command_line_switch;
-  }
+  void set_command_line_switch(std::string command_line_switch);

[lazyboy, 2016/07/22 20:58:23]

(trying to understand stuff here, so this is probably naive q)
Why not std::string&& command_line_switch if we know the param is constructed in
place?
The advantage of that is someone outside of generated code cannot use l-value
anymore (including test):
string s;
...
set_command_line_switch(s)

Now, style guide says "only use these (rvalue) to define move constructors and
move assignment operators, and for perfect forwarding." So, that makes it no-go.
Is this the only reason to not use it?

[Devlin, 2016/07/22 21:27:35]

Interesting idea!  Yes, style is the only reason not to use it.  WDYT?  Worth
breaking the style rule?

   void set_component_extensions_auto_granted(bool granted) {
     component_extensions_auto_granted_ = granted;
   }
-  void set_contexts(const std::vector<Context>& contexts) {
-    contexts_ = contexts;
-  }
-  void set_dependencies(const std::vector<std::string>& dependencies) {
-    dependencies_ = dependencies;
-  }
-  void set_extension_types(const std::vector<Manifest::Type> types) {
-    extension_types_ = types;
-  }
+  void set_contexts(std::vector<Context> contexts);
+  void set_dependencies(std::vector<std::string> dependencies);
+  void set_extension_types(std::vector<Manifest::Type> types);
   void set_internal(bool is_internal) { is_internal_ = is_internal; }
   void set_location(Location location) { location_ = location; }
+  // set_matches() is an exception to pass-by-value since we construct an
+  // URLPatternSet from the vector of strings.
+  // TODO(devlin): Pass in an URLPatternSet directly.
   void set_matches(const std::vector<std::string>& matches);
   void set_max_manifest_version(int max_manifest_version) {
     max_manifest_version_ = max_manifest_version;
   }
   void set_min_manifest_version(int min_manifest_version) {
     min_manifest_version_ = min_manifest_version;
   }
-  void set_platforms(const std::vector<Platform>& platforms) {
-    platforms_ = platforms;
-  }
-  void set_whitelist(const std::vector<std::string>& whitelist) {
-    whitelist_ = whitelist;
-  }
+  void set_platforms(std::vector<Platform> platforms);
+  void set_whitelist(std::vector<std::string> whitelist);
 
  protected:
   // Accessors used by subclasses in feature verification.
   const std::vector<std::string>& blacklist() const { return blacklist_; }
   const std::vector<std::string>& whitelist() const { return whitelist_; }
   const std::vector<Manifest::Type>& extension_types() const {
     return extension_types_;
   }
   const std::vector<Platform>& platforms() const { return platforms_; }
   const std::vector<Context>& contexts() const { return contexts_; }
@@ skipping 89 matching lines @@
   bool is_internal_;
   std::string command_line_switch_;
   std::unique_ptr<version_info::Channel> channel_;
 
   DISALLOW_COPY_AND_ASSIGN(SimpleFeature);
 };
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_COMMON_FEATURES_SIMPLE_FEATURE_H_