[Experimental Framework] Make the OriginTrialContext a member of ExecutionContext

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

Index: third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp
diff --git a/third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp b/third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..2b7d7b4c45972cfbdb07b3ac6ef666d4256449d6
--- /dev/null
+++ b/third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp
@@ -0,0 +1,167 @@
+// Copyright 2015 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 "core/origin_trials/DocumentOriginTrialContext.h"
+
+#include "core/HTMLNames.h"
+#include "core/dom/DOMException.h"
+#include "core/dom/ExceptionCode.h"
+#include "core/frame/FrameView.h"
+#include "core/html/HTMLDocument.h"
+#include "core/html/HTMLHeadElement.h"
+#include "core/html/HTMLMetaElement.h"
+#include "core/testing/DummyPageHolder.h"
+#include "platform/weborigin/KURL.h"
+#include "platform/weborigin/SecurityOrigin.h"
+#include "public/platform/WebTrialTokenValidator.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace blink {
+namespace {
+
+// API Key which will appear valid
+const char* kGoodTrialToken = "1|AnySignatureWillDo|https://www.example.com|Frobulate|2000000000";
+const char* kAnotherTrialToken = "1|AnySignatureWillDo|https://www.example.com|FrobulateV2|2000000000";
+
+class MockTokenValidator : public WebTrialTokenValidator {

[jbroman, 2016/02/29 20:41:53]

This class seems unused. (If you did have uses, I'd suggest using gmock
instead.)

[iclelland, 2016/03/01 21:51:42]

I've pulled it out now, thanks -- this was a holdover from splitting up the
tests between the original OriginTrialContextTest and this class. It's not used
here; the actual calls to the validator are tested in the other file.

+public:
+    MockTokenValidator()
+        : m_response(false)
+        , m_callCount(0)
+    {
+    }
+    ~MockTokenValidator() override {}
+
+    // blink::WebTrialTokenValidator implementation
+    bool validateToken(const blink::WebString& token, const blink::WebString& origin, const blink::WebString& featureName) override
+    {
+        m_callCount++;
+        return m_response;
+    }
+
+    // Useful methods for controlling the validator
+    void setResponse(bool response)
+    {
+        m_response = response;
+    }
+    void reset()
+    {
+        m_response = false;
+        m_callCount = 0;
+    }
+    int callCount()
+    {
+        return m_callCount;
+    }
+
+private:
+    bool m_response;
+    int m_callCount;
+
+    DISALLOW_COPY_AND_ASSIGN(MockTokenValidator);
+};
+
+} // namespace
+
+class DocumentOriginTrialContextTest : public ::testing::Test {
+protected:
+    DocumentOriginTrialContextTest()
+        : m_page(DummyPageHolder::create())
+        , m_frameworkWasEnabled(RuntimeEnabledFeatures::experimentalFrameworkEnabled())
+        , m_tokenValidator(adoptPtr(new MockTokenValidator()))
+    {
+        if (!RuntimeEnabledFeatures::experimentalFrameworkEnabled()) {

[jbroman, 2016/02/29 20:41:53]

This is just a field (the setter does nothing else), so you can write this
without the condition.

[iclelland, 2016/03/01 21:51:42]

It is, yes. I've changed the original code from OriginTrialContextTest to match
as well.

+            RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(true);
+        }
+    }
+
+    ~DocumentOriginTrialContextTest()
+    {
+        if (!m_frameworkWasEnabled) {

[jbroman, 2016/02/29 20:41:53]

similarly,
"RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(m_frameworkWasEnabled)"
seems more concise here, no?

[iclelland, 2016/03/01 21:51:42]

Yep :) Ditto here, too.

+            RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(false);
+        }
+
+        m_page.clear();
+    }
+
+    void SetUp() override
+    {
+        m_document = toHTMLDocument(&m_page->document());

[jbroman, 2016/02/29 20:41:53]

nit: do you need an HTMLDocument pointer (rather than a Document one) anywhere
here? No objections if you do, but otherwise this just seems like noise.

[iclelland, 2016/03/01 21:51:42]

Unneeded; removed.

+        setInnerHTML(
+            "<html>"
+            "<head>"
+            "</head>"
+            "<body>"
+            "</body>"
+            "</html>");
+    }
+
+    Document* document() { return &(m_page->document()); }
+    MockTokenValidator* tokenValidator() { return m_tokenValidator.get(); }
+    HTMLDocument& document() const { return *m_document; }

