From 37927c86f55f6e9c06977767176e481570928d67 Mon Sep 17 00:00:00 2001 From: Karan Bhatia Date: Tue, 9 Oct 2018 23:21:26 +0000 Subject: [PATCH] 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 Reviewed-by: Devlin Cr-Commit-Position: refs/heads/master@{#598127} --- chrome/browser/extensions/unpacked_installer.cc | 2 +- .../permissions/permissions_data_unittest.cc | 2 +- .../browser/api/declarative_net_request/utils.cc | 2 +- extensions/browser/sandboxed_unpacker.cc | 4 ++-- extensions/common/csp_validator.cc | 7 +++++-- extensions/common/extension.cc | 11 ++++++----- extensions/common/extension.h | 4 ++-- extensions/common/file_util.cc | 2 +- extensions/common/install_warning.cc | 4 ++++ extensions/common/install_warning.h | 6 ++++++ extensions/common/manifest_handlers/automation.cc | 2 +- extensions/common/manifest_handlers/csp_info.cc | 2 +- .../manifest_handlers/externally_connectable.cc | 2 +- .../common/manifest_handlers/file_handler_info.cc | 2 +- .../common/manifest_handlers/options_page_info.cc | 2 +- .../common/manifest_handlers/sandboxed_page_info.cc | 2 +- 16 files changed, 35 insertions(+), 21 deletions(-) diff --git a/chrome/browser/extensions/unpacked_installer.cc b/chrome/browser/extensions/unpacked_installer.cc index 0546017b71856e..b4b02f4bb061b1 100644 --- 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 index 941bf38bb7bac1..bfcb8648943185 100644 --- 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 warnings = extension->install_warnings(); + const std::vector& 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 index ea06174b0aa3a5..7fbad120d143bc 100644 --- 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 index 89820286e50354..7abab6753abd9b 100644 --- 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 index 112e2bc5864b22..db3a6aa71f639e 100644 --- a/extensions/common/csp_validator.cc +++ b/extensions/common/csp_validator.cc @@ -7,6 +7,7 @@ #include #include +#include #include #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 index 711b93a770b867..06da0fc816df43 100644 --- a/extensions/common/extension.cc +++ b/extensions/common/extension.cc @@ -7,6 +7,7 @@ #include #include +#include #include #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& new_warnings) { +void Extension::AddInstallWarnings(std::vector 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 index 470221f8474614..a99bf4c83b634f 100644 --- a/extensions/common/extension.h +++ b/extensions/common/extension.h @@ -274,8 +274,8 @@ class Extension : public base::RefCountedThreadSafe { } // Appends |new_warning[s]| to install_warnings_. - void AddInstallWarning(const InstallWarning& new_warning); - void AddInstallWarnings(const std::vector& new_warnings); + void AddInstallWarning(InstallWarning new_warning); + void AddInstallWarnings(std::vector new_warnings); const std::vector& install_warnings() const { return install_warnings_; } diff --git a/extensions/common/file_util.cc b/extensions/common/file_util.cc index e698124d6b9994..da14e55ee78868 100644 --- a/extensions/common/file_util.cc +++ b/extensions/common/file_util.cc @@ -225,7 +225,7 @@ scoped_refptr LoadExtension(const base::FilePath& extension_path, std::vector 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 index 3db5a5be077ef3..01f79383953156 100644 --- 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 index ca79477ef03a8d..be1937edb184d0 100644 --- 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 #include @@ -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 index 7140f99e8068a8..1d076c34e466cd 100644 --- 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 index d95aa993352f63..07237f52402c33 100644 --- 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 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 index c56cc27137017c..cd1313bbd47caf 100644 --- 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 index 9a9ea9ce85bc14..e37b8f2bb3ea98 100644 --- 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 index f4227042c0838e..87ab51d86f4138 100644 --- 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 index defa0985aebc44..f29b3197bbb235 100644 --- 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;