[Extensions] Make ExternalInstallInfo less pointer-y

Remove the std::unique_ptr<> members from ExternalInstallInfoFile and
ExternalInstallInfoUpdateUrl. These were used to hold the version and
url members, but each of these will never be null and are cheaply 
copyable (or, in the case of GURL, movable), so have no reason to
be pointers.

Bug: 770007
Change-Id: I4b9c3ef8c9888a2d460ab082a9fa3339423a5ee6
Reviewed-on: https://chromium-review.googlesource.com/691045
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505398}

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -392,7 +392,7 @@ class TestExternalProvider : public ExternalProviderInterface {
     visitor_->OnExternalExtensionUpdateUrlFound(
         ExternalInstallInfoUpdateUrl(
             extension_id_, std::string() /* install_parameter */,
-            base::MakeUnique<GURL>(extension_urls::GetWebstoreUpdateUrl()),
+            extension_urls::GetWebstoreUpdateUrl(),
             Manifest::EXTERNAL_POLICY_DOWNLOAD, 0 /* creation_flags */,
             true /* mark_acknowledged */),
         true /* is_initial_load */);

chrome/browser/extensions/extension_service.cc

@@ -236,7 +236,7 @@ bool ExtensionService::OnExternalExtensionUpdateUrlFound(
   // install source.  In this case, signal that this extension will not be
   // installed by returning false.
   if (!pending_extension_manager()->AddFromExternalUpdateUrl(
-          info.extension_id, info.install_parameter, *info.update_url,
+          info.extension_id, info.install_parameter, info.update_url,
           info.download_location, info.creation_flags,
           info.mark_acknowledged)) {
     return false;
@@ -2058,9 +2058,7 @@ bool ExtensionService::OnExternalExtensionFileFound(
          Manifest::IsExternalLocation(existing->location()));
 
     if (!is_default_apps_migration) {
-      DCHECK(info.version.get());
-
-      switch (existing->version()->CompareTo(*info.version)) {
+      switch (existing->version()->CompareTo(info.version)) {
         case -1:  // existing version is older, we should upgrade
           break;
         case 0:  // existing version is same, do nothing
@@ -2070,7 +2068,7 @@ bool ExtensionService::OnExternalExtensionFileFound(
                        << info.extension_id
                        << "that is older than current version. Current version "
                        << "is: " << existing->VersionString() << ". New "
-                       << "version is: " << info.version->GetString()
+                       << "version is: " << info.version.GetString()
                        << ". Keeping current version.";
           return false;
       }
@@ -2079,7 +2077,7 @@ bool ExtensionService::OnExternalExtensionFileFound(
 
   // If the extension is already pending, don't start an install.
   if (!pending_extension_manager()->AddFromExternalFile(
-          info.extension_id, info.crx_location, *info.version,
+          info.extension_id, info.crx_location, info.version,
           info.creation_flags, info.mark_acknowledged)) {
     return false;
   }
@@ -2088,7 +2086,7 @@ bool ExtensionService::OnExternalExtensionFileFound(
   scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(this));
   installer->set_install_source(info.crx_location);
   installer->set_expected_id(info.extension_id);
-  installer->set_expected_version(*info.version,
+  installer->set_expected_version(info.version,
                                   true /* fail_install_if_unexpected */);
   installer->set_install_cause(extension_misc::INSTALL_CAUSE_EXTERNAL_FILE);
   installer->set_install_immediately(info.install_immediately);

chrome/browser/extensions/extension_service_unittest.cc

@@ -277,8 +277,8 @@ std::unique_ptr<ExternalInstallInfoFile> CreateExternalExtension(
     Manifest::Location location,
     Extension::InitFromValueFlags flags) {
   return base::MakeUnique<ExternalInstallInfoFile>(
-      extension_id, base::MakeUnique<base::Version>(version_str), path,
-      location, flags, false, false);
+      extension_id, base::Version(version_str), path, location, flags, false,
+      false);
 }
 
 // Helper function to persist the passed directories and file paths in
@@ -422,13 +422,13 @@ class MockProviderVisitor
       base::FilePath crx_path;
 
       EXPECT_TRUE(provider_->GetExtensionDetails(info.extension_id, NULL, &v1));
-      EXPECT_STREQ(info.version->GetString().c_str(), v1->GetString().c_str());
+      EXPECT_EQ(info.version.GetString(), v1->GetString());
 
       std::unique_ptr<base::Version> v2;
       EXPECT_TRUE(
           provider_->GetExtensionDetails(info.extension_id, &location, &v2));
-      EXPECT_STREQ(info.version->GetString().c_str(), v1->GetString().c_str());
-      EXPECT_STREQ(info.version->GetString().c_str(), v2->GetString().c_str());
+      EXPECT_EQ(info.version.GetString(), v1->GetString());
+      EXPECT_EQ(info.version.GetString(), v2->GetString());
       EXPECT_EQ(crx_location_, location);
 
       // Remove it so we won't count it ever again.
@@ -4487,12 +4487,12 @@ TEST_F(ExtensionServiceTest, MAYBE_UpdatingPendingExternalExtensionWithFlags) {
   base::FilePath path = data_dir().AppendASCII("good.crx");
 
   // Register and install an external extension.
-  std::unique_ptr<base::Version> version(new base::Version("1.0.0.0"));
+  base::Version version("1.0.0.0");
   content::WindowedNotificationObserver observer(
       extensions::NOTIFICATION_CRX_INSTALLER_DONE,
       content::NotificationService::AllSources());
   std::unique_ptr<ExternalInstallInfoFile> info(new ExternalInstallInfoFile(
-      good_crx, std::move(version), path, Manifest::EXTERNAL_PREF,
+      good_crx, version, path, Manifest::EXTERNAL_PREF,
       Extension::FROM_BOOKMARK, false /* mark_acknowledged */,
       false /* install_immediately */));
   ASSERT_TRUE(service()->OnExternalExtensionFileFound(*info));
@@ -5988,7 +5988,7 @@ TEST_F(ExtensionServiceTest, InstallPriorityExternalUpdateUrl) {
   EXPECT_FALSE(pending->IsIdPending(kGoodId));
 
   // Skip install when the location is the same.
-  std::unique_ptr<GURL> good_update_url(new GURL(kGoodUpdateURL));
+  GURL good_update_url(kGoodUpdateURL);
   std::unique_ptr<ExternalInstallInfoUpdateUrl> info(
       new ExternalInstallInfoUpdateUrl(
           kGoodId, std::string(), std::move(good_update_url),
@@ -6051,12 +6051,9 @@ TEST_F(ExtensionServiceTest, InstallPriorityExternalLocalFile) {
       service()->pending_extension_manager();
   EXPECT_FALSE(pending->IsIdPending(kGoodId));
 
-  std::unique_ptr<base::Version> older_version_ptr(
-      new base::Version(older_version));
   std::unique_ptr<ExternalInstallInfoFile> info(new ExternalInstallInfoFile(
-      kGoodId, std::move(older_version_ptr), kInvalidPathToCrx,
-      Manifest::INTERNAL, kCreationFlags, kDontMarkAcknowledged,
-      kDontInstallImmediately));
+      kGoodId, older_version, kInvalidPathToCrx, Manifest::INTERNAL,
+      kCreationFlags, kDontMarkAcknowledged, kDontInstallImmediately));
   {
     // Simulate an external source adding the extension as INTERNAL.
     content::WindowedNotificationObserver observer(
@@ -6133,29 +6130,29 @@ TEST_F(ExtensionServiceTest, InstallPriorityExternalLocalFile) {
   // older, or the same, and succeed if the version is newer.
 
   // Older than the installed version...
-  info->version.reset(new base::Version(older_version));
+  info->version = older_version;
   EXPECT_FALSE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_FALSE(pending->IsIdPending(kGoodId));
 
   // Same version as the installed version...
-  info->version.reset(new base::Version(ext->VersionString()));
+  info->version = *ext->version();
   EXPECT_FALSE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_FALSE(pending->IsIdPending(kGoodId));
 
   // Newer than the installed version...
-  info->version.reset(new base::Version(newer_version));
+  info->version = newer_version;
   EXPECT_TRUE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_TRUE(pending->IsIdPending(kGoodId));
 
   // An external install for a higher priority install source should succeed
   // if the version is greater.  |older_version| is not...
-  info->version.reset(new base::Version(older_version));
+  info->version = older_version;
   info->crx_location = Manifest::EXTERNAL_PREF;
   EXPECT_FALSE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_TRUE(pending->IsIdPending(kGoodId));
 
   // |newer_version| is newer.
-  info->version.reset(new base::Version(newer_version));
+  info->version = newer_version;
   EXPECT_TRUE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_TRUE(pending->IsIdPending(kGoodId));
 
@@ -6189,9 +6186,8 @@ TEST_F(ExtensionServiceTest, ConcurrentExternalLocalFile) {
 
   // An external provider starts installing from a local crx.
   std::unique_ptr<ExternalInstallInfoFile> info(new ExternalInstallInfoFile(
-      kGoodId, base::MakeUnique<base::Version>(kVersion123),
-      kInvalidPathToCrx, Manifest::EXTERNAL_PREF, kCreationFlags,
-      kDontMarkAcknowledged, kDontInstallImmediately));
+      kGoodId, kVersion123, kInvalidPathToCrx, Manifest::EXTERNAL_PREF,
+      kCreationFlags, kDontMarkAcknowledged, kDontInstallImmediately));
   EXPECT_TRUE(service()->OnExternalExtensionFileFound(*info));
 
   const extensions::PendingExtensionInfo* pending_info;
@@ -6200,14 +6196,14 @@ TEST_F(ExtensionServiceTest, ConcurrentExternalLocalFile) {
   EXPECT_EQ(pending_info->version(), kVersion123);
 
   // Adding a newer version overrides the currently pending version.
-  info->version.reset(new base::Version(kVersion124));
+  info->version = kVersion124;
   EXPECT_TRUE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_TRUE((pending_info = pending->GetById(kGoodId)));
   EXPECT_TRUE(pending_info->version().IsValid());
   EXPECT_EQ(pending_info->version(), kVersion124);
 
   // Adding an older version fails.
-  info->version.reset(new base::Version(kVersion123));
+  info->version = kVersion123;
   EXPECT_FALSE(service()->OnExternalExtensionFileFound(*info));
   EXPECT_TRUE((pending_info = pending->GetById(kGoodId)));
   EXPECT_TRUE(pending_info->version().IsValid());
@@ -6222,10 +6218,9 @@ TEST_F(ExtensionServiceTest, ConcurrentExternalLocalFile) {
   EXPECT_EQ(pending_info->version(), kVersion124);
 
   // Adding the latest version from the webstore overrides a specific version.
-  GURL kUpdateUrl("http://example.com/update");
   std::unique_ptr<ExternalInstallInfoUpdateUrl> update_info(
       new ExternalInstallInfoUpdateUrl(
-          kGoodId, std::string(), base::MakeUnique<GURL>(kUpdateUrl),
+          kGoodId, std::string(), GURL("http://example.com/update"),
           Manifest::EXTERNAL_POLICY_DOWNLOAD, Extension::NO_FLAGS, false));
   EXPECT_TRUE(service()->OnExternalExtensionUpdateUrlFound(*update_info, true));
   EXPECT_TRUE((pending_info = pending->GetById(kGoodId)));
@@ -6280,9 +6275,8 @@ class ExtensionSourcePriorityTest : public ExtensionServiceTest {
 
   // Fake an external file from external_extensions.json.
   bool AddPendingExternalPrefFileInstall() {
-    std::unique_ptr<base::Version> version(new base::Version("1.0.0.0"));
     std::unique_ptr<ExternalInstallInfoFile> info(new ExternalInstallInfoFile(
-        crx_id_, std::move(version), crx_path_, Manifest::EXTERNAL_PREF,
+        crx_id_, base::Version("1.0.0.0"), crx_path_, Manifest::EXTERNAL_PREF,
         Extension::NO_FLAGS, false, false));
     return service()->OnExternalExtensionFileFound(*info);
   }
@@ -6300,11 +6294,10 @@ class ExtensionSourcePriorityTest : public ExtensionServiceTest {
   // Fake a policy install.
   bool AddPendingPolicyInstall() {
     // Get path to the CRX with id |kGoodId|.
-    std::unique_ptr<GURL> empty_url(new GURL());
     std::unique_ptr<ExternalInstallInfoUpdateUrl> info(
-        new ExternalInstallInfoUpdateUrl(
-            crx_id_, std::string(), std::move(empty_url),
-            Manifest::EXTERNAL_POLICY_DOWNLOAD, Extension::NO_FLAGS, false));
+        new ExternalInstallInfoUpdateUrl(crx_id_, std::string(), GURL(),
+                                         Manifest::EXTERNAL_POLICY_DOWNLOAD,
+                                         Extension::NO_FLAGS, false));
     return service()->OnExternalExtensionUpdateUrlFound(*info, true);
   }
 

chrome/browser/extensions/external_provider_impl.cc

@@ -384,9 +384,8 @@ void ExternalProviderImpl::RetrieveExtensionsFromPrefs(
         path = base_path.Append(external_crx);
       }
 
-      std::unique_ptr<base::Version> version(
-          new base::Version(external_version));
-      if (!version->IsValid()) {
+      base::Version version(external_version);
+      if (!version.IsValid()) {
         LOG(WARNING) << "Malformed extension dictionary for extension: "
                      << extension_id.c_str() << ".  Invalid version string \""
                      << external_version << "\".";
@@ -394,8 +393,8 @@ void ExternalProviderImpl::RetrieveExtensionsFromPrefs(
       }
       external_file_extensions->push_back(
           base::MakeUnique<ExternalInstallInfoFile>(
-              extension_id, std::move(version), path, crx_location_,
-              creation_flags, auto_acknowledge_, install_immediately_));
+              extension_id, version, path, crx_location_, creation_flags,
+              auto_acknowledge_, install_immediately_));
     } else {  // if (has_external_update_url)
       CHECK(has_external_update_url);  // Checking of keys above ensures this.
       if (download_location_ == Manifest::INVALID_LOCATION) {
@@ -403,8 +402,8 @@ void ExternalProviderImpl::RetrieveExtensionsFromPrefs(
                      << "extensions from update URLs.";
         continue;
       }
-      std::unique_ptr<GURL> update_url(new GURL(external_update_url));
-      if (!update_url->is_valid()) {
+      GURL update_url(external_update_url);
+      if (!update_url.is_valid()) {
         LOG(WARNING) << "Malformed extension dictionary for extension: "
                      << extension_id.c_str() << ".  Key " << kExternalUpdateUrl
                      << " has value \"" << external_update_url

extensions/browser/external_install_info.cc

@@ -4,9 +4,6 @@
 
 #include "extensions/browser/external_install_info.h"
 
-#include "base/version.h"
-#include "url/gurl.h"
-
 namespace extensions {
 
 ExternalInstallInfo::ExternalInstallInfo(const std::string& extension_id,
@@ -18,14 +15,14 @@ ExternalInstallInfo::ExternalInstallInfo(const std::string& extension_id,
 
 ExternalInstallInfoFile::ExternalInstallInfoFile(
     const std::string& extension_id,
-    std::unique_ptr<base::Version> version,
+    const base::Version& version,
     const base::FilePath& path,
     Manifest::Location crx_location,
     int creation_flags,
     bool mark_acknowledged,
     bool install_immediately)
     : ExternalInstallInfo(extension_id, creation_flags, mark_acknowledged),
-      version(std::move(version)),
+      version(version),
       path(path),
       crx_location(crx_location),
       install_immediately(install_immediately) {}
@@ -35,7 +32,7 @@ ExternalInstallInfoFile::~ExternalInstallInfoFile() {}
 ExternalInstallInfoUpdateUrl::ExternalInstallInfoUpdateUrl(
     const std::string& extension_id,
     const std::string& install_parameter,
-    std::unique_ptr<GURL> update_url,
+    GURL update_url,
     Manifest::Location download_location,
     int creation_flags,
     bool mark_acknowledged)

extensions/browser/external_install_info.h

@@ -6,13 +6,9 @@
 #define EXTENSIONS_BROWSER_EXTERNAL_INSTALL_INFO_H_
 
 #include "base/files/file_path.h"
+#include "base/version.h"
 #include "extensions/common/manifest.h"
-
-class GURL;
-
-namespace base {
-class Version;
-}
+#include "url/gurl.h"
 
 namespace extensions {
 
@@ -34,7 +30,7 @@ struct ExternalInstallInfo {
 
 struct ExternalInstallInfoFile : public ExternalInstallInfo {
   ExternalInstallInfoFile(const std::string& extension_id,
-                          std::unique_ptr<base::Version> version,
+                          const base::Version& version,
                           const base::FilePath& path,
                           Manifest::Location crx_location,
                           int creation_flags,
@@ -42,7 +38,7 @@ struct ExternalInstallInfoFile : public ExternalInstallInfo {
                           bool install_immediately);
   ~ExternalInstallInfoFile() override;
 
-  std::unique_ptr<base::Version> version;
+  base::Version version;
   base::FilePath path;
   Manifest::Location crx_location;
   bool install_immediately;
@@ -51,14 +47,14 @@ struct ExternalInstallInfoFile : public ExternalInstallInfo {
 struct ExternalInstallInfoUpdateUrl : public ExternalInstallInfo {
   ExternalInstallInfoUpdateUrl(const std::string& extension_id,
                                const std::string& install_parameter,
-                               std::unique_ptr<GURL> update_url,
+                               GURL update_url,
                                Manifest::Location download_location,
                                int creation_flags,
                                bool mark_acknowledged);
   ~ExternalInstallInfoUpdateUrl() override;
 
   std::string install_parameter;
-  std::unique_ptr<GURL> update_url;
+  GURL update_url;
   Manifest::Location download_location;
 };
 

extensions/browser/mock_external_provider.cc

@@ -4,7 +4,8 @@
 
 #include "extensions/browser/mock_external_provider.h"
 
-#include "base/memory/ptr_util.h"
+#include <memory>
+
 #include "base/version.h"
 #include "extensions/browser/external_install_info.h"
 #include "extensions/common/extension.h"
@@ -20,10 +21,9 @@ MockExternalProvider::~MockExternalProvider() {}
 void MockExternalProvider::UpdateOrAddExtension(const ExtensionId& id,
                                                 const std::string& version_str,
                                                 const base::FilePath& path) {
-  auto version = std::make_unique<base::Version>(version_str);
   auto info = std::make_unique<ExternalInstallInfoFile>(
-      id, std::move(version), path, location_, Extension::NO_FLAGS, false,
-      false);
+      id, base::Version(version_str), path, location_, Extension::NO_FLAGS,
+      false, false);
   extension_map_[id] = std::move(info);
 }
 
@@ -57,7 +57,7 @@ bool MockExternalProvider::GetExtensionDetails(
     return false;
 
   if (version)
-    version->reset(new base::Version(it->second->version->GetString()));
+    version->reset(new base::Version(it->second->version));
 
   if (location)
     *location = location_;