Extensions: Make InstallWarning moveable and non-copyable.

This CL adds move constructors and assignment operator for the InstallWarning struct and makes it non-copyable. Current usages of the copy constructor are changed to use move-semantics. BUG=None Change-Id: I0d3c7029d7418075beaaa8237f6643e13ac235dd Reviewed-on: https://chromium-review.googlesource.com/c/1270477 Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#598127}
FairyWorld · Oct 9, 2018 · 37927c8 · 37927c8
1 parent 231fbf9
commit 37927c8
Show file tree

Hide file tree

Showing 16 changed files with 35 additions and 21 deletions.
diff --git a/chrome/browser/extensions/unpacked_installer.cc b/chrome/browser/extensions/unpacked_installer.cc
@@ -286,7 +286,7 @@ bool UnpackedInstaller::IndexAndPersistRulesIfNeeded(std::string* error) {
 
   dnr_ruleset_checksum_ = result.ruleset_checksum;
   if (!result.warnings.empty())
-    extension_->AddInstallWarnings(result.warnings);
+    extension_->AddInstallWarnings(std::move(result.warnings));
 
   return true;
 }

diff --git a/chrome/common/extensions/permissions/permissions_data_unittest.cc b/chrome/common/extensions/permissions/permissions_data_unittest.cc
@@ -510,7 +510,7 @@ TEST_F(ExtensionScriptAndCaptureVisibleTest, Permissions) {
                                     "extension_wildcard_chrome.json",
                                     Manifest::INTERNAL, Extension::NO_FLAGS,
                                     &error);
