Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

components/content_settings/core/common/content_settings_pattern.cc

 // Copyright (c) 2012 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.
 
 #include "components/content_settings/core/common/content_settings_pattern.h"
 
 #include <stddef.h>
 
 #include <memory>
 #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"
 #include "net/base/url_util.h"
 #include "url/gurl.h"
 
 namespace {
 
 // The component supports only one scheme for simplicity.
 const char* non_port_non_domain_wildcard_scheme = NULL;
@@ skipping 70 matching lines @@
 typedef ContentSettingsPattern::BuilderInterface BuilderInterface;
 
 }  // namespace
 
 // ////////////////////////////////////////////////////////////////////////////
 // ContentSettingsPattern::Builder
 //
 class ContentSettingsPattern::Builder :
     public ContentSettingsPattern::BuilderInterface {
  public:
-  explicit Builder(bool use_legacy_validate);
+  explicit Builder();
   ~Builder() override;
 
   // BuilderInterface:
   BuilderInterface* WithPort(const std::string& port) override;
   BuilderInterface* WithPortWildcard() override;
   BuilderInterface* WithHost(const std::string& host) override;
   BuilderInterface* WithDomainWildcard() override;
   BuilderInterface* WithScheme(const std::string& scheme) override;
   BuilderInterface* WithSchemeWildcard() override;
   BuilderInterface* WithPath(const std::string& path) override;
   BuilderInterface* WithPathWildcard() override;
   BuilderInterface* Invalid() override;
   ContentSettingsPattern Build() override;
 
  private:
   // Canonicalizes the pattern parts so that they are ASCII only, either
   // in original (if it was already ASCII) or punycode form. Returns true if
   // the canonicalization was successful.
   static bool Canonicalize(PatternParts* parts);
 
   // 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() {}
 
 BuilderInterface* ContentSettingsPattern::Builder::WithPort(
     const std::string& port) {
   parts_.port = port;
   parts_.is_port_wildcard = false;
   return this;
 }
 
@@ skipping 43 matching lines @@
 BuilderInterface* ContentSettingsPattern::Builder::Invalid() {
   is_valid_ = false;
   return this;
 }
 
 ContentSettingsPattern ContentSettingsPattern::Builder::Build() {
   if (!is_valid_)
     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();
   if (ContentSettingsPattern(parts_, true) !=
       ContentSettingsPattern(parts, true)) {
     return ContentSettingsPattern();
   }
 
   return ContentSettingsPattern(parts_, is_valid_);
 }
@@ skipping 70 matching lines @@
 
   // 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;
 }
 
-// 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
 //
 ContentSettingsPattern::PatternParts::PatternParts()
         : is_scheme_wildcard(false),
           has_domain_wildcard(false),
           is_port_wildcard(false),
           is_path_wildcard(false) {}
 
 ContentSettingsPattern::PatternParts::PatternParts(const PatternParts& other) =
@@ skipping 10 matching lines @@
 //   - [*.]domain.tld (matches domain.tld and all sub-domains)
 //   - host (matches an exact hostname)
 //   - a.b.c.d (matches an exact IPv4 ip)
 //   - [a:b:c:d:e:f:g:h] (matches an exact IPv6 ip)
 //   - file:///tmp/test.html (a complete URL without a host)
 // Version 2 adds a resource identifier for plugins.
 // TODO(jochen): update once this feature is no longer behind a flag.
 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
 ContentSettingsPattern ContentSettingsPattern::Wildcard() {
   PatternParts parts;
   parts.is_scheme_wildcard = true;
   parts.has_domain_wildcard = true;
   parts.is_port_wildcard = true;
   parts.is_path_wildcard = true;
   return ContentSettingsPattern(parts, true);
 }
 
 // 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();
   }
   if (local_url->SchemeIsFile()) {
     builder->WithScheme(local_url->scheme())->WithPath(local_url->path());
   } else {
     // Please keep the order of the ifs below as URLs with an IP as host can
     // also have a "http" scheme.
     if (local_url->HostIsIPAddress()) {
@@ skipping 15 matching lines @@
     } else {
       builder->WithPort(local_url->port());
     }
   }
   return builder->Build();
 }
 
 // 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();
   }
   if (local_url->SchemeIsFile()) {
     builder->WithScheme(local_url->scheme())->WithPath(local_url->path());
   } else {
     builder->WithScheme(local_url->scheme())->WithHost(local_url->host());
     if (local_url->port().empty()) {
       builder->WithPort(GetDefaultPort(local_url->scheme()));
     } else {
       builder->WithPort(local_url->port());
     }
   }
   return builder->Build();
 }
 
 // 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();
 }
 
 // static
 bool ContentSettingsPattern::MigrateFromDomainToOrigin(
     const ContentSettingsPattern& domain_pattern,
     ContentSettingsPattern* origin_pattern) {
   DCHECK(origin_pattern);
@@ skipping 14 matching lines @@
 
   // Patterns generated with ::FromURL will always have a domain wildcard. Those
   // generated with ::FromURLNoWildcard don't.
   if (!domain_pattern.parts_.has_domain_wildcard)
     return false;
 
   // Generated patterns with ::FromURL will always have a host.
   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
     builder->WithScheme(domain_pattern.parts_.scheme);
 
   builder->WithHost(domain_pattern.parts_.host);
 
   if (domain_pattern.parts_.is_port_wildcard) {
     if (domain_pattern.parts_.scheme == url::kHttpsScheme) {
       builder->WithPort(GetDefaultPort(url::kHttpsScheme));
@@ skipping 324 matching lines @@
   if (!parts.is_path_wildcard && other_parts.is_path_wildcard)
     return ContentSettingsPattern::PREDECESSOR;
 
   int result = parts.path.compare(other_parts.path);
   if (result == 0)
     return ContentSettingsPattern::IDENTITY;
   if (result > 0)
     return ContentSettingsPattern::DISJOINT_ORDER_PRE;
   return ContentSettingsPattern::DISJOINT_ORDER_POST;
 }