Store the list of trial tokens in OriginTrialContext

third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp

Index: third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
diff --git a/third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp b/third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
similarity index 74%
rename from third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
rename to third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
index c7ebd46f67b1f7324353791cd53fad67ca3affa6..04000529be899a3d8479aa65e86cec41ed340263 100644
--- a/third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp
+++ b/third_party/WebKit/Source/core/origin_trials/OriginTrialsBaseTest.cpp
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "core/origin_trials/OriginTrialContext.h"
+#include "core/origin_trials/OriginTrialsBase.h"
 
 #include "core/HTMLNames.h"
 #include "core/dom/DOMException.h"
@@ -68,76 +68,50 @@ private:
     DISALLOW_COPY_AND_ASSIGN(MockTokenValidator);
 };
 
-// Concrete subclass of OriginTrialContext which simply maintains a vector of
-// token strings to use for tests.
-class TestOriginTrialContext : public OriginTrialContext {
+// Concrete subclass of OriginTrialsBase to expose test functionality.
+class TestOriginTrials : public OriginTrialsBase {
 public:
-    explicit TestOriginTrialContext()
-        : m_parent(adoptRefWillBeNoop(new NullExecutionContext()))
+    explicit TestOriginTrials(ExecutionContext* parent)
+        : OriginTrialsBase(parent)
     {
     }
 
-    ~TestOriginTrialContext() override = default;
-
-    ExecutionContext* getExecutionContext() override { return m_parent.get(); }
-
-    void addToken(const String& token)
-    {
-        m_tokens.append(token);
-    }
-
-    void updateSecurityOrigin(const String& origin)
-    {
-        KURL pageURL(ParsedURLString, origin);
-        RefPtr<SecurityOrigin> pageOrigin = SecurityOrigin::create(pageURL);
-        m_parent->setSecurityOrigin(pageOrigin);
-        m_parent->setIsSecureContext(SecurityOrigin::isSecure(pageURL));
-    }
-
-    Vector<String> getTokens() override
-    {
-        Vector<String> tokens;
-        for (String token : m_tokens) {
-            tokens.append(token);
-        }
-        return tokens;
-    }
-
-    DEFINE_INLINE_VIRTUAL_TRACE()
-    {
-        visitor->trace(m_parent);
-        OriginTrialContext::trace(visitor);
-    }
-
-private:
-    RefPtrWillBeMember<NullExecutionContext> m_parent;
-    Vector<String> m_tokens;
+    using OriginTrialsBase::isFeatureEnabled;

[iclelland, 2016/03/31 15:00:00]

I had no idea you could do that -- that's amazingly useful, thanks!

[iclelland, 2016/03/31 18:05:20]

Although, as chasej@ points out to me, isn't it more common in Chromium to
declare OriginTrialsBaseTest a friend of OriginTrialsBase, and then have access
to its internals that way, rather than subclassing to expose methods?

[Marijn Kruisselbrink, 2016/03/31 19:01:48]

I generally try to avoid friending whenever I can. Most of the time an API that
can only be tested by having access to implementation details/internals of the
class your testing is a pretty bad API. Also depending on internals rather than
on parts that the class being tested has explicitly made available makes
refactoring much harder (and makes it more likely that you're tests aren't
actually testing what you want to test but just test one particular
implementation).

(more specifically about using "using" this way, I'm not actually sure it's
allowed. It's a c++11 thing that wasn't supported by MSVC2013. The c++11 allowed
features page doesn't mention it as either allowed or denied (it only mentions
using for constructors). So I'd probably replace it with an explicit
isFeatureEnabled method that just calls OriginTrialsBase::isFeatureEnabled. Or
just add a isFeatureEnabledForTesting to OriginTrialsBase; which would still be
way better than adding a friend definition as it would be enforced by presubmit
hooks that only test code calls it).

[iclelland, 2016/04/01 15:19:09]

Reading some more, it looks like this use should be allowed. MSDN says it's
available at least as far back as MSVC2010
https://msdn.microsoft.com/en-us/library/was37tzw(v=vs.100).aspx -- see B::g in
the last example on that page.

AFAICT, this goes back at least to C++03, which would explain its absence from
the C++11 allowed features list.
Inherited constructors, *are* a more recent (C++11) addition, and may still be
buggy in MSVC2013, although they're listed as supported. (And I think we
switched to 2015 recently anyway)

 };
 
 } // namespace
 