[jbroman, 2016/02/29 20:41:53]

This is confusing. You have both a non-const accessor that returns a Document*
and a const accessor that returns an HTMLDocument&. I strongly suspect these
should both be replaced with:

Document& document() { return m_pageHolder->document(); }

[iclelland, 2016/03/01 21:51:42]

Done.

+
+    void setPageOrigin(const String& origin)
+    {
+        KURL pageURL(ParsedURLString, origin);
+        RefPtr<SecurityOrigin> pageOrigin = SecurityOrigin::create(pageURL);
+        m_page->document().updateSecurityOrigin(pageOrigin);

[jbroman, 2016/02/29 20:41:53]

nit: you can just use document(), since you've already bothered to define that
convenient accessor

[iclelland, 2016/03/01 21:51:42]

Done.

+    }
+
+    void setInnerHTML(const char* htmlContent)
+    {
+        document()->documentElement()->setInnerHTML(String::fromUTF8(htmlContent), ASSERT_NO_EXCEPTION);
+        document()->view()->updateAllLifecyclePhases();
+    }
+
+    void addTrialToken(const String& token)
+    {
+        HTMLElement* head = document()->head();
+        ASSERT_TRUE(head);
+
+        RefPtrWillBeRawPtr<HTMLMetaElement> meta = HTMLMetaElement::create(*document());
+        meta->setAttribute(HTMLNames::nameAttr, OriginTrialContext::kTrialMetaTagName);
+        AtomicString value(token);
+        meta->setAttribute(HTMLNames::contentAttr, value);

[jbroman, 2016/02/29 20:41:53]

nit: no need to put the AtomicString construction on another line; you can just
write:
meta->setAttribute(HTMLNames::contentAttr, AtomicString(token));

Alternatively, this method could just take "const AtomicString&".

[iclelland, 2016/03/01 21:51:42]

Done, thanks for that tip

+        head->appendChild(meta.release());
+    }
+
+    Vector<String> getTokens()
+    {
+        DocumentOriginTrialContext ctx(document());

[jbroman, 2016/02/29 20:41:53]

Shouldn't this use the one owned by the document?

[iclelland, 2016/03/01 21:51:41]

Yes. Yes it should. The only reason it hadn't is that the context is private to
Document, and it was at least as easy to create a new one for the document to
test as to define friends and accessors to get to it from here. I've updated
that, though, as it really should just use the already created
OriginTrialContext.

+        return ctx.getTokens();
+    }
+
+private:
+    OwnPtr<DummyPageHolder> m_page;

[jbroman, 2016/02/29 20:41:53]

nit: a page holder isn't a page; suggest "m_pageHolder". I realize we're not
consistent elsewhere, but to me this seems especially confusing given that there
is such a thing as blink::Page.

[iclelland, 2016/03/01 21:51:42]

Done.

+    RefPtrWillBePersistent<HTMLDocument> m_document;
+    const bool m_frameworkWasEnabled;
+    OwnPtr<MockTokenValidator> m_tokenValidator;
+};
+
+TEST_F(DocumentOriginTrialContextTest, DetectsZeroTokens)
+{
+    String errorMessage;
+    Vector<String> tokens = getTokens();
+    EXPECT_EQ(tokens.size(), 0UL);
+}
+
+TEST_F(DocumentOriginTrialContextTest, ExtractsSingleToken)
+{
+    addTrialToken(kGoodTrialToken);
+    Vector<String> tokens = getTokens();
+    EXPECT_EQ(tokens.size(), 1UL);
+    EXPECT_EQ(tokens[0], kGoodTrialToken);
+}
+
+TEST_F(DocumentOriginTrialContextTest, ExtractsAllTokens)
+{
+    addTrialToken(kGoodTrialToken);
+    addTrialToken(kAnotherTrialToken);
+    Vector<String> tokens = getTokens();
+    EXPECT_EQ(tokens.size(), 2UL);

[jbroman, 2016/02/29 20:41:53]

Expected value is the left argument to EXPECT_EQ; this surfaces in the messages
gtest gives you on failure

[iclelland, 2016/03/01 21:51:41]

Yes it is; I've changed them all (I must have picked that habit up from python
unit test frameworks, I think)

+    EXPECT_TRUE(tokens.contains(kGoodTrialToken));
+    EXPECT_TRUE(tokens.contains(kAnotherTrialToken));
+}
+
+} // namespace blink