Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

components/content_settings/core/common/content_settings_pattern.cc

Index: components/content_settings/core/common/content_settings_pattern.cc
diff --git a/components/content_settings/core/common/content_settings_pattern.cc b/components/content_settings/core/common/content_settings_pattern.cc
index a83a6959a02240575eae95fe7168e4ea75c644bc..55c1854075d952b6e3f0b2af930d9941069f5fc9 100644
--- a/components/content_settings/core/common/content_settings_pattern.cc
+++ b/components/content_settings/core/common/content_settings_pattern.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "components/content_settings/core/common/content_settings_pattern_parser.h"
@@ -100,7 +101,7 @@ typedef ContentSettingsPattern::BuilderInterface BuilderInterface;
 class ContentSettingsPattern::Builder :
     public ContentSettingsPattern::BuilderInterface {
  public:
-  explicit Builder(bool use_legacy_validate);
+  explicit Builder();
   ~Builder() override;
 
   // BuilderInterface:
@@ -124,20 +125,14 @@ class ContentSettingsPattern::Builder :
   // Returns true when the pattern |parts| represent a valid pattern.
   static bool Validate(const PatternParts& parts);
 
-  static bool LegacyValidate(const PatternParts& parts);
-
   bool is_valid_;
 
-  bool use_legacy_validate_;
-
   PatternParts parts_;
 
   DISALLOW_COPY_AND_ASSIGN(Builder);
 };
 
-ContentSettingsPattern::Builder::Builder(bool use_legacy_validate)
-    : is_valid_(true),
-      use_legacy_validate_(use_legacy_validate) {}
+ContentSettingsPattern::Builder::Builder() : is_valid_(true) {}
 
 ContentSettingsPattern::Builder::~Builder() {}
 
