Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1743623002:
[Experimental Framework] Make the OriginTrialContext a member of ExecutionContext (Closed)
|
Created:
4 years, 10 months ago by iclelland Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, chasej+watch_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Experimental Framework] Make the OriginTrialContext a supplement to ExecutionContext
This patch makes OriginTrialContext a supplement to the ExecutionContext object,
rather than being a collection of static methods. This allows us to maintain
state on the object, and allows it to be specialized for the different types of
execution contexts, so that document contexts can be handled differently than
workers, for instance.
BUG=582063
Committed: https://crrev.com/c970675cb18a6ba91220d95128dced7b3ffd24b2
Cr-Commit-Position: refs/heads/master@{#379575}
Patch Set 1 #Patch Set 2 : Fix broken constructor #Patch Set 3 : Fix for pre-oilpan builds #
Total comments: 17
Patch Set 4 : Avoid changing bindings code for now #Patch Set 5 : Addressing first round comments from PS#3 #
Total comments: 5
Patch Set 6 : Fix nullptr crash, test that an error message was set #
Total comments: 35
Patch Set 7 : Addressing most of the comments from PS#6 #Patch Set 8 : Rebase #Patch Set 9 : Remove reference cycle in DocumentOriginTrialContext #Patch Set 10 : Use WeakPersistent for Oilpan #
Total comments: 20
Patch Set 11 : Move everything to the oilpan heap, Make OTC a supplement, and streamline tests #Patch Set 12 : Clean up unused headers #
Total comments: 12
Patch Set 13 : Fixing nits #Patch Set 14 : Rebase #Dependent Patchsets: Messages
Total messages: 30 (8 generated)
iclelland@chromium.org changed reviewers: + chasej@chromium.org
+r chasej
https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:28: Vector<String> getTokensForFeatureName(const String&) override; The getTokensForFeatureName() is an implementation detail of the overall algorithm for checking enabled. I would prefer to see it protected. I see it used in tests, could just make those friends? Also, I'm assuming question about using protected (in the preceding comment) just applies to this method? https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:160: // The API should be enabled if a valid key for the origin is provided I'm guessing this comment is a hold-over from the previous implementation. Probably should be removed? https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:168: EXPECT_EQ(tokens[1], kAnotherTrialToken); I assume that getTokensForFeatureName() doesn't need to guarantee that the results vector matches the order in which tokens appear in the document. For other types of context, it might be difficult/infeasible to guarantee a specific order. As written, this test suggests to me that the order is deterministic and/or matches document order. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (left): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:20: const char kTrialMetaTagName[] = "origin-trials"; I would prefer to leave this defined by the OriginTrialContext. Yes, it's only used in documents right now, but when we add support for HTTP headers, I think the various contexts should be sharing the same name. Later, we'll probably want to rename, since it won't just be a <meta> tag name. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:38: virtual Vector<String> getTokensForFeatureName(const String&) = 0; Why getTokensForFeatureName() instead of getTokens()? Looking at the DocumentOriginTrialContext override, it seems to be ignoring the provided feature name. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (left): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:222: EXPECT_TRUE(errorMessage.contains("secure origin")) << "Message should indicate only secure origins are allowed, was: " << errorMessage; I understand that there is no error message produced, because this test is using the NullExecutionContext. If the OriginTrialContext is relying on the ExecutionContext to produce a message, then it probably makes sense not to verify the exact contents of the message. I think the OriginTrialContext should always produce some message. Otherwise, this failure case is inconsistent with the others, and developers might be at loss without the help of a message (assuming the message is eventually surfaced in devtools). Perhaps OriginTrialContext should check if ExecutionContext.isSecureContext() returns a message, and generate it's own when that is empty?
https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:28: Vector<String> getTokensForFeatureName(const String&) override; On 2016/02/26 17:57:53, chasej wrote: > The getTokensForFeatureName() is an implementation detail of the overall > algorithm for checking enabled. I would prefer to see it protected. I see it > used in tests, could just make those friends? > > Also, I'm assuming question about using protected (in the preceding comment) > just applies to this method? Yes -- Sorry for leaving that in there; it was my own question to myself: "Should this be protected?". I'll make it so, and friend the tests. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:168: EXPECT_EQ(tokens[1], kAnotherTrialToken); On 2016/02/26 17:57:53, chasej wrote: > I assume that getTokensForFeatureName() doesn't need to guarantee that the > results vector matches the order in which tokens appear in the document. For > other types of context, it might be difficult/infeasible to guarantee a specific > order. As written, this test suggests to me that the order is deterministic > and/or matches document order. Good catch; I'll rewrite to search the result instead. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (left): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:20: const char kTrialMetaTagName[] = "origin-trials"; On 2016/02/26 17:57:53, chasej wrote: > I would prefer to leave this defined by the OriginTrialContext. Yes, it's only > used in documents right now, but when we add support for HTTP headers, I think > the various contexts should be sharing the same name. Later, we'll probably want > to rename, since it won't just be a <meta> tag name. I'll have to make it a public (or at least protected) member, then, rather than a static here. Can do; I'm not particular either way. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:38: virtual Vector<String> getTokensForFeatureName(const String&) = 0; On 2016/02/26 17:57:54, chasej wrote: > Why getTokensForFeatureName() instead of getTokens()? Looking at the > DocumentOriginTrialContext override, it seems to be ignoring the provided > feature name. Originally I had considered that it would (could?) be optimized to just provide the likely-relevant tokens, but this particular implementation is naive and doesn't even look at their content.
https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:28: Vector<String> getTokensForFeatureName(const String&) override; On 2016/02/26 21:15:56, iclelland wrote: > On 2016/02/26 17:57:53, chasej wrote: > > The getTokensForFeatureName() is an implementation detail of the overall > > algorithm for checking enabled. I would prefer to see it protected. I see it > > used in tests, could just make those friends? > > > > Also, I'm assuming question about using protected (in the preceding comment) > > just applies to this method? > > Yes -- Sorry for leaving that in there; it was my own question to myself: > "Should this be protected?". I'll make it so, and friend the tests. Done. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:160: // The API should be enabled if a valid key for the origin is provided On 2016/02/26 17:57:53, chasej wrote: > I'm guessing this comment is a hold-over from the previous implementation. > Probably should be removed? Done. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:168: EXPECT_EQ(tokens[1], kAnotherTrialToken); On 2016/02/26 21:15:56, iclelland wrote: > On 2016/02/26 17:57:53, chasej wrote: > > I assume that getTokensForFeatureName() doesn't need to guarantee that the > > results vector matches the order in which tokens appear in the document. For > > other types of context, it might be difficult/infeasible to guarantee a > specific > > order. As written, this test suggests to me that the order is deterministic > > and/or matches document order. > > Good catch; I'll rewrite to search the result instead. Done. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (left): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:20: const char kTrialMetaTagName[] = "origin-trials"; On 2016/02/26 21:15:56, iclelland wrote: > On 2016/02/26 17:57:53, chasej wrote: > > I would prefer to leave this defined by the OriginTrialContext. Yes, it's only > > used in documents right now, but when we add support for HTTP headers, I think > > the various contexts should be sharing the same name. Later, we'll probably > want > > to rename, since it won't just be a <meta> tag name. > > I'll have to make it a public (or at least protected) member, then, rather than > a static here. Can do; I'm not particular either way. Done. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:38: virtual Vector<String> getTokensForFeatureName(const String&) = 0; On 2016/02/26 21:15:56, iclelland wrote: > On 2016/02/26 17:57:54, chasej wrote: > > Why getTokensForFeatureName() instead of getTokens()? Looking at the > > DocumentOriginTrialContext override, it seems to be ignoring the provided > > feature name. > > Originally I had considered that it would (could?) be optimized to just provide > the likely-relevant tokens, but this particular implementation is naive and > doesn't even look at their content. Done. https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (left): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:222: EXPECT_TRUE(errorMessage.contains("secure origin")) << "Message should indicate only secure origins are allowed, was: " << errorMessage; On 2016/02/26 17:57:54, chasej wrote: > I understand that there is no error message produced, because this test is using > the NullExecutionContext. If the OriginTrialContext is relying on the > ExecutionContext to produce a message, then it probably makes sense not to > verify the exact contents of the message. > > I think the OriginTrialContext should always produce some message. Otherwise, > this failure case is inconsistent with the others, and developers might be at > loss without the help of a message (assuming the message is eventually surfaced > in devtools). If the origin trial context always produces a message, then we should always *use* that message, I think. Otherwise, there's a conflict between saying "We know what went wrong, and what to tell the user", and "The execution content knows what's wrong, and knows what to tell the user". > Perhaps OriginTrialContext should check if ExecutionContext.isSecureContext() > returns a message, and generate it's own when that is empty? I really don't want to go this way -- There are only three non-test ExecutionContexts, and they all *do* provide a message, so any message that we add is going to be unexercised code, added into mainline code only for the sake of tests, and for some vaguely-possible future condition. I see two ways to resolve this: One is to avoid using the EC's message at all, but defer directly to the origin's isPotentiallyTrustworthyErrorMessage() method (which is, in fact, what the ECs do anyway). The other is to add a DCHECK in code that the execution context *does* provide a message, and if we ever come across a new place where it doesn't, then we'll be alerted right away, and hopefully the right course of action will be clear at that point. (Either change that specific EC to produce a message, or replace all EC messages with our own message in OTC). I'm leaning towards the second option, on the grounds that it's the execution context that we actually *asked*, not the Origin, so it may eventually have another, more specific reason for failing the isSecureContext() check, and we should use that if it is available. I'll implement that now, but if you feel strongly that the other way is correct, then I'll change it.
iclelland@chromium.org changed reviewers: + jbroman@chromium.org
+r jbroman -- would you be able to take a look at this? Thanks!
https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (left): https://codereview.chromium.org/1743623002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:222: EXPECT_TRUE(errorMessage.contains("secure origin")) << "Message should indicate only secure origins are allowed, was: " << errorMessage; On 2016/02/29 15:00:22, iclelland wrote: > On 2016/02/26 17:57:54, chasej wrote: > > I understand that there is no error message produced, because this test is > using > > the NullExecutionContext. If the OriginTrialContext is relying on the > > ExecutionContext to produce a message, then it probably makes sense not to > > verify the exact contents of the message. > > > > I think the OriginTrialContext should always produce some message. Otherwise, > > this failure case is inconsistent with the others, and developers might be at > > loss without the help of a message (assuming the message is eventually > surfaced > > in devtools). > > If the origin trial context always produces a message, then we should always > *use* that message, I think. Otherwise, there's a conflict between saying "We > know what went wrong, and what to tell the user", and "The execution content > knows what's wrong, and knows what to tell the user". > > > Perhaps OriginTrialContext should check if ExecutionContext.isSecureContext() > > returns a message, and generate it's own when that is empty? > > I really don't want to go this way -- There are only three non-test > ExecutionContexts, and they all *do* provide a message, so any message that we > add is going to be unexercised code, added into mainline code only for the sake > of tests, and for some vaguely-possible future condition. > > I see two ways to resolve this: One is to avoid using the EC's message at all, > but defer directly to the origin's isPotentiallyTrustworthyErrorMessage() method > (which is, in fact, what the ECs do anyway). > > The other is to add a DCHECK in code that the execution context *does* provide a > message, and if we ever come across a new place where it doesn't, then we'll be > alerted right away, and hopefully the right course of action will be clear at > that point. (Either change that specific EC to produce a message, or replace all > EC messages with our own message in OTC). > > I'm leaning towards the second option, on the grounds that it's the execution > context that we actually *asked*, not the Origin, so it may eventually have > another, more specific reason for failing the isSecureContext() check, and we > should use that if it is available. > > I'll implement that now, but if you feel strongly that the other way is correct, > then I'll change it. I like the second option to add a DCHECK. It addresses my main concern about always providing an error message, without some of the drawbacks. https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:54: DCHECK(!errorMessage->isEmpty()); Need to check if errorMessage was provided, it can be null. Is it worth adding a test (i.e. EnabledNonSecureRegisteredOriginWithoutErrorMessage)? It appears none of the existing tests cover the lack of a pointer check. I'm not sure, because that's the kind of test added for 100% coverage, which is probably not the goal here. https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:223: EXPECT_NE("Unset", errorMessage); Should this set the message to empty (instead of "Unset"), and make sure the resulting message is non-empty? In the implementation, the comment says it expects an error message to be provided from isSecureContext(). I think this test would pass if isSecureContext() returned an empty message.
https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:54: DCHECK(!errorMessage->isEmpty()); On 2016/02/29 18:46:49, chasej wrote: > Need to check if errorMessage was provided, it can be null. Thanks, fixed. > > Is it worth adding a test (i.e. > EnabledNonSecureRegisteredOriginWithoutErrorMessage)? It appears none of the > existing tests cover the lack of a pointer check. I'm not sure, because that's > the kind of test added for 100% coverage, which is probably not the goal here. Yes, it's probably good for robustness to make sure that that works. This is just the *first* bug that such a test would have caught. There may one day be more. Done. https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:223: EXPECT_NE("Unset", errorMessage); On 2016/02/29 18:46:49, chasej wrote: > Should this set the message to empty (instead of "Unset"), and make sure the > resulting message is non-empty? > > In the implementation, the comment says it expects an error message to be > provided from isSecureContext(). I think this test would pass if > isSecureContext() returned an empty message. Done.
https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp:30: for (HTMLMetaElement* metaElement = head ? Traversal<HTMLMetaElement>::firstChild(*head) : 0; metaElement; metaElement = Traversal<HTMLMetaElement>::nextSibling(*metaElement)) { nit: this loop can be written more concisely: if (!head) { for (const auto& metaElement : Traversal<HTMLMetaElement>::childrenOf(*head)) { // ... } } https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1409: OwnPtr<DocumentOriginTrialContext> m_originTrialContext; nit: Why not just make this a plain member of Document? Then you wouldn't need the extra heap allocation (the originTrailContext override would be "return &m_originTrialContext;"). It looks like it's only sizeof(Document*) large anyhow. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp:14: #include "wtf/CurrentTime.h" several unused headers here; please include what you use https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:9: #include "core/dom/DOMException.h" unused header? https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:12: #include "wtf/Vector.h" I think you only need wtf/Forward.h. Not a big deal here, but if you do end up including this header in Document.h then keeping the included headers down would be a plus. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:35: RefPtrWillBePersistent<Document> m_parent; Can this object ever need to extend the lifetime of the document? Isn't it always owned by this document? (why not Document*?) https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:27: class MockTokenValidator : public WebTrialTokenValidator { This class seems unused. (If you did have uses, I'd suggest using gmock instead.) https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:74: if (!RuntimeEnabledFeatures::experimentalFrameworkEnabled()) { This is just a field (the setter does nothing else), so you can write this without the condition. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:81: if (!m_frameworkWasEnabled) { similarly, "RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(m_frameworkWasEnabled)" seems more concise here, no? https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:90: m_document = toHTMLDocument(&m_page->document()); 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. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:102: HTMLDocument& document() const { return *m_document; } 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(); } https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:108: m_page->document().updateSecurityOrigin(pageOrigin); nit: you can just use document(), since you've already bothered to define that convenient accessor https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:125: meta->setAttribute(HTMLNames::contentAttr, value); 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&". https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:131: DocumentOriginTrialContext ctx(document()); Shouldn't this use the one owned by the document? https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:136: OwnPtr<DummyPageHolder> m_page; 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. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:162: EXPECT_EQ(tokens.size(), 2UL); Expected value is the left argument to EXPECT_EQ; this surfaces in the messages gtest gives you on failure https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:9: #include "core/dom/DOMException.h" nit: There are a number of headers here, like DOMException and ExecutionContext, that it's not clear are needed in this header. Not added in this CL, but thought I'd mention it.
https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:231: nullptr /* errorMessage */); This should call isFeatureEnabledWithoutErrorMessage(), like other similar tests.
https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:35: RefPtrWillBePersistent<Document> m_parent; On 2016/02/29 20:41:53, jbroman wrote: > Can this object ever need to extend the lifetime of the document? Isn't it > always owned by this document? (why not Document*?) This'll create a Document leak - the owned object (DocumentOriginTrialContext) keeping the owner alive.
Thanks for the thorough review, jbroman -- the unit tests were only very roughly split into two files; I should have cleaned that up more before sending it your way. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp:14: #include "wtf/CurrentTime.h" On 2016/02/29 20:41:53, jbroman wrote: > several unused headers here; please include what you use Done. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:9: #include "core/dom/DOMException.h" On 2016/02/29 20:41:53, jbroman wrote: > unused header? Yes. Removed. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:12: #include "wtf/Vector.h" On 2016/02/29 20:41:53, jbroman wrote: > I think you only need wtf/Forward.h. Not a big deal here, but if you do end up > including this header in Document.h then keeping the included headers down would > be a plus. Done. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:35: RefPtrWillBePersistent<Document> m_parent; On 2016/02/29 20:41:53, jbroman wrote: > Can this object ever need to extend the lifetime of the document? Isn't it > always owned by this document? (why not Document*?) Oilpan complains "Raw pointer field 'm_parent' to a GC managed class" when I do that. I've replaced it with RawPtrWillBeWeakPersistent to make both Oilpan and pre-oilpan builds happy. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:27: class MockTokenValidator : public WebTrialTokenValidator { On 2016/02/29 20:41:53, jbroman wrote: > This class seems unused. (If you did have uses, I'd suggest using gmock > instead.) 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. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:74: if (!RuntimeEnabledFeatures::experimentalFrameworkEnabled()) { On 2016/02/29 20:41:53, jbroman wrote: > This is just a field (the setter does nothing else), so you can write this > without the condition. It is, yes. I've changed the original code from OriginTrialContextTest to match as well. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:81: if (!m_frameworkWasEnabled) { On 2016/02/29 20:41:53, jbroman wrote: > similarly, > "RuntimeEnabledFeatures::setExperimentalFrameworkEnabled(m_frameworkWasEnabled)" > seems more concise here, no? Yep :) Ditto here, too. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:90: m_document = toHTMLDocument(&m_page->document()); On 2016/02/29 20:41:53, jbroman wrote: > 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. Unneeded; removed. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:102: HTMLDocument& document() const { return *m_document; } On 2016/02/29 20:41:53, jbroman wrote: > 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(); } Done. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:108: m_page->document().updateSecurityOrigin(pageOrigin); On 2016/02/29 20:41:53, jbroman wrote: > nit: you can just use document(), since you've already bothered to define that > convenient accessor Done. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:125: meta->setAttribute(HTMLNames::contentAttr, value); On 2016/02/29 20:41:53, jbroman wrote: > 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&". Done, thanks for that tip https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:131: DocumentOriginTrialContext ctx(document()); On 2016/02/29 20:41:53, jbroman wrote: > Shouldn't this use the one owned by the document? 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. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:136: OwnPtr<DummyPageHolder> m_page; On 2016/02/29 20:41:53, jbroman wrote: > 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. Done. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:162: EXPECT_EQ(tokens.size(), 2UL); On 2016/02/29 20:41:53, jbroman wrote: > Expected value is the left argument to EXPECT_EQ; this surfaces in the messages > gtest gives you on failure Yes it is; I've changed them all (I must have picked that habit up from python unit test frameworks, I think) https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:9: #include "core/dom/DOMException.h" On 2016/02/29 20:41:53, jbroman wrote: > nit: There are a number of headers here, like DOMException and ExecutionContext, > that it's not clear are needed in this header. Not added in this CL, but thought > I'd mention it. Thanks; I've tried to rationalize it now. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:231: nullptr /* errorMessage */); On 2016/02/29 21:14:47, chasej wrote: > This should call isFeatureEnabledWithoutErrorMessage(), like other similar > tests. Done. https://codereview.chromium.org/1743623002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:231: nullptr /* errorMessage */); On 2016/02/29 21:14:47, chasej wrote: > This should call isFeatureEnabledWithoutErrorMessage(), like other similar > tests. Done.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:34: RawPtrWillBeWeakPersistent<Document> m_parent; Please move DocumentOriginTrialContext to the Oilpan heap rather than use WeakPersistent<> for a back pointer.
Sorry to be a pain, but I'm still having some trouble with this factoring. I think I'd suggest (I say this in pieces below): ExecutionContext owns OriginTrials (via WillBeHeapSupplement<> or OwnPtrWillBeMember<>) OriginTrials owns (and delegates to) OriginTrialsContext (owned via OwnPtrWillBeMember<>) particular implementations of OriginTrialsContext have a RawPtrWillBeMember<> pointing back to their ExecutionContext Where ExecutionContext only knows enough to create the delegate, and the delegate only knows the details particular to the kind of ExecutionContext. (If I were painting bikesheds, I'd have it named OriginTrialsDelegate.) https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1410: OwnPtr<DocumentOriginTrialContext> m_originTrialContext; Why do we need a separate member here, when we only use it to create originTrialContext? It seems wasteful to have a field that is only ever read once. If Document and DocumentOriginTrialContext are going to be separate objects anyhow, how about having the method be something like: PassOwnPtrWillBeRawPtr<OriginTrialContext> createOriginTrialContext() override; And then the owning pointer would be held by the OriginTrials object, and no field would be needed here. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ExecutionContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ExecutionContext.h:208: OwnPtr<OriginTrials> m_originTrials; One thought here: consider using a supplement (WillBeHeapSupplement<ExecutionContext>, see platform/Supplementable.h) instead of adding a field to every execution context, especially since this is likely to be rare (at least until developers start using the origin trials feature). ExecutionContext is already supplementable. I'd prefer to keep the number of feature-specific fields in ExecutionContext low if possible. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp:25: for (HTMLMetaElement* metaElement = head ? Traversal<HTMLMetaElement>::firstChild(*head) : 0; metaElement; metaElement = Traversal<HTMLMetaElement>::nextSibling(*metaElement)) { nit: still like iterating over Traversal<HTMLMetaElement>::childrenOf(...) instead of writing out the firstChild and nextSibling calls https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:34: RawPtrWillBeWeakPersistent<Document> m_parent; On 2016/03/01 at 21:55:21, sof wrote: > Please move DocumentOriginTrialContext to the Oilpan heap rather than use WeakPersistent<> for a back pointer. Agreed that WeakPersistent is icky. If Document* is invalid (it's not clear to me that it should be, if ), then I agree that this should just be a GarbageCollected object, and can hold a Member<Document> (or WeakMember<Document>, but IIUC Member is cheaper). https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:83: return reinterpret_cast<DocumentOriginTrialContext*>(document().originTrialContext())->getTokens(); This should not be a reinterpret_cast. If you need to cast it, you should static_cast. But I don't think you need to cast it at all, since getTokens is defined on the superclass. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:84: } It seems odd for a unit test to be testing the protected member of the class. Is there a reason this is factored this way? I'd have expected the thing you want to test to be public, especially for such a small class. This suggests to me that either this test should be calling the public methods that exist now, or OriginTrialContext should be a pure virtual interface, with the rest of the logic in OriginTrials. WDYT? https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:111: EXPECT_EQ(2UL, tokens.size()); You could use gmock to make this a little more concise: EXPECT_THAT(getTokens(), UnorderedElementsAre(kGoodTrialToken, kAnotherTrialToken)); https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:30: // This class is not intended to be instantiated. Any required state is kept Out of date comment? You do seem to instantiate it. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:229: EXPECT_EQ(0, tokenValidator()->callCount()); Not added in this CL (so I won't make you change it), but you could have used gmock to make this mock class much more concisely, and then this would be: EXPECT_CALL(tokenValidator(), validateToken(_, _, _)).Times(0); Just mentioning it in case you hadn't considered it.
https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:34: RawPtrWillBeWeakPersistent<Document> m_parent; On 2016/03/01 23:29:21, jbroman wrote: > On 2016/03/01 at 21:55:21, sof wrote: > > Please move DocumentOriginTrialContext to the Oilpan heap rather than use > WeakPersistent<> for a back pointer. > > Agreed that WeakPersistent is icky. If Document* is invalid (it's not clear to > me that it should be, if ), then I agree that this should just be a > GarbageCollected object, and can hold a Member<Document> (or > WeakMember<Document>, but IIUC Member is cheaper). For an example of owner-ownee w/Oilpan that's near identical to this one, see VisitedLinkState.
On 2016/03/02 12:36:29, sof wrote: > https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h > (right): > > https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:34: > RawPtrWillBeWeakPersistent<Document> m_parent; > On 2016/03/01 23:29:21, jbroman wrote: > > On 2016/03/01 at 21:55:21, sof wrote: > > > Please move DocumentOriginTrialContext to the Oilpan heap rather than use > > WeakPersistent<> for a back pointer. > > > > Agreed that WeakPersistent is icky. If Document* is invalid (it's not clear to > > me that it should be, if ), then I agree that this should just be a > > GarbageCollected object, and can hold a Member<Document> (or > > WeakMember<Document>, but IIUC Member is cheaper). > > For an example of owner-ownee w/Oilpan that's near identical to this one, see > VisitedLinkState. Thanks, Sigbjorn, I appreciate the pointer there. I'll read through that and apply what I can to this work.
https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1410: OwnPtr<DocumentOriginTrialContext> m_originTrialContext; On 2016/03/01 23:29:21, jbroman wrote: > Why do we need a separate member here, when we only use it to create > originTrialContext? It seems wasteful to have a field that is only ever read > once. > > If Document and DocumentOriginTrialContext are going to be separate objects > anyhow, how about having the method be something like: > > PassOwnPtrWillBeRawPtr<OriginTrialContext> createOriginTrialContext() override; > > And then the owning pointer would be held by the OriginTrials object, and no > field would be needed here. Done. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ExecutionContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ExecutionContext.h:208: OwnPtr<OriginTrials> m_originTrials; On 2016/03/01 23:29:21, jbroman wrote: > One thought here: consider using a supplement > (WillBeHeapSupplement<ExecutionContext>, see platform/Supplementable.h) instead > of adding a field to every execution context, especially since this is likely to > be rare (at least until developers start using the origin trials feature). > ExecutionContext is already supplementable. > > I'd prefer to keep the number of feature-specific fields in ExecutionContext low > if possible. That's a really good idea. Thanks, done. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.cpp:25: for (HTMLMetaElement* metaElement = head ? Traversal<HTMLMetaElement>::firstChild(*head) : 0; metaElement; metaElement = Traversal<HTMLMetaElement>::nextSibling(*metaElement)) { On 2016/03/01 23:29:21, jbroman wrote: > nit: still like iterating over Traversal<HTMLMetaElement>::childrenOf(...) > instead of writing out the firstChild and nextSibling calls Sure; this is older code, just moved to this class, but easy enough to change. Thanks, done. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h:34: RawPtrWillBeWeakPersistent<Document> m_parent; On 2016/03/02 12:36:29, sof wrote: > On 2016/03/01 23:29:21, jbroman wrote: > > On 2016/03/01 at 21:55:21, sof wrote: > > > Please move DocumentOriginTrialContext to the Oilpan heap rather than use > > WeakPersistent<> for a back pointer. > > > > Agreed that WeakPersistent is icky. If Document* is invalid (it's not clear to > > me that it should be, if ), then I agree that this should just be a > > GarbageCollected object, and can hold a Member<Document> (or > > WeakMember<Document>, but IIUC Member is cheaper). > > For an example of owner-ownee w/Oilpan that's near identical to this one, see > VisitedLinkState. Done. Moved to heap, replaced this with RawPtrWillBeMember. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:83: return reinterpret_cast<DocumentOriginTrialContext*>(document().originTrialContext())->getTokens(); On 2016/03/01 23:29:21, jbroman wrote: > This should not be a reinterpret_cast. > > If you need to cast it, you should static_cast. But I don't think you need to > cast it at all, since getTokens is defined on the superclass. It was cast because OriginTrialContext::getTokens isn't friendly with this test class, but DocumentOriginTrialContext is. (And there didn't seem to be a need to make getTokens publically accessable). I didn't want to specify every test class as a friend to OriginTrialContext. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:84: } On 2016/03/01 23:29:21, jbroman wrote: > It seems odd for a unit test to be testing the protected member of the class. Is > there a reason this is factored this way? > > I'd have expected the thing you want to test to be public, especially for such a > small class. This suggests to me that either this test should be calling the > public methods that exist now, or OriginTrialContext should be a pure virtual > interface, with the rest of the logic in OriginTrials. > > WDYT? The original argument was that OriginTrials, as generated code, should be as simple as possible; hopefully just wrapper calls to the underlying OriginTrialContext object. We want every consumer to be using the generated code, rather than probing the underlying mechanisms provided by the context object. On the other hand, this is the *unit test* suite for the context object, so it needs to test those things. (The context object's API is available only to OriginTrials, but it's still an API, so we test it) That all applied to the isFeatureEnabled and hasValidToken methods, though -- I can easily live with getTokens() being public. Changed. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:111: EXPECT_EQ(2UL, tokens.size()); On 2016/03/01 23:29:21, jbroman wrote: > You could use gmock to make this a little more concise: > > EXPECT_THAT(getTokens(), UnorderedElementsAre(kGoodTrialToken, > kAnotherTrialToken)); Love it! Thanks! https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:30: // This class is not intended to be instantiated. Any required state is kept On 2016/03/01 23:29:21, jbroman wrote: > Out of date comment? You do seem to instantiate it. Yep, out of date now. Thanks. Fixed. https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/OriginTrialContextTest.cpp:229: EXPECT_EQ(0, tokenValidator()->callCount()); On 2016/03/01 23:29:21, jbroman wrote: > Not added in this CL (so I won't make you change it), but you could have used > gmock to make this mock class much more concisely, and then this would be: > > EXPECT_CALL(tokenValidator(), validateToken(_, _, _)).Times(0); > > Just mentioning it in case you hadn't considered it. Ack. I'll look at refactoring the tests in a different CL.
lgtm (thanks for bearing with me) Please update the CL description (and add a bug number) before landing. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl:12: OriginTrials::OriginTrials(PassOwnPtrWillBeRawPtr<OriginTrialContext> originTrialContext) : m_originTrialContext(originTrialContext) {} nit: long line; consider breaking https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:30: // TODO(iclelland): Remove this static method in favour of calling the Does this TODO still apply? Given the use of a supplement, I actually prefer using the static syntax. Up to you. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ExecutionContext.h (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ExecutionContext.h:37: #include "core/origin_trials/OriginTrialContext.h" nit: ExecutionContext.h is a header included in lots of places; would a forward declaration be enough? (You might need to move the default implementation of createOriginTrialContext to ExecutionContext.cpp for the non-Oilpan build to be happy, but otherwise it should work.) https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:111: Vector<String> tokens = getTokens(); unused variable https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:112: EXPECT_THAT(getTokens(), testing::UnorderedElementsAre(kGoodTrialToken, kAnotherTrialToken)); super-nit: it's fairly ordinary to put "using ::testing::UnorderedElementsAre;" at the top of the file to avoid having to qualify, if you'd prefer that. I don't mind other way. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp:9: #include "core/dom/ExecutionContext.h" I don't think this include is required anymore.
chasej -- can you take one more look at this? Thanks. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/OriginTrials.cpp.tmpl:12: OriginTrials::OriginTrials(PassOwnPtrWillBeRawPtr<OriginTrialContext> originTrialContext) : m_originTrialContext(originTrialContext) {} On 2016/03/04 15:23:48, jbroman wrote: > nit: long line; consider breaking Done. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/templates/OriginTrials.h.tmpl:30: // TODO(iclelland): Remove this static method in favour of calling the On 2016/03/04 15:23:48, jbroman wrote: > Does this TODO still apply? Given the use of a supplement, I actually prefer > using the static syntax. Up to you. I think you're right; the static method is easier to use. I'll update the test API code as well https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ExecutionContext.h (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ExecutionContext.h:37: #include "core/origin_trials/OriginTrialContext.h" On 2016/03/04 15:23:48, jbroman wrote: > nit: ExecutionContext.h is a header included in lots of places; would a forward > declaration be enough? (You might need to move the default implementation of > createOriginTrialContext to ExecutionContext.cpp for the non-Oilpan build to be > happy, but otherwise it should work.) Thanks, done. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:111: Vector<String> tokens = getTokens(); On 2016/03/04 15:23:48, jbroman wrote: > unused variable Removed. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContextTest.cpp:112: EXPECT_THAT(getTokens(), testing::UnorderedElementsAre(kGoodTrialToken, kAnotherTrialToken)); On 2016/03/04 15:23:48, jbroman wrote: > super-nit: it's fairly ordinary to put "using ::testing::UnorderedElementsAre;" > at the top of the file to avoid having to qualify, if you'd prefer that. I don't > mind other way. Cool, thanks. Interestingly, it doesn't seem to be used anywhere in Blink (up until now), but similar functions are. https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp (right): https://codereview.chromium.org/1743623002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/origin_trials/testing/InternalsFrobulate.cpp:9: #include "core/dom/ExecutionContext.h" On 2016/03/04 15:23:48, jbroman wrote: > I don't think this include is required anymore. Done.
lgtm
Description was changed from ========== [Experimental Framework] Make the OriginTrialContext a member of ExecutionContext This patch promotes OriginTrialContext to an actual member of its ExecutionContext, rather than being a collection of static methods. This allows us to maintain state on the context, and allows it to be specialized for the different types of execution contexts, so that document contexts can be handled differently than workers, for instance. BUG= ========== to ========== [Experimental Framework] Make the OriginTrialContext a supplement to ExecutionContext This patch makes OriginTrialContext a supplement to the ExecutionContext object, rather than being a collection of static methods. This allows us to maintain state on the object, and allows it to be specialized for the different types of execution contexts, so that document contexts can be handled differently than workers, for instance. BUG=582063 ==========
The CQ bit was checked by iclelland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/1743623002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1743623002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1743623002/260001
Message was sent while issue was closed.
Description was changed from ========== [Experimental Framework] Make the OriginTrialContext a supplement to ExecutionContext This patch makes OriginTrialContext a supplement to the ExecutionContext object, rather than being a collection of static methods. This allows us to maintain state on the object, and allows it to be specialized for the different types of execution contexts, so that document contexts can be handled differently than workers, for instance. BUG=582063 ========== to ========== [Experimental Framework] Make the OriginTrialContext a supplement to ExecutionContext This patch makes OriginTrialContext a supplement to the ExecutionContext object, rather than being a collection of static methods. This allows us to maintain state on the object, and allows it to be specialized for the different types of execution contexts, so that document contexts can be handled differently than workers, for instance. BUG=582063 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [Experimental Framework] Make the OriginTrialContext a supplement to ExecutionContext This patch makes OriginTrialContext a supplement to the ExecutionContext object, rather than being a collection of static methods. This allows us to maintain state on the object, and allows it to be specialized for the different types of execution contexts, so that document contexts can be handled differently than workers, for instance. BUG=582063 ========== to ========== [Experimental Framework] Make the OriginTrialContext a supplement to ExecutionContext This patch makes OriginTrialContext a supplement to the ExecutionContext object, rather than being a collection of static methods. This allows us to maintain state on the object, and allows it to be specialized for the different types of execution contexts, so that document contexts can be handled differently than workers, for instance. BUG=582063 Committed: https://crrev.com/c970675cb18a6ba91220d95128dced7b3ffd24b2 Cr-Commit-Position: refs/heads/master@{#379575} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c970675cb18a6ba91220d95128dced7b3ffd24b2 Cr-Commit-Position: refs/heads/master@{#379575} |