-class OriginTrialContextTest : public ::testing::Test {
+class OriginTrialsBaseTest : public ::testing::Test {
 protected:
-    OriginTrialContextTest()
+    OriginTrialsBaseTest()
         : m_frameworkWasEnabled(RuntimeEnabledFeatures::experimentalFrameworkEnabled())
+        , m_executionContext(adoptRefWillBeNoop(new NullExecutionContext()))
         , m_tokenValidator(adoptPtr(new MockTokenValidator()))
-        , m_originTrialContext(adoptPtrWillBeNoop(new TestOriginTrialContext))
+        , m_originTrials(adoptPtrWillBeNoop(new TestOriginTrials(m_executionContext.get())))
     {
         RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(true);
     }
 
-    ~OriginTrialContextTest()
+    ~OriginTrialsBaseTest()
     {
         RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(m_frameworkWasEnabled);
     }
 
     MockTokenValidator* tokenValidator() { return m_tokenValidator.get(); }
 
+    void updateSecurityOrigin(const String& origin)
+    {
+        KURL pageURL(ParsedURLString, origin);
+        RefPtr<SecurityOrigin> pageOrigin = SecurityOrigin::create(pageURL);
+        m_executionContext->setSecurityOrigin(pageOrigin);
+        m_executionContext->setIsSecureContext(SecurityOrigin::isSecure(pageURL));
+    }
+
     bool isFeatureEnabled(const String& origin, const String& featureName, const String& token, String* errorMessage)
     {
-        m_originTrialContext->updateSecurityOrigin(origin);
-        m_originTrialContext->addToken(token);
-        return m_originTrialContext->isFeatureEnabled(featureName, errorMessage, tokenValidator());
+        updateSecurityOrigin(origin);
+        m_originTrials->addToken(token);
+        return m_originTrials->isFeatureEnabled(featureName, errorMessage, tokenValidator());
     }
 
     bool isFeatureEnabledWithoutErrorMessage(const String& origin, const String& featureName, const char* token)
@@ -147,11 +121,12 @@ protected:
 
 private:
     const bool m_frameworkWasEnabled;
+    RefPtrWillBePersistent<NullExecutionContext> m_executionContext;
     OwnPtr<MockTokenValidator> m_tokenValidator;
-    OwnPtrWillBePersistent<TestOriginTrialContext> m_originTrialContext;
+    OwnPtrWillBePersistent<TestOriginTrials> m_originTrials;
 };
 
-TEST_F(OriginTrialContextTest, EnabledNonExistingFeature)
+TEST_F(OriginTrialsBaseTest, EnabledNonExistingFeature)
 {
     String errorMessage;
     bool isNonExistingFeatureEnabled = isFeatureEnabled(kFrobulateEnabledOrigin,
@@ -162,7 +137,7 @@ TEST_F(OriginTrialContextTest, EnabledNonExistingFeature)
     EXPECT_EQ(("The provided token(s) are not valid for the 'This feature does not exist' feature."), errorMessage);
 }
 
-TEST_F(OriginTrialContextTest, EnabledNonExistingFeatureWithoutErrorMessage)
+TEST_F(OriginTrialsBaseTest, EnabledNonExistingFeatureWithoutErrorMessage)
 {
     bool isNonExistingFeatureEnabled = isFeatureEnabledWithoutErrorMessage(
         kFrobulateEnabledOrigin,
@@ -172,7 +147,7 @@ TEST_F(OriginTrialContextTest, EnabledNonExistingFeatureWithoutErrorMessage)
 }
 
 // The feature should be enabled if a valid token for the origin is provided
-TEST_F(OriginTrialContextTest, EnabledSecureRegisteredOrigin)
+TEST_F(OriginTrialsBaseTest, EnabledSecureRegisteredOrigin)
 {
     String errorMessage;
     tokenValidator()->setResponse(true);
@@ -187,7 +162,7 @@ TEST_F(OriginTrialContextTest, EnabledSecureRegisteredOrigin)
 
 // ... but if the browser says it's invalid for any reason, that's enough to
 // reject.
-TEST_F(OriginTrialContextTest, InvalidTokenResponseFromPlatform)
+TEST_F(OriginTrialsBaseTest, InvalidTokenResponseFromPlatform)
 {
     String errorMessage;
     tokenValidator()->setResponse(false);
@@ -200,7 +175,7 @@ TEST_F(OriginTrialContextTest, InvalidTokenResponseFromPlatform)
     EXPECT_EQ(1, tokenValidator()->callCount());
 }
 
-TEST_F(OriginTrialContextTest, OnlyOneErrorMessageGenerated)
+TEST_F(OriginTrialsBaseTest, OnlyOneErrorMessageGenerated)
 {
     String errorMessage1;
     String errorMessage2;
@@ -211,7 +186,7 @@ TEST_F(OriginTrialContextTest, OnlyOneErrorMessageGenerated)
     EXPECT_TRUE(errorMessage2.isEmpty());
 }
 
-TEST_F(OriginTrialContextTest, ErrorMessageClearedIfStringReused)
+TEST_F(OriginTrialsBaseTest, ErrorMessageClearedIfStringReused)
 {
     String errorMessage;
     tokenValidator()->setResponse(false);
@@ -221,7 +196,7 @@ TEST_F(OriginTrialContextTest, ErrorMessageClearedIfStringReused)
     EXPECT_TRUE(errorMessage.isEmpty());
 }
 
-TEST_F(OriginTrialContextTest, ErrorMessageGeneratedPerFeature)
+TEST_F(OriginTrialsBaseTest, ErrorMessageGeneratedPerFeature)
 {
     String errorMessage1;
     String errorMessage2;
@@ -232,7 +207,7 @@ TEST_F(OriginTrialContextTest, ErrorMessageGeneratedPerFeature)
     EXPECT_FALSE(errorMessage2.isEmpty());
 }
 
-TEST_F(OriginTrialContextTest, EnabledSecureRegisteredOriginWithoutErrorMessage)
+TEST_F(OriginTrialsBaseTest, EnabledSecureRegisteredOriginWithoutErrorMessage)
 {
     tokenValidator()->setResponse(true);
     bool isOriginEnabled = isFeatureEnabledWithoutErrorMessage(
@@ -245,7 +220,7 @@ TEST_F(OriginTrialContextTest, EnabledSecureRegisteredOriginWithoutErrorMessage)
 
 // The feature should not be enabled if the origin is unsecure, even if a valid
 // token for the origin is provided
-TEST_F(OriginTrialContextTest, EnabledNonSecureRegisteredOrigin)
+TEST_F(OriginTrialsBaseTest, EnabledNonSecureRegisteredOrigin)
 {
     String errorMessage;
     bool isOriginEnabled = isFeatureEnabled(kFrobulateEnabledOriginUnsecure,
@@ -257,7 +232,7 @@ TEST_F(OriginTrialContextTest, EnabledNonSecureRegisteredOrigin)
     EXPECT_FALSE(errorMessage.isEmpty());
 }
 
-TEST_F(OriginTrialContextTest, EnabledNonSecureRegisteredOriginWithoutErrorMessage)
+TEST_F(OriginTrialsBaseTest, EnabledNonSecureRegisteredOriginWithoutErrorMessage)
 {
     bool isOriginEnabled = isFeatureEnabledWithoutErrorMessage(
         kFrobulateEnabledOriginUnsecure,