@@ -201,17 +196,11 @@ ContentSettingsPattern ContentSettingsPattern::Builder::Build() {
     return ContentSettingsPattern();
   if (!Canonicalize(&parts_))
     return ContentSettingsPattern();
-  if (use_legacy_validate_) {
-    is_valid_ = LegacyValidate(parts_);
-  } else {
-    is_valid_ = Validate(parts_);
-  }
+  is_valid_ = Validate(parts_);
   if (!is_valid_)
     return ContentSettingsPattern();
 
   // A pattern is invalid if canonicalization is not idempotent.
-  // This check is here because it should be checked no matter
-  // use_legacy_validate_ is.
   PatternParts parts(parts_);
   if (!Canonicalize(&parts))
     return ContentSettingsPattern();
@@ -302,41 +291,6 @@ bool ContentSettingsPattern::Builder::Validate(const PatternParts& parts) {
   return true;
 }
 
-// static
-bool ContentSettingsPattern::Builder::LegacyValidate(
-    const PatternParts& parts) {
-  // If the pattern is for a "file-pattern" test if it is valid.
-  if (parts.scheme == std::string(url::kFileScheme) &&
-      !parts.is_scheme_wildcard &&
-      parts.host.empty() &&
-      parts.port.empty())
-    return true;
-
-  // If the pattern is for an extension URL test if it is valid.
-  if (IsNonWildcardDomainNonPortScheme(parts.scheme) &&
-      !parts.is_scheme_wildcard &&
-      !parts.host.empty() &&
-      !parts.has_domain_wildcard &&
-      parts.port.empty() &&
-      !parts.is_port_wildcard)
-    return true;
-
-  // Non-file patterns are invalid if either the scheme, host or port part is
-  // empty.
-  if ((!parts.is_scheme_wildcard) ||
-      (parts.host.empty() && !parts.has_domain_wildcard) ||
-      (!parts.is_port_wildcard))
-    return false;
-
-  // Test if the scheme is supported or a wildcard.
-  if (!parts.is_scheme_wildcard &&
-      parts.scheme != std::string(url::kHttpScheme) &&
-      parts.scheme != std::string(url::kHttpsScheme)) {
-    return false;
-  }
-  return true;
-}
-
 // ////////////////////////////////////////////////////////////////////////////
 // ContentSettingsPattern::PatternParts
 //
@@ -367,9 +321,8 @@ ContentSettingsPattern::PatternParts::~PatternParts() {}
 const int ContentSettingsPattern::kContentSettingsPatternVersion = 1;
 
 // static
-BuilderInterface* ContentSettingsPattern::CreateBuilder(
-    bool validate) {
-  return new Builder(validate);
+std::unique_ptr<BuilderInterface> ContentSettingsPattern::CreateBuilder() {
+  return base::MakeUnique<Builder>();
 }
 
 // static
@@ -385,8 +338,7 @@ ContentSettingsPattern ContentSettingsPattern::Wildcard() {
 // static
 ContentSettingsPattern ContentSettingsPattern::FromURL(
     const GURL& url) {
-  std::unique_ptr<ContentSettingsPattern::BuilderInterface> builder(
-      ContentSettingsPattern::CreateBuilder(false));
+  auto builder = ContentSettingsPattern::CreateBuilder();

[msramek, 2017/05/31 10:42:32]

The style guide suggests that auto should not be used if the type cannot be
inferred by looking at the surrounding lines. I think that holds here and in the
other replacements in this file.

However, auto would be permitted if the type was visible from the template. That
brings me to a question - do we even need CreateBuilder()? Nowadays, one could
just do

auto builder = base::MakeUnique<ContentSettingsPattern>();

I think the existence of CreateBuilder() was only supposed to hide the raw
"new", and nowadays we have standard ways to do that.

[dullweber, 2017/05/31 11:42:37]

CreateBuilder returns a ContentSettingsBuilder::BuilderInterface.
The only reason to have BuilderInterface is that
ContentSettingsPatternParserTest mocks the builder to check for the right calls.
I could either move the ::Builder class to the header and make it public, so it
could be created directly (even on the stack by ContentSettingsPattern::Builder
builder;) or keep it this way and use an explicit type instead of auto. I will
do the second for now because it requires less changes. I can still use stack
allocation inside ContentSettingsPattern.

[msramek, 2017/05/31 16:08:46]

Acknowledged. I'm fine with the other proposal too, but let's not overcomplicate
this CL.

   const GURL* local_url = &url;
   if (url.SchemeIsFileSystem() && url.inner_url()) {
     local_url = url.inner_url();
@@ -422,9 +374,7 @@ ContentSettingsPattern ContentSettingsPattern::FromURL(
 // static
 ContentSettingsPattern ContentSettingsPattern::FromURLNoWildcard(
     const GURL& url) {
-  std::unique_ptr<ContentSettingsPattern::BuilderInterface> builder(
-      ContentSettingsPattern::CreateBuilder(false));
-
+  auto builder = ContentSettingsPattern::CreateBuilder();
   const GURL* local_url = &url;
   if (url.SchemeIsFileSystem() && url.inner_url()) {
     local_url = url.inner_url();
@@ -445,8 +395,7 @@ ContentSettingsPattern ContentSettingsPattern::FromURLNoWildcard(
 // static
 ContentSettingsPattern ContentSettingsPattern::FromString(
     const std::string& pattern_spec) {
-  std::unique_ptr<ContentSettingsPattern::BuilderInterface> builder(
-      ContentSettingsPattern::CreateBuilder(false));
+  auto builder = ContentSettingsPattern::CreateBuilder();
   content_settings::PatternParser::Parse(pattern_spec,
                                          builder.get());
   return builder->Build();
@@ -481,9 +430,7 @@ bool ContentSettingsPattern::MigrateFromDomainToOrigin(
   if (domain_pattern.parts_.host.empty())
     return false;
 
-  std::unique_ptr<ContentSettingsPattern::BuilderInterface> builder(
-      ContentSettingsPattern::CreateBuilder(false));
-
+  auto builder = ContentSettingsPattern::CreateBuilder();
   if (domain_pattern.parts_.is_scheme_wildcard)
     builder->WithScheme(url::kHttpScheme);
   else