Avoiding diamond inheritance in url/origin/trustworthiness tests.

This CL removes pure-virtual interfaces that used to document
requirements of traits that back the AbstractUrlTest, AbstractOriginTest
and AbstractTrustworthinessTest (and enforce these requirements at
compile time).  The documentation is moved to a comment, and at
compile-time we now rely on duck-typing (i.e. what C++ templates do).

The main motivation for this change is the removal of diamond/virtual
inheritance that was necessary before.  Additionally, the shorter
resulting code can be argued to be simpler and easier to read.

Bug: 1164416
Change-Id: I6fee0bdf9bf24d553535be6b8e0ba70400e05f2b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2644150
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846817}

services/network/public/cpp/is_potentially_trustworthy_unittest.cc

@@ -215,19 +215,22 @@ TEST_F(SecureOriginAllowlistTest, Canonicalization) {
   EXPECT_THAT(canonicalized, ::testing::ElementsAre("*.example.com"));
 }
 
-class TrustworthinessTestTraits final
-    : public virtual url::UrlOriginTestTraits,
-      public virtual TrustworthinessTraitsBase<url::Origin> {
+class TrustworthinessTestTraits : public url::UrlOriginTestTraits {
  public:
-  bool IsOriginPotentiallyTrustworthy(const OriginType& origin) override {
+  using OriginType = url::Origin;
+
+  static bool IsOriginPotentiallyTrustworthy(const OriginType& origin) {
     return network::IsOriginPotentiallyTrustworthy(origin);
   }
-  bool IsUrlPotentiallyTrustworthy(base::StringPiece str) override {
+  static bool IsUrlPotentiallyTrustworthy(base::StringPiece str) {
     return network::IsUrlPotentiallyTrustworthy(GURL(str));
   }
-  bool IsOriginOfLocalhost(const OriginType& origin) override {
+  static bool IsOriginOfLocalhost(const OriginType& origin) {
     return net::IsLocalhost(origin.GetURL());
   }
+
+  // Only static members = no constructors are needed.
+  TrustworthinessTestTraits() = delete;
 };
 
 INSTANTIATE_TYPED_TEST_SUITE_P(UrlOrigin,

services/network/public/cpp/is_potentially_trustworthy_unittest.h

@@ -16,51 +16,34 @@ namespace test {
 // AbstractTrustworthinessTest below abstracts away differences between
 // network::IsOriginPotentiallyTrustworthy and
 // blink::SecurityOrigin::IsPotentiallyTrustworthy by parametrizing the tests
-// with a class that has to be derived from TrustworthinessTraitsBase
-// below.
-template <typename TConcreteOriginType>
-class TrustworthinessTraitsBase
-    : public virtual url::OriginTraitsBase<TConcreteOriginType> {
- public:
-  using OriginType = TConcreteOriginType;
-  virtual bool IsOriginPotentiallyTrustworthy(const OriginType& origin) = 0;
-  virtual bool IsUrlPotentiallyTrustworthy(base::StringPiece str) = 0;
-  virtual bool IsOriginOfLocalhost(const OriginType& origin) = 0;
-};
-
-// Test suite for tests that cover both url::Origin and blink::SecurityOrigin.
+// with a class that has to expose the same members as url::UrlOriginTestTraits
+// and the following extra members:
+//   static bool IsOriginPotentiallyTrustworthy(const OriginType& origin);
+//   static bool IsUrlPotentiallyTrustworthy(base::StringPiece str);
+//   static bool IsOriginOfLocalhost(const OriginType& origin);
 template <typename TTrustworthinessTraits>
 class AbstractTrustworthinessTest
     : public url::AbstractOriginTest<TTrustworthinessTraits> {
-  static_assert(
-      std::is_base_of<TrustworthinessTraitsBase<
-                          typename TTrustworthinessTraits::OriginType>,
-                      TTrustworthinessTraits>::value,
-      "TTrustworthinessTraits needs to expose the right members.");
-
  protected:
-  // Wrappers that allow tests to ignore presence of `test_traits_`.
+  // Wrappers that help ellide away TTrustworthinessTraits.
   //
   // Note that calling the wrappers needs to be prefixed with `this->...` to
   // avoid hitting: explicit qualification required to use member 'FooBar'
   // from dependent base class.
   using OriginType = typename TTrustworthinessTraits::OriginType;
   bool IsOriginPotentiallyTrustworthy(const OriginType& origin) {
-    return trustworthiness_traits_.IsOriginPotentiallyTrustworthy(origin);
+    return TTrustworthinessTraits::IsOriginPotentiallyTrustworthy(origin);
   }
   bool IsOriginPotentiallyTrustworthy(base::StringPiece str) {
     auto origin = this->CreateOriginFromString(str);
-    return this->IsOriginPotentiallyTrustworthy(origin);
+    return TTrustworthinessTraits::IsOriginPotentiallyTrustworthy(origin);
   }
   bool IsUrlPotentiallyTrustworthy(base::StringPiece str) {
-    return trustworthiness_traits_.IsUrlPotentiallyTrustworthy(str);
+    return TTrustworthinessTraits::IsUrlPotentiallyTrustworthy(str);
   }
   bool IsOriginOfLocalhost(const OriginType& origin) {
-    return trustworthiness_traits_.IsOriginOfLocalhost(origin);
+    return TTrustworthinessTraits::IsOriginOfLocalhost(origin);
   }
-
- private:
-  TTrustworthinessTraits trustworthiness_traits_;
 };
 
 TYPED_TEST_SUITE_P(AbstractTrustworthinessTest);

third_party/blink/renderer/platform/weborigin/kurl_test.cc

@@ -1073,19 +1073,22 @@ INSTANTIATE_TEST_SUITE_P(All,
 // namespace as where the typed test suite was defined.
 namespace url {
 
-class KURLTestTraits final : public UrlTraitsBase<blink::KURL> {
+class KURLTestTraits {
  public:
-  UrlType CreateUrlFromString(base::StringPiece s) override {
+  using UrlType = blink::KURL;
+
+  static UrlType CreateUrlFromString(base::StringPiece s) {
     return blink::KURL(String::FromUTF8(s));
   }
 
-  bool IsAboutBlank(const UrlType& url) override {
-    return url.IsAboutBlankURL();
-  }
+  static bool IsAboutBlank(const UrlType& url) { return url.IsAboutBlankURL(); }
 
-  bool IsAboutSrcdoc(const UrlType& url) override {
+  static bool IsAboutSrcdoc(const UrlType& url) {
     return url.IsAboutSrcdocURL();
   }
+
+  // Only static members.
+  KURLTestTraits() = delete;
 };
 
 INSTANTIATE_TYPED_TEST_SUITE_P(KURL, AbstractUrlTest, KURLTestTraits);

third_party/blink/renderer/platform/weborigin/security_origin_test.cc

@@ -1065,46 +1065,43 @@ TEST_F(SecurityOriginTest, PercentEncodesHost) {
 // namespace as where the typed test suite was defined.
 namespace url {
 
-class BlinkSecurityOriginTestTraits final
-    : public network::test::TrustworthinessTraitsBase<
-          scoped_refptr<blink::SecurityOrigin>> {
+class BlinkSecurityOriginTestTraits {
  public:
-  OriginType CreateOriginFromString(base::StringPiece s) override {
+  using OriginType = scoped_refptr<blink::SecurityOrigin>;
+
+  static OriginType CreateOriginFromString(base::StringPiece s) {
     return blink::SecurityOrigin::CreateFromString(String::FromUTF8(s));
   }
 
-  OriginType CreateUniqueOpaqueOrigin() override {
+  static OriginType CreateUniqueOpaqueOrigin() {
     return blink::SecurityOrigin::CreateUniqueOpaque();
   }
 
-  OriginType CreateWithReferenceOrigin(
+  static OriginType CreateWithReferenceOrigin(
       base::StringPiece url,
-      const OriginType& reference_origin) override {
+      const OriginType& reference_origin) {
     return blink::SecurityOrigin::CreateWithReferenceOrigin(
         blink::KURL(String::FromUTF8(url)), reference_origin.get());
   }
 
-  OriginType DeriveNewOpaqueOrigin(
-      const OriginType& reference_origin) override {
+  static OriginType DeriveNewOpaqueOrigin(const OriginType& reference_origin) {
     return reference_origin->DeriveNewOpaqueOrigin();
   }
 
-  bool IsOpaque(const OriginType& origin) override {
-    return origin->IsOpaque();
-  }
+  static bool IsOpaque(const OriginType& origin) { return origin->IsOpaque(); }
 
-  std::string GetScheme(const OriginType& origin) override {
+  static std::string GetScheme(const OriginType& origin) {
     return origin->Protocol().Utf8();
   }
 
-  std::string GetHost(const OriginType& origin) override {
+  static std::string GetHost(const OriginType& origin) {
     return origin->Host().Utf8();
   }
 
-  uint16_t GetPort(const OriginType& origin) override { return origin->Port(); }
+  static uint16_t GetPort(const OriginType& origin) { return origin->Port(); }
 
-  SchemeHostPort GetTupleOrPrecursorTupleIfOpaque(
-      const OriginType& origin) override {
+  static SchemeHostPort GetTupleOrPrecursorTupleIfOpaque(
+      const OriginType& origin) {
     const blink::SecurityOrigin* precursor =
         origin->GetOriginOrPrecursorOriginIfOpaque();
     if (!precursor)
@@ -1113,29 +1110,32 @@ class BlinkSecurityOriginTestTraits final
                           precursor->Host().Utf8(), precursor->Port());
   }
 
-  bool IsSameOrigin(const OriginType& a, const OriginType& b) override {
+  static bool IsSameOrigin(const OriginType& a, const OriginType& b) {
     return a->IsSameOriginWith(b.get());
   }
 
-  std::string Serialize(const OriginType& origin) override {
+  static std::string Serialize(const OriginType& origin) {
     return origin->ToString().Utf8();
   }
 
-  bool IsValidUrl(base::StringPiece str) override {
+  static bool IsValidUrl(base::StringPiece str) {
     return blink::KURL(String::FromUTF8(str)).IsValid();
   }
 
-  bool IsOriginPotentiallyTrustworthy(const OriginType& origin) override {
+  static bool IsOriginPotentiallyTrustworthy(const OriginType& origin) {
     return origin->IsPotentiallyTrustworthy();
   }
 
-  bool IsUrlPotentiallyTrustworthy(base::StringPiece str) override {
+  static bool IsUrlPotentiallyTrustworthy(base::StringPiece str) {
     return blink::SecurityOrigin::IsSecure(blink::KURL(String::FromUTF8(str)));
   }
 
-  bool IsOriginOfLocalhost(const OriginType& origin) override {
+  static bool IsOriginOfLocalhost(const OriginType& origin) {
     return origin->IsLocalhost();
   }
+
+  // Only static members = no constructors are needed.
+  BlinkSecurityOriginTestTraits() = delete;
 };
 
 INSTANTIATE_TYPED_TEST_SUITE_P(BlinkSecurityOrigin,

url/gurl_abstract_tests.h

@@ -5,53 +5,33 @@
 #ifndef URL_GURL_ABSTRACT_TESTS_H_
 #define URL_GURL_ABSTRACT_TESTS_H_
 
-// AbstractUrlTest below abstracts away differences between GURL and blink::KURL
-// by parametrizing the tests with a class that has to be derived from
-// UrlTraitsBase below.
-template <typename TConcreteUrlType>
-class UrlTraitsBase {
- public:
-  using UrlType = TConcreteUrlType;
-  UrlTraitsBase() = default;
-
-  // Constructing an origin.
-  virtual UrlType CreateUrlFromString(base::StringPiece s) = 0;
-
-  // Accessors for origin properties.
-  virtual bool IsAboutBlank(const UrlType& url) = 0;
-  virtual bool IsAboutSrcdoc(const UrlType& url) = 0;
-
-  // This type is non-copyable and non-moveable.
-  UrlTraitsBase(const UrlTraitsBase&) = delete;
-  UrlTraitsBase& operator=(const UrlTraitsBase&) = delete;
-};
-
 // Test suite for tests that cover both url::Url and blink::SecurityUrl.
+//
+// AbstractUrlTest below abstracts away differences between GURL and blink::KURL
+// by parametrizing the tests with a class that has to expose the following
+// members:
+//   using UrlType = ...;
+//   static UrlType CreateUrlFromString(base::StringPiece s);
+//   static bool IsAboutBlank(const UrlType& url);
+//   static bool IsAboutSrcdoc(const UrlType& url);
 template <typename TUrlTraits>
 class AbstractUrlTest : public testing::Test {
-  static_assert(std::is_base_of<UrlTraitsBase<typename TUrlTraits::UrlType>,
-                                TUrlTraits>::value,
-                "TUrlTraits needs to expose the right members.");
-
  protected:
-  // Wrappers that allow tests to ignore presence of `url_traits_`.
+  // Wrappers that help ellide away TUrlTraits.
   //
   // Note that calling the wrappers needs to be prefixed with `this->...` to
   // avoid hitting: explicit qualification required to use member 'IsAboutBlank'
   // from dependent base class.
   using UrlType = typename TUrlTraits::UrlType;
   UrlType CreateUrlFromString(base::StringPiece s) {
-    return url_traits_.CreateUrlFromString(s);
+    return TUrlTraits::CreateUrlFromString(s);
   }
   bool IsAboutBlank(const UrlType& url) {
-    return url_traits_.IsAboutBlank(url);
+    return TUrlTraits::IsAboutBlank(url);
   }
   bool IsAboutSrcdoc(const UrlType& url) {
-    return url_traits_.IsAboutSrcdoc(url);
+    return TUrlTraits::IsAboutSrcdoc(url);
   }
-
- private:
-  TUrlTraits url_traits_;
 };
 
 TYPED_TEST_SUITE_P(AbstractUrlTest);

url/gurl_unittest.cc

@@ -992,15 +992,16 @@ TEST(GURLTest, PortZero) {
   EXPECT_FALSE(default_port_origin.IsSameOriginWith(resolved_origin));
 }
 
-class GURLTestTraits final : public UrlTraitsBase<GURL> {
+class GURLTestTraits {
  public:
-  UrlType CreateUrlFromString(base::StringPiece s) override { return GURL(s); }
+  using UrlType = GURL;
 
-  bool IsAboutBlank(const UrlType& url) override { return url.IsAboutBlank(); }
+  static UrlType CreateUrlFromString(base::StringPiece s) { return GURL(s); }
+  static bool IsAboutBlank(const UrlType& url) { return url.IsAboutBlank(); }
+  static bool IsAboutSrcdoc(const UrlType& url) { return url.IsAboutSrcdoc(); }
 
-  bool IsAboutSrcdoc(const UrlType& url) override {
-    return url.IsAboutSrcdoc();
-  }
+  // Only static members.
+  GURLTestTraits() = delete;
 };
 
 INSTANTIATE_TYPED_TEST_SUITE_P(GURL, AbstractUrlTest, GURLTestTraits);

url/origin_abstract_tests.cc

@@ -6,54 +6,66 @@
 
 namespace url {
 
+// static
 Origin UrlOriginTestTraits::CreateOriginFromString(base::StringPiece s) {
   return Origin::Create(GURL(s));
 }
 
+// static
 Origin UrlOriginTestTraits::CreateUniqueOpaqueOrigin() {
   return Origin();
 }
 
+// static
 Origin UrlOriginTestTraits::CreateWithReferenceOrigin(
     base::StringPiece url,
     const Origin& reference_origin) {
   return Origin::Resolve(GURL(url), reference_origin);
 }
 
+// static
 Origin UrlOriginTestTraits::DeriveNewOpaqueOrigin(
     const Origin& reference_origin) {
   return reference_origin.DeriveNewOpaqueOrigin();
 }
 
+// static
 bool UrlOriginTestTraits::IsOpaque(const Origin& origin) {
   return origin.opaque();
 }
 
+// static
 std::string UrlOriginTestTraits::GetScheme(const Origin& origin) {
   return origin.scheme();
 }
 
+// static
 std::string UrlOriginTestTraits::GetHost(const Origin& origin) {
   return origin.host();
 }
 
+// static
 uint16_t UrlOriginTestTraits::GetPort(const Origin& origin) {
   return origin.port();
 }
 
+// static
 SchemeHostPort UrlOriginTestTraits::GetTupleOrPrecursorTupleIfOpaque(
     const Origin& origin) {
   return origin.GetTupleOrPrecursorTupleIfOpaque();
 }
 
+// static
 bool UrlOriginTestTraits::IsSameOrigin(const Origin& a, const Origin& b) {
   return a.IsSameOriginWith(b);
 }
 
+// static
 std::string UrlOriginTestTraits::Serialize(const Origin& origin) {
   return origin.Serialize();
 }
 
+// static
 bool UrlOriginTestTraits::IsValidUrl(base::StringPiece str) {
   return GURL(str).is_valid();
 }