From 123987f5972ba8da1710d43a251b4e978e6f4176 Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Mon, 25 Jan 2021 19:17:29 +0000 Subject: [PATCH] Avoiding diamond inheritance in url/origin/trustworthiness tests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Tom Sepez Reviewed-by: Daniel Cheng Commit-Queue: Łukasz Anforowicz Cr-Commit-Position: refs/heads/master@{#846817} --- .../is_potentially_trustworthy_unittest.cc | 15 +-- .../cpp/is_potentially_trustworthy_unittest.h | 37 ++------ .../renderer/platform/weborigin/kurl_test.cc | 15 +-- .../weborigin/security_origin_test.cc | 46 ++++----- url/gurl_abstract_tests.h | 44 +++------ url/gurl_unittest.cc | 13 +-- url/origin_abstract_tests.cc | 12 +++ url/origin_abstract_tests.h | 95 +++++++------------ 8 files changed, 115 insertions(+), 162 deletions(-) diff --git a/services/network/public/cpp/is_potentially_trustworthy_unittest.cc b/services/network/public/cpp/is_potentially_trustworthy_unittest.cc index 983eadf9c477c3..947dd4a8e55e3b 100644 --- a/services/network/public/cpp/is_potentially_trustworthy_unittest.cc +++ b/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 { +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, diff --git a/services/network/public/cpp/is_potentially_trustworthy_unittest.h b/services/network/public/cpp/is_potentially_trustworthy_unittest.h index e8e97c052132c3..0c2ff61b28962c 100644 --- a/services/network/public/cpp/is_potentially_trustworthy_unittest.h +++ b/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 -class TrustworthinessTraitsBase - : public virtual url::OriginTraitsBase { - 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 class AbstractTrustworthinessTest : public url::AbstractOriginTest { - static_assert( - std::is_base_of, - 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); diff --git a/third_party/blink/renderer/platform/weborigin/kurl_test.cc b/third_party/blink/renderer/platform/weborigin/kurl_test.cc index 799f3da690fc88..5e3068ab97feb9 100644 --- a/third_party/blink/renderer/platform/weborigin/kurl_test.cc +++ b/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 { +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); diff --git a/third_party/blink/renderer/platform/weborigin/security_origin_test.cc b/third_party/blink/renderer/platform/weborigin/security_origin_test.cc index 9675125411e66b..7e06602d1b10fb 100644 --- a/third_party/blink/renderer/platform/weborigin/security_origin_test.cc +++ b/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> { +class BlinkSecurityOriginTestTraits { public: - OriginType CreateOriginFromString(base::StringPiece s) override { + using OriginType = scoped_refptr; + + 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, diff --git a/url/gurl_abstract_tests.h b/url/gurl_abstract_tests.h index 38909be8c57607..118648409623d9 100644 --- a/url/gurl_abstract_tests.h +++ b/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 -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 class AbstractUrlTest : public testing::Test { - static_assert(std::is_base_of, - 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); diff --git a/url/gurl_unittest.cc b/url/gurl_unittest.cc index e9784c78caffde..556e8cb2b17ff5 100644 --- a/url/gurl_unittest.cc +++ b/url/gurl_unittest.cc @@ -992,15 +992,16 @@ TEST(GURLTest, PortZero) { EXPECT_FALSE(default_port_origin.IsSameOriginWith(resolved_origin)); } -class GURLTestTraits final : public UrlTraitsBase { +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); diff --git a/url/origin_abstract_tests.cc b/url/origin_abstract_tests.cc index f19388d109b959..0d9467f6d44c09 100644 --- a/url/origin_abstract_tests.cc +++ b/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(); } diff --git a/url/origin_abstract_tests.h b/url/origin_abstract_tests.h index ff1f6cc443a275..5894d7595aa648 100644 --- a/url/origin_abstract_tests.h +++ b/url/origin_abstract_tests.h @@ -20,54 +20,45 @@ namespace url { // AbstractOriginTest below abstracts away differences between url::Origin and -// blink::SecurityOrigin by parametrizing the tests with a class that has to be -// derived from OriginTraitsBase below. -template -class OriginTraitsBase { +// blink::SecurityOrigin by parametrizing the tests with a class that has to +// expose the same public members as UrlOriginTestTraits below. +class UrlOriginTestTraits { public: - using OriginType = TConcreteOriginType; - OriginTraitsBase() = default; + using OriginType = Origin; // Constructing an origin. - virtual OriginType CreateOriginFromString(base::StringPiece s) = 0; - virtual OriginType CreateUniqueOpaqueOrigin() = 0; - virtual OriginType CreateWithReferenceOrigin( + static OriginType CreateOriginFromString(base::StringPiece s); + static OriginType CreateUniqueOpaqueOrigin(); + static OriginType CreateWithReferenceOrigin( base::StringPiece url, - const OriginType& reference_origin) = 0; - virtual OriginType DeriveNewOpaqueOrigin( - const OriginType& reference_origin) = 0; + const OriginType& reference_origin); + static OriginType DeriveNewOpaqueOrigin(const OriginType& reference_origin); // Accessors for origin properties. - virtual bool IsOpaque(const OriginType& origin) = 0; - virtual std::string GetScheme(const OriginType& origin) = 0; - virtual std::string GetHost(const OriginType& origin) = 0; - virtual uint16_t GetPort(const OriginType& origin) = 0; - virtual SchemeHostPort GetTupleOrPrecursorTupleIfOpaque( - const OriginType& origin) = 0; + static bool IsOpaque(const OriginType& origin); + static std::string GetScheme(const OriginType& origin); + static std::string GetHost(const OriginType& origin); + static uint16_t GetPort(const OriginType& origin); + static SchemeHostPort GetTupleOrPrecursorTupleIfOpaque( + const OriginType& origin); // Wrappers for other instance methods of OriginType. - virtual bool IsSameOrigin(const OriginType& a, const OriginType& b) = 0; - virtual std::string Serialize(const OriginType& origin) = 0; + static bool IsSameOrigin(const OriginType& a, const OriginType& b); + static std::string Serialize(const OriginType& origin); // "Accessors" of URL properties. // // TODO(lukasza): Consider merging together OriginTraitsBase here and // UrlTraitsBase in //url/gurl_abstract_tests.h. - virtual bool IsValidUrl(base::StringPiece str) = 0; + static bool IsValidUrl(base::StringPiece str); - // This type is non-copyable and non-moveable. - OriginTraitsBase(const OriginTraitsBase&) = delete; - OriginTraitsBase& operator=(const OriginTraitsBase&) = delete; + // Only static members = no constructors are needed. + UrlOriginTestTraits() = delete; }; // Test suite for tests that cover both url::Origin and blink::SecurityOrigin. template class AbstractOriginTest : public testing::Test { - static_assert( - std::is_base_of, - TOriginTraits>::value, - "TOriginTraits needs to expose the right members."); - public: void SetUp() override { const char* kSchemesToRegister[] = { @@ -97,48 +88,48 @@ class AbstractOriginTest : public testing::Test { } protected: - // Wrappers that allow tests to ignore presence of `origin_test_traits_`. + // Wrappers that help ellide away TOriginTraits. // // Note that calling the wrappers needs to be prefixed with `this->...` to // avoid hitting: explicit qualification required to use member 'IsOpaque' // from dependent base class. using OriginType = typename TOriginTraits::OriginType; OriginType CreateOriginFromString(base::StringPiece s) { - return origin_traits_.CreateOriginFromString(s); + return TOriginTraits::CreateOriginFromString(s); } OriginType CreateUniqueOpaqueOrigin() { - return origin_traits_.CreateUniqueOpaqueOrigin(); + return TOriginTraits::CreateUniqueOpaqueOrigin(); } OriginType CreateWithReferenceOrigin(base::StringPiece url, const OriginType& reference_origin) { - return origin_traits_.CreateWithReferenceOrigin(url, reference_origin); + return TOriginTraits::CreateWithReferenceOrigin(url, reference_origin); } OriginType DeriveNewOpaqueOrigin(const OriginType& reference_origin) { - return origin_traits_.DeriveNewOpaqueOrigin(reference_origin); + return TOriginTraits::DeriveNewOpaqueOrigin(reference_origin); } bool IsOpaque(const OriginType& origin) { - return origin_traits_.IsOpaque(origin); + return TOriginTraits::IsOpaque(origin); } std::string GetScheme(const OriginType& origin) { - return origin_traits_.GetScheme(origin); + return TOriginTraits::GetScheme(origin); } std::string GetHost(const OriginType& origin) { - return origin_traits_.GetHost(origin); + return TOriginTraits::GetHost(origin); } uint16_t GetPort(const OriginType& origin) { - return origin_traits_.GetPort(origin); + return TOriginTraits::GetPort(origin); } SchemeHostPort GetTupleOrPrecursorTupleIfOpaque(const OriginType& origin) { - return origin_traits_.GetTupleOrPrecursorTupleIfOpaque(origin); + return TOriginTraits::GetTupleOrPrecursorTupleIfOpaque(origin); } bool IsSameOrigin(const OriginType& a, const OriginType& b) { - return origin_traits_.IsSameOrigin(a, b); + return TOriginTraits::IsSameOrigin(a, b); } std::string Serialize(const OriginType& origin) { - return origin_traits_.Serialize(origin); + return TOriginTraits::Serialize(origin); } bool IsValidUrl(base::StringPiece str) { - return origin_traits_.IsValidUrl(str); + return TOriginTraits::IsValidUrl(str); } #define EXPECT_SAME_ORIGIN(a, b) \ @@ -233,7 +224,6 @@ class AbstractOriginTest : public testing::Test { } private: - TOriginTraits origin_traits_; ScopedSchemeRegistryForTests scoped_scheme_registry_; }; @@ -499,25 +489,6 @@ REGISTER_TYPED_TEST_SUITE_P(AbstractOriginTest, CustomSchemes_OpaqueOrigins, CustomSchemes_TupleOrigins); -class UrlOriginTestTraits : public virtual OriginTraitsBase { - public: - OriginType CreateOriginFromString(base::StringPiece s) override; - OriginType CreateUniqueOpaqueOrigin() override; - OriginType CreateWithReferenceOrigin( - base::StringPiece url, - const OriginType& reference_origin) override; - OriginType DeriveNewOpaqueOrigin(const OriginType& reference_origin) override; - bool IsOpaque(const OriginType& origin) override; - std::string GetScheme(const OriginType& origin) override; - std::string GetHost(const OriginType& origin) override; - uint16_t GetPort(const OriginType& origin) override; - SchemeHostPort GetTupleOrPrecursorTupleIfOpaque( - const OriginType& origin) override; - bool IsSameOrigin(const OriginType& a, const OriginType& b) override; - std::string Serialize(const OriginType& origin) override; - bool IsValidUrl(base::StringPiece str) override; -}; - } // namespace url #endif // URL_ORIGIN_ABSTRACT_TESTS_H_