-  std::vector<InstallWarning> warnings = extension->install_warnings();
+  const std::vector<InstallWarning>& warnings = extension->install_warnings();
   EXPECT_FALSE(warnings.empty());
   EXPECT_EQ(ErrorUtils::FormatErrorMessage(
                 manifest_errors::kInvalidPermissionScheme,

diff --git a/extensions/browser/api/declarative_net_request/utils.cc b/extensions/browser/api/declarative_net_request/utils.cc
@@ -198,7 +198,7 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
   if (install_warning) {
     install_warning->key = manifest_keys::kDeclarativeNetRequestKey;
     install_warning->specific = manifest_keys::kDeclarativeRuleResourcesKey;
-    warnings->push_back(*install_warning);
+    warnings->push_back(std::move(*install_warning));
   }
 
   UMA_HISTOGRAM_TIMES(kIndexAndPersistRulesTimeHistogram, timer.Elapsed());

diff --git a/extensions/browser/sandboxed_unpacker.cc b/extensions/browser/sandboxed_unpacker.cc
@@ -464,7 +464,7 @@ void SandboxedUnpacker::ReadManifestDone(
     ReportUnpackExtensionFailed(error_msg);
     return;
   }
-  extension->AddInstallWarnings(warnings);
+  extension->AddInstallWarnings(std::move(warnings));
 
   UnpackExtensionSucceeded(std::move(manifest_dict));
 }
@@ -702,7 +702,7 @@ void SandboxedUnpacker::OnJSONRulesetIndexed(
     declarative_net_request::IndexAndPersistRulesResult result) {
   if (result.success) {
     if (!result.warnings.empty())
-      extension_->AddInstallWarnings(result.warnings);
+      extension_->AddInstallWarnings(std::move(result.warnings));
     ReportSuccess(std::move(manifest), result.ruleset_checksum);
     return;
   }

diff --git a/extensions/common/csp_validator.cc b/extensions/common/csp_validator.cc
@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <initializer_list>
+#include <iterator>
 #include <vector>
 
 #include "base/bind.h"
@@ -463,8 +464,10 @@ std::string CSPEnforcer::Enforce(const std::string& policy,
         // This |status| falls back to "default-src". So warnings from
         // "default-src" will apply.
         if (warnings) {
-          warnings->insert(warnings->end(), default_src_csp_warnings.begin(),
-                           default_src_csp_warnings.end());
+          warnings->insert(
+              warnings->end(),
+              std::make_move_iterator(default_src_csp_warnings.begin()),
+              std::make_move_iterator(default_src_csp_warnings.end()));
         }
         break;
       }

diff --git a/extensions/common/extension.cc b/extensions/common/extension.cc
@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include <algorithm>
+#include <iterator>
 #include <utility>
 
 #include "base/base64.h"
@@ -428,14 +429,14 @@ const std::string Extension::GetVersionForDisplay() const {
   return VersionString();
 }
 
-void Extension::AddInstallWarning(const InstallWarning& new_warning) {
-  install_warnings_.push_back(new_warning);
+void Extension::AddInstallWarning(InstallWarning new_warning) {
+  install_warnings_.push_back(std::move(new_warning));
 }
 
-void Extension::AddInstallWarnings(
-    const std::vector<InstallWarning>& new_warnings) {
+void Extension::AddInstallWarnings(std::vector<InstallWarning> new_warnings) {
   install_warnings_.insert(install_warnings_.end(),
-                           new_warnings.begin(), new_warnings.end());
+                           std::make_move_iterator(new_warnings.begin()),
+                           std::make_move_iterator(new_warnings.end()));
 }
 
 bool Extension::is_app() const {

diff --git a/extensions/common/extension.h b/extensions/common/extension.h
@@ -274,8 +274,8 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   }
 
   // Appends |new_warning[s]| to install_warnings_.
-  void AddInstallWarning(const InstallWarning& new_warning);
-  void AddInstallWarnings(const std::vector<InstallWarning>& new_warnings);
+  void AddInstallWarning(InstallWarning new_warning);
+  void AddInstallWarnings(std::vector<InstallWarning> new_warnings);
   const std::vector<InstallWarning>& install_warnings() const {
     return install_warnings_;
   }

diff --git a/extensions/common/file_util.cc b/extensions/common/file_util.cc
@@ -225,7 +225,7 @@ scoped_refptr<Extension> LoadExtension(const base::FilePath& extension_path,
   std::vector<InstallWarning> warnings;
   if (!ValidateExtension(extension.get(), error, &warnings))
     return NULL;
-  extension->AddInstallWarnings(warnings);
+  extension->AddInstallWarnings(std::move(warnings));
 
   return extension;
 }

diff --git a/extensions/common/install_warning.cc b/extensions/common/install_warning.cc
@@ -20,6 +20,10 @@ InstallWarning::InstallWarning(const std::string& message,
     : message(message), key(key), specific(specific) {
 }
 
+InstallWarning::InstallWarning(InstallWarning&& other) = default;
+
+InstallWarning& InstallWarning::operator=(InstallWarning&& other) = default;
+
 InstallWarning::~InstallWarning() {
 }
 

diff --git a/extensions/common/install_warning.h b/extensions/common/install_warning.h
@@ -5,6 +5,8 @@
 #ifndef EXTENSIONS_COMMON_INSTALL_WARNING_H_
 #define EXTENSIONS_COMMON_INSTALL_WARNING_H_
 
+#include "base/macros.h"
+
 #include <ostream>
 #include <string>
 
@@ -19,6 +21,8 @@ struct InstallWarning {
   InstallWarning(const std::string& message,
                  const std::string& key,
                  const std::string& specific);
+  InstallWarning(InstallWarning&& other);
+  InstallWarning& operator=(InstallWarning&& other);
   ~InstallWarning();
 
   bool operator==(const InstallWarning& other) const {
@@ -38,6 +42,8 @@ struct InstallWarning {
   // Optional - for specifying the incorrect portion of a key in the manifest
   // (e.g., an unrecognized permission "foo" in "permissions").
   std::string specific;
+
+  DISALLOW_COPY(InstallWarning);
 };
 
 // Let gtest print InstallWarnings.

diff --git a/extensions/common/manifest_handlers/automation.cc b/extensions/common/manifest_handlers/automation.cc
@@ -168,7 +168,7 @@ bool AutomationHandler::Parse(Extension* extension, base::string16* error) {
   if (!error->empty())
     return false;
 
-  extension->AddInstallWarnings(install_warnings);
+  extension->AddInstallWarnings(std::move(install_warnings));
 
   if (!info)
     return true;

diff --git a/extensions/common/manifest_handlers/csp_info.cc b/extensions/common/manifest_handlers/csp_info.cc
@@ -138,7 +138,7 @@ bool CSPHandler::Parse(Extension* extension, base::string16* error) {
   std::vector<InstallWarning> warnings;
   content_security_policy = SanitizeContentSecurityPolicy(
       content_security_policy, GetValidatorOptions(extension), &warnings);
-  extension->AddInstallWarnings(warnings);
+  extension->AddInstallWarnings(std::move(warnings));
 
   extension->SetManifestData(
       keys::kContentSecurityPolicy,

diff --git a/extensions/common/manifest_handlers/externally_connectable.cc b/extensions/common/manifest_handlers/externally_connectable.cc
@@ -79,7 +79,7 @@ bool ExternallyConnectableHandler::Parse(Extension* extension,
     PermissionsParser::AddAPIPermission(extension,
                                         APIPermission::kWebConnectable);
   }
-  extension->AddInstallWarnings(install_warnings);
+  extension->AddInstallWarnings(std::move(install_warnings));
   extension->SetManifestData(keys::kExternallyConnectable, std::move(info));
   return true;
 }

diff --git a/extensions/common/manifest_handlers/file_handler_info.cc b/extensions/common/manifest_handlers/file_handler_info.cc
@@ -206,7 +206,7 @@ bool FileHandlersParser::Parse(Extension* extension, base::string16* error) {
   }
 
   extension->SetManifestData(keys::kFileHandlers, std::move(info));
-  extension->AddInstallWarnings(install_warnings);
+  extension->AddInstallWarnings(std::move(install_warnings));
   return true;
 }
 

diff --git a/extensions/common/manifest_handlers/options_page_info.cc b/extensions/common/manifest_handlers/options_page_info.cc
@@ -197,7 +197,7 @@ bool OptionsPageManifestHandler::Parse(Extension* extension,
   if (!info)
     return false;
 
-  extension->AddInstallWarnings(install_warnings);
+  extension->AddInstallWarnings(std::move(install_warnings));
   extension->SetManifestData(keys::kOptionsUI, std::move(info));
   return true;
 }

diff --git a/extensions/common/manifest_handlers/sandboxed_page_info.cc b/extensions/common/manifest_handlers/sandboxed_page_info.cc
@@ -113,7 +113,7 @@ bool SandboxedPageHandler::Parse(Extension* extension, base::string16* error) {
     sandboxed_info->content_security_policy =
         csp_validator::GetEffectiveSandoxedPageCSP(content_security_policy,
                                                    &warnings);
-    extension->AddInstallWarnings(warnings);
+    extension->AddInstallWarnings(std::move(warnings));
   } else {
     sandboxed_info->content_security_policy =
         kDefaultSandboxedPageContentSecurityPolicy;