Add support for origin trials in workers.

Add support for origin trials in workers.

This adds support for origin trials in workers (dedicated, shared
and service workers). It does so by adding a list of tokens to
WorkerThreadStartupData. That list is filled using the response headers
for shared and service workers, and filled with the documents tokens
for dedicated workers.

BUG=582063
BUG=582195
BUG=582197
Committed: https://crrev.com/de1993ee636b9c8a2ca0f1a817ff3d3ca5c88a6d
Cr-Commit-Position: refs/heads/master@{#392661}

Committed: https://crrev.com/7c46bff409ea163d14af6569b004f7904aa6438e
Cr-Commit-Position: refs/heads/master@{#392778}

[Marijn Kruisselbrink, 2016-03-24 18:45:49]

Description was changed from

==========
WIP: origin trials in shared and service workers

BUG=
==========

to

==========
WIP: origin trials in shared and service workers

BUG=582197
BUG=582195
==========

[Marijn Kruisselbrink, 2016-03-25 21:07:53]

Description was changed from

==========
WIP: origin trials in shared and service workers

BUG=582197
BUG=582195
==========

to

==========
WIP: origin trials in workers

BUG=582063
BUG=582195
BUG=582197
==========

[Marijn Kruisselbrink, 2016-03-25 21:12:45]

Description was changed from

==========
WIP: origin trials in workers

BUG=582063
BUG=582195
BUG=582197
==========

to

==========
Add support for origin trials in workers.

This adds support for origin trials in workers (dedicated, shared
and service workers). It does so by adding a list of tokens to 
WorkerThreadStartupData. That list is filled using the response headers
for shared and service workers, and filled with the documents tokens
for dedicated workers.

BUG=582063
BUG=582195
BUG=582197
==========

[Marijn Kruisselbrink, 2016-03-25 21:21:15]

mek@chromium.org changed reviewers:
+ iclelland@chromium.org

[Marijn Kruisselbrink, 2016-03-25 21:21:16]

PTAL whenever you're back and done with my other review requests :)

https://codereview.chromium.org/1828063002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThreadStartupData.cpp (right):

https://codereview.chromium.org/1828063002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThreadStartupData.cpp:37:
WorkerThreadStartupData::WorkerThreadStartupData(const KURL& scriptURL, const
String& userAgent, const String& sourceCode, PassOwnPtr<Vector<char>>
cachedMetaData, WorkerThreadStartMode startMode, const
PassOwnPtr<Vector<CSPHeaderAndType>> contentSecurityPolicyHeaders, const
SecurityOrigin* starterOrigin, PassOwnPtrWillBeRawPtr<WorkerClients>
workerClients, WebAddressSpace addressSpace, const PassOwnPtr<Vector<String>>
originTrialTokens, V8CacheOptions v8CacheOptions)
Not sure why this is passing the various vectors as PassOwnPtr (and "const
PassOwnPtr<> even), but just did the same for consistency... It seems passing
the vectors that are going to get copied anyway as Vector<> or const Vector<>&
would be nicer... But maybe I'm missing something subtle.

But anyway since this is using pointers, all the other places where these
vectors of tokens are passed around now also use OwnPtr. If it turns out OwnPtr
isn't needed here, I'll also remove it everywhere else where token vectors are
passed around.

[Marijn Kruisselbrink, 2016-04-06 21:17:01]

Patchset #6 (id:100001) has been deleted

[iclelland, 2016-04-13 16:04:15]

This is really exciting -- I'm glad to see it come together!

I don't see a test for a dedicated worker -- just Service Worker and Shared
Workers; did I miss it?

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:79:
PassOwnPtr<Vector<String>> OriginTrialContext::parseHeaderValue(const String&
headerValue)
It's too bad the types aren't compatible; it looks like this could just live in
net::HttpUtil, since it doesn't do anything origin-trials-specific anymore.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:39: static
OriginTrialContext* from(ExecutionContext*, bool create = true);
Should we document the potential for a nullptr return now?

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:115:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace(), OriginTrialContext::getTokens(document));
Can this code be called from a CompositorWorker? I'm not entirely sure what the
relationship between Compositorworkers and documents is, but
CompositorWorkerMessagingProxy also inherits from this class.

If this CL is adding Origin trial support in those, (even only incidentally,)
then I'd want to get someone from that team to review as well. We may want to
disable trials in CW explicitly until we've figured out if there are any
subtleties.

[Marijn Kruisselbrink, 2016-04-14 19:02:56]

On 2016/04/13 at 16:04:15, iclelland wrote:
> I don't see a test for a dedicated worker -- just Service Worker and Shared
Workers; did I miss it?

The fetch_tests_from_worker calls in origin_trials.js add tests for dedicated
workers to all the tests that tested support in regular documents. But I'm not
really happy with these worker tests, since we don't really have a way currently
to expose the testing APIs in workers only in content_shell (and possibly even
only when running layout tests) like we do for regular documents using the
internals thing.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:79:
PassOwnPtr<Vector<String>> OriginTrialContext::parseHeaderValue(const String&
headerValue)
On 2016/04/13 at 16:04:14, iclelland wrote:
> It's too bad the types aren't compatible; it looks like this could just live
in net::HttpUtil, since it doesn't do anything origin-trials-specific anymore.

I don't think net:: owners would agree with having this live in net::HttpUtil
since it isn't really part of core HTTP. But blink platform/network/HttpParsers
could probably have a combined method that does both the splitting and the
unquoting. Not sure how often such a method would be useful though; I guess
having the combined method would avoid some extra string conversions.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:39: static
OriginTrialContext* from(ExecutionContext*, bool create = true);
On 2016/04/13 at 16:04:14, iclelland wrote:
> Should we document the potential for a nullptr return now?

Yeah, good point. Maybe the |create| thing is also an unnecessary optimization
though. After all any OriginTrials::fooEnabled() method call will (currently)
possibly create an empty OriginTrialContext anyway. Or maybe the generated code
should call from with create=false as well.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:115:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace(), OriginTrialContext::getTokens(document));
On 2016/04/13 at 16:04:14, iclelland wrote:
> Can this code be called from a CompositorWorker? I'm not entirely sure what
the relationship between Compositorworkers and documents is, but
CompositorWorkerMessagingProxy also inherits from this class.
> 
> If this CL is adding Origin trial support in those, (even only incidentally,)
then I'd want to get someone from that team to review as well. We may want to
disable trials in CW explicitly until we've figured out if there are any
subtleties.

Good point. Maybe I should move the OriginTrialContext::addTokens call from
WorkerThread::initialize to the various constructors for the various types of
worker global scopes. That would make it explicit which types of workers enable
phosphor. It is also more similar to how things like CSP policies are passed to
workers via WorkerThreadStartupData.

[iclelland, 2016-04-15 19:55:42]

On 2016/04/13 at 16:04:15, iclelland wrote:
> > I don't see a test for a dedicated worker -- just Service Worker and Shared
> > Workers; did I miss it?

> The fetch_tests_from_worker calls in origin_trials.js add tests for dedicated
> workers to all the tests that tested support in regular documents.

Got it -- thanks, I saw that, registered it, and then forgot about it.

> But I'm not
> really happy with these worker tests, since we don't really have a way
currently
> to expose the testing APIs in workers only in content_shell (and possibly even
> only when running layout tests) like we do for regular documents using the
> internals thing.

Yeah, I was looking at that for a little bit, and so was chasej for a while. It
would be good to fix that, but not necessary here.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:79:
PassOwnPtr<Vector<String>> OriginTrialContext::parseHeaderValue(const String&
headerValue)
On 2016/04/14 19:02:56, Marijn Kruisselbrink wrote:
> On 2016/04/13 at 16:04:14, iclelland wrote:
> > It's too bad the types aren't compatible; it looks like this could just live
> in net::HttpUtil, since it doesn't do anything origin-trials-specific anymore.
> 
> I don't think net:: owners would agree with having this live in net::HttpUtil
> since it isn't really part of core HTTP. But blink
platform/network/HttpParsers
> could probably have a combined method that does both the splitting and the
> unquoting. Not sure how often such a method would be useful though; I guess
> having the combined method would avoid some extra string conversions.

One could argue that it implements part of section 8.3.1 of RFC 7231, but I'm
not going to push for that today :) Anyway, this wasn't a change request, just
wishful thinking.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:39: static
OriginTrialContext* from(ExecutionContext*, bool create = true);
On 2016/04/14 19:02:56, Marijn Kruisselbrink wrote:
> On 2016/04/13 at 16:04:14, iclelland wrote:
> > Should we document the potential for a nullptr return now?
> 
> Yeah, good point. Maybe the |create| thing is also an unnecessary optimization
> though. After all any OriginTrials::fooEnabled() method call will (currently)
> possibly create an empty OriginTrialContext anyway. Or maybe the generated
code
> should call from with create=false as well.

But isn't the point of autovivification that we don't create the context until
it's actually used? (Even if it's just to check -- the assumption is that the
vast majority of pages *won't* be testing those methods) What would trigger the
creation in that scenario?

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:115:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace(), OriginTrialContext::getTokens(document));
On 2016/04/14 19:02:56, Marijn Kruisselbrink wrote:
> On 2016/04/13 at 16:04:14, iclelland wrote:
> > Can this code be called from a CompositorWorker? I'm not entirely sure what
> the relationship between Compositorworkers and documents is, but
> CompositorWorkerMessagingProxy also inherits from this class.
> > 
> > If this CL is adding Origin trial support in those, (even only
incidentally,)
> then I'd want to get someone from that team to review as well. We may want to
> disable trials in CW explicitly until we've figured out if there are any
> subtleties.
> 
> Good point. Maybe I should move the OriginTrialContext::addTokens call from
> WorkerThread::initialize to the various constructors for the various types of
> worker global scopes. That would make it explicit which types of workers
enable
> phosphor. It is also more similar to how things like CSP policies are passed
to
> workers via WorkerThreadStartupData.

Explicit is good. I'd support that.

I also talked to vollick@ offline, and he's supportive of adding phosphor to
CompositorWorkers. (Adding him to the CL as an FYI)

[Marijn Kruisselbrink, 2016-04-28 00:12:19]

On 2016/04/15 at 19:55:42, iclelland wrote:
> > But I'm not
> > really happy with these worker tests, since we don't really have a way
currently
> > to expose the testing APIs in workers only in content_shell (and possibly
even
> > only when running layout tests) like we do for regular documents using the
> > internals thing.
> 
> Yeah, I was looking at that for a little bit, and so was chasej for a while.
It would be good to fix that, but not necessary here.

I started trying to do something like a WorkerInternals thing in
https://codereview.chromium.org/1915373002 but it still seems a bit heavy-handed
to add worker internals just for this...

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:39: static
OriginTrialContext* from(ExecutionContext*, bool create = true);
On 2016/04/15 at 19:55:42, iclelland (OOO until 25 April) wrote:
> On 2016/04/14 19:02:56, Marijn Kruisselbrink wrote:
> > On 2016/04/13 at 16:04:14, iclelland wrote:
> > > Should we document the potential for a nullptr return now?
> > 
> > Yeah, good point. Maybe the |create| thing is also an unnecessary
optimization
> > though. After all any OriginTrials::fooEnabled() method call will
(currently)
> > possibly create an empty OriginTrialContext anyway. Or maybe the generated
code
> > should call from with create=false as well.
> 
> But isn't the point of autovivification that we don't create the context until
it's actually used? (Even if it's just to check -- the assumption is that the
vast majority of pages *won't* be testing those methods) What would trigger the
creation in that scenario?

Ah yes, with the current bindings that is of course true. That might potentially
change if bindings change to just not expose origin trial enabled features
without the proper tokens present? No idea how that's going to be implemented,
so maybe I'm wrong there.

Anyway, added a comment explaining the |create| argument and the possibility of
returning null.

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:115:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace(), OriginTrialContext::getTokens(document));
On 2016/04/15 at 19:55:42, iclelland (OOO until 25 April) wrote:
> On 2016/04/14 19:02:56, Marijn Kruisselbrink wrote:
> > On 2016/04/13 at 16:04:14, iclelland wrote:
> > > Can this code be called from a CompositorWorker? I'm not entirely sure
what
> > the relationship between Compositorworkers and documents is, but
> > CompositorWorkerMessagingProxy also inherits from this class.
> > > 
> > > If this CL is adding Origin trial support in those, (even only
incidentally,)
> > then I'd want to get someone from that team to review as well. We may want
to
> > disable trials in CW explicitly until we've figured out if there are any
> > subtleties.
> > 
> > Good point. Maybe I should move the OriginTrialContext::addTokens call from
> > WorkerThread::initialize to the various constructors for the various types
of
> > worker global scopes. That would make it explicit which types of workers
enable
> > phosphor. It is also more similar to how things like CSP policies are passed
to
> > workers via WorkerThreadStartupData.
> 
> Explicit is good. I'd support that.
> 
> I also talked to vollick@ offline, and he's supportive of adding phosphor to
CompositorWorkers. (Adding him to the CL as an FYI)

Okay, moved the code to add tokens to a WorkerGlobalScope to be in the specific
WorkerGlobalScope subclasses, rather than in WorkerThread. So for now that means
only DedicatedWorkerGlobalScope, SharedWorkerGlobalScope and
ServiceWorkerGlobalScope.

[iclelland, 2016-04-28 15:39:22]

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1828063002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:115:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace(), OriginTrialContext::getTokens(document));
On 2016/04/28 00:12:19, Marijn Kruisselbrink wrote:
> On 2016/04/15 at 19:55:42, iclelland (OOO until 25 April) wrote:
> > On 2016/04/14 19:02:56, Marijn Kruisselbrink wrote:
> > > On 2016/04/13 at 16:04:14, iclelland wrote:
> > > > Can this code be called from a CompositorWorker? I'm not entirely sure
> what
> > > the relationship between Compositorworkers and documents is, but
> > > CompositorWorkerMessagingProxy also inherits from this class.
> > > > 
> > > > If this CL is adding Origin trial support in those, (even only
> incidentally,)
> > > then I'd want to get someone from that team to review as well. We may want
> to
> > > disable trials in CW explicitly until we've figured out if there are any
> > > subtleties.
> > > 
> > > Good point. Maybe I should move the OriginTrialContext::addTokens call
from
> > > WorkerThread::initialize to the various constructors for the various types
> of
> > > worker global scopes. That would make it explicit which types of workers
> enable
> > > phosphor. It is also more similar to how things like CSP policies are
passed
> to
> > > workers via WorkerThreadStartupData.
> > 
> > Explicit is good. I'd support that.
> > 
> > I also talked to vollick@ offline, and he's supportive of adding phosphor to
> CompositorWorkers. (Adding him to the CL as an FYI)
> 
> Okay, moved the code to add tokens to a WorkerGlobalScope to be in the
specific
> WorkerGlobalScope subclasses, rather than in WorkerThread. So for now that
means
> only DedicatedWorkerGlobalScope, SharedWorkerGlobalScope and
> ServiceWorkerGlobalScope.

Awesome. We can explicitly deal with CompositorWorker in a separate change, if
and when that comes up.

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadStartupData.h:53: static
PassOwnPtr<WorkerThreadStartupData> create(const KURL& scriptURL, const String&
userAgent, const String& sourceCode, PassOwnPtr<Vector<char>> cachedMetaData,
WorkerThreadStartMode startMode, const Vector<CSPHeaderAndType>*
contentSecurityPolicyHeaders, const SecurityOrigin* starterOrigin,
WorkerClients* workerClients, WebAddressSpace addressSpace, const
Vector<String>* originTrialTokens, V8CacheOptions v8CacheOptions =
V8CacheOptionsDefault)
Why isn't the ownership of originTrialTokens (and contentSecurityPolicyHeaders)
transferred when creating the WorkerThreadStartupData?

It looks like it used to be, for CSP, but isn't now -- it's just a raw pointer
whose contents get explicitly copied, and it's up to the caller to dispose of
the original, and originTrialTokens is the same. Was there a reason for not
keeping the PassOwnPtr, and using std::unique_ptr<Vector<String>> as the
argument type for originTrialTokens here?

[Marijn Kruisselbrink, 2016-04-28 17:45:27]

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadStartupData.h:53: static
PassOwnPtr<WorkerThreadStartupData> create(const KURL& scriptURL, const String&
userAgent, const String& sourceCode, PassOwnPtr<Vector<char>> cachedMetaData,
WorkerThreadStartMode startMode, const Vector<CSPHeaderAndType>*
contentSecurityPolicyHeaders, const SecurityOrigin* starterOrigin,
WorkerClients* workerClients, WebAddressSpace addressSpace, const
Vector<String>* originTrialTokens, V8CacheOptions v8CacheOptions =
V8CacheOptionsDefault)
On 2016/04/28 at 15:39:22, iclelland wrote:
> Why isn't the ownership of originTrialTokens (and
contentSecurityPolicyHeaders) transferred when creating the
WorkerThreadStartupData?
> 
> It looks like it used to be, for CSP, but isn't now -- it's just a raw pointer
whose contents get explicitly copied, and it's up to the caller to dispose of
the original, and originTrialTokens is the same. Was there a reason for not
keeping the PassOwnPtr, and using std::unique_ptr<Vector<String>> as the
argument type for originTrialTokens here?

It actually never transferred ownership for CSP. Until the recent cleanup to not
copy PassOwnPtr instances around this in fact took a "const PassOwnPtr<>" rather
than just a PassOwnPtr, which already made it impossible to actually transfer
ownership. WorkerThreadStartupData has to make copies of anything containing
strings since strings aren't thread-safe in blink. So WorkerThreadStartupData
didn't and doesn't gain ownership of anything containing strings that was passed
on. So since it isn't actually getting ownership of some existing structure it
seemed kind of pointless (and weird) to require the caller to give up ownership.
Using raw pointers more accurately reflects the semantics.

[iclelland, 2016-04-28 18:02:52]

Thanks -- this LGTM

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThreadStartupData.h:53: static
PassOwnPtr<WorkerThreadStartupData> create(const KURL& scriptURL, const String&
userAgent, const String& sourceCode, PassOwnPtr<Vector<char>> cachedMetaData,
WorkerThreadStartMode startMode, const Vector<CSPHeaderAndType>*
contentSecurityPolicyHeaders, const SecurityOrigin* starterOrigin,
WorkerClients* workerClients, WebAddressSpace addressSpace, const
Vector<String>* originTrialTokens, V8CacheOptions v8CacheOptions =
V8CacheOptionsDefault)
On 2016/04/28 17:45:27, Marijn Kruisselbrink wrote:
> On 2016/04/28 at 15:39:22, iclelland wrote:
> > Why isn't the ownership of originTrialTokens (and
> contentSecurityPolicyHeaders) transferred when creating the
> WorkerThreadStartupData?
> > 
> > It looks like it used to be, for CSP, but isn't now -- it's just a raw
pointer
> whose contents get explicitly copied, and it's up to the caller to dispose of
> the original, and originTrialTokens is the same. Was there a reason for not
> keeping the PassOwnPtr, and using std::unique_ptr<Vector<String>> as the
> argument type for originTrialTokens here?
> 
> It actually never transferred ownership for CSP. Until the recent cleanup to
not
> copy PassOwnPtr instances around this in fact took a "const PassOwnPtr<>"
rather
> than just a PassOwnPtr, which already made it impossible to actually transfer
> ownership. WorkerThreadStartupData has to make copies of anything containing
> strings since strings aren't thread-safe in blink. So WorkerThreadStartupData
> didn't and doesn't gain ownership of anything containing strings that was
passed
> on. So since it isn't actually getting ownership of some existing structure it
> seemed kind of pointless (and weird) to require the caller to give up
ownership.
> Using raw pointers more accurately reflects the semantics.

Got it -- I missed the const ref, but that makes sense now. Thanks.

[haraken, 2016-04-28 19:29:29]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-04-28 19:29:30]

bindings LGTM

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerScriptLoader.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerScriptLoader.h:129:
std::unique_ptr<Vector<String>> m_originTrialTokens;

Per yutak's PSA
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/eCDdVh-VsM4/Fe-T-QXB...),
it looks better to use OwnPtr for now (at least we should avoid replacing
existing OwnPtrs with unique_ptrs).

[Marijn Kruisselbrink, 2016-04-28 21:54:23]

mek@chromium.org changed reviewers:
+ kinuko@chromium.org

[Marijn Kruisselbrink, 2016-04-28 21:54:24]

+kinuko for core/ OWNERS.

And I'm still not happy nor sure what to do about testing. Adding testing-only
API to WorkerGlobalScope like this CL is doing seems rather wrong, especially as
long as the origin trials bindings don't actually completely hide code with the
trial disabled. But introducing a WorkerInternals like thing just for this
doesn't seem like the answer either. I could maybe test some of this with
unit-tests, but without also having some end-to-end tests it seems like that
would still make it way too easy for bugs to remain that would make all this not
work.

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerScriptLoader.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerScriptLoader.h:129:
std::unique_ptr<Vector<String>> m_originTrialTokens;
On 2016/04/28 at 19:29:30, haraken wrote:
> Per yutak's PSA
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/eCDdVh-VsM4/Fe-T-QXB...),
it looks better to use OwnPtr for now (at least we should avoid replacing
existing OwnPtrs with unique_ptrs).

I'm a bit confused if I should or shouldn't be using unique_ptr for new code,
since I thought the previous PSA was that new code could/should do so. And not
using unique_ptr here would be somewhat inconvenient since it is assigned from a
method I added in a previous CL that already returns a unique_ptr.

[haraken, 2016-04-29 06:46:32]

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerScriptLoader.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerScriptLoader.h:129:
std::unique_ptr<Vector<String>> m_originTrialTokens;
On 2016/04/28 21:54:24, Marijn Kruisselbrink wrote:
> On 2016/04/28 at 19:29:30, haraken wrote:
> > Per yutak's PSA
>
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/eCDdVh-VsM4/Fe-T-QXB...),
> it looks better to use OwnPtr for now (at least we should avoid replacing
> existing OwnPtrs with unique_ptrs).
> 
> I'm a bit confused if I should or shouldn't be using unique_ptr for new code,
> since I thought the previous PSA was that new code could/should do so. And not
> using unique_ptr here would be somewhat inconvenient since it is assigned from
a
> method I added in a previous CL that already returns a unique_ptr.

I looked at the emails and it looks like he's saying that 1) new code should use
unique_ptr but 2) existing code should not replace OwnPtr with unique_ptr yet.

So this CL looks fine. Sorry about the noise.

[kinuko, 2016-04-29 09:43:05]

lgtm

(assuming using unique_ptr in new files is ok)

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/core.gypi (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/core.gypi:444:
'origin_trials/testing/WorkerGlobalScopeFrobulate.idl',
No way to link this only for testing binary at gyp/gn level?

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:139:
OriginTrialContext* context = from(host, false);
nit: add comment to note this doesn't create a new OriginTrialContext (or use
enum to make intention more explicit in the code)

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:44: static
OriginTrialContext* from(ExecutionContext*, bool create = true);
nit: prefer enum over bool in general

[Marijn Kruisselbrink, 2016-05-02 23:19:30]

mek@chromium.org changed reviewers:
+ rbyers@chromium.org

[Marijn Kruisselbrink, 2016-05-02 23:19:31]

+rbyers as OWNER for the unfortunate stable webexposed layouttest changes... And
maybe you have other suggestions on how to improve the way this is tested and/or
if it makes sense to add some kind of "internals" equivalent to workers.

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/core.gypi (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/core.gypi:444:
'origin_trials/testing/WorkerGlobalScopeFrobulate.idl',
On 2016/04/29 at 09:43:05, kinuko wrote:
> No way to link this only for testing binary at gyp/gn level?

Well, that's pretty much my question about adding something like a
WorkerInternals interface that is linked only in testing similar to the existing
Internals interface for documents. Any other way to only have this available for
testing would probably require about the same amount of plumbing.
https://codereview.chromium.org/1915373002 is my attempt at doing so, but it
still seems like a lot of plumbing etc for something that might not be terribly
useful for anything else. But maybe there are many other things I'm not aware of
that would be greatly simplified by the presence of a self.internals available
in workers.

So any suggestions on how to get the right balance between being able to test
this and not needlessly exposing test-only code to the web would be appreciated.

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp
(right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.cpp:139:
OriginTrialContext* context = from(host, false);
On 2016/04/29 at 09:43:05, kinuko wrote:
> nit: add comment to note this doesn't create a new OriginTrialContext (or use
enum to make intention more explicit in the code)

Changed it to enum.

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h (right):

https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/origin_trials/OriginTrialContext.h:44: static
OriginTrialContext* from(ExecutionContext*, bool create = true);
On 2016/04/29 at 09:43:05, kinuko wrote:
> nit: prefer enum over bool in general

Changed it to an enum.

[chasej, 2016-05-03 02:55:32]

On 2016/05/02 23:19:31, Marijn Kruisselbrink wrote:
> +rbyers as OWNER for the unfortunate stable webexposed layouttest changes...
And
> maybe you have other suggestions on how to improve the way this is tested
and/or
> if it makes sense to add some kind of "internals" equivalent to workers.
> 
>
https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/core.gypi (right):
> 
>
https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/core.gypi:444:
> 'origin_trials/testing/WorkerGlobalScopeFrobulate.idl',
> On 2016/04/29 at 09:43:05, kinuko wrote:
> > No way to link this only for testing binary at gyp/gn level?
> 
> Well, that's pretty much my question about adding something like a
> WorkerInternals interface that is linked only in testing similar to the
existing
> Internals interface for documents. Any other way to only have this available
for
> testing would probably require about the same amount of plumbing.
> https://codereview.chromium.org/1915373002 is my attempt at doing so, but it
> still seems like a lot of plumbing etc for something that might not be
terribly
> useful for anything else. But maybe there are many other things I'm not aware
of
> that would be greatly simplified by the presence of a self.internals available
> in workers.
> 
> So any suggestions on how to get the right balance between being able to test
> this and not needlessly exposing test-only code to the web would be
appreciated.
> 

There was some previous discussion about this problem. I created
crbug.com/570335 to address it, and capture the context. Probably best to
discuss on that issue?

[Rick Byers, 2016-05-03 16:09:41]

On 2016/05/03 02:55:32, chasej wrote:
> On 2016/05/02 23:19:31, Marijn Kruisselbrink wrote:
> > +rbyers as OWNER for the unfortunate stable webexposed layouttest changes...
> And
> > maybe you have other suggestions on how to improve the way this is tested
> and/or
> > if it makes sense to add some kind of "internals" equivalent to workers.
> > 
> >
>
https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/core.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1828063002/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/core.gypi:444:
> > 'origin_trials/testing/WorkerGlobalScopeFrobulate.idl',
> > On 2016/04/29 at 09:43:05, kinuko wrote:
> > > No way to link this only for testing binary at gyp/gn level?
> > 
> > Well, that's pretty much my question about adding something like a
> > WorkerInternals interface that is linked only in testing similar to the
> existing
> > Internals interface for documents. Any other way to only have this available
> for
> > testing would probably require about the same amount of plumbing.
> > https://codereview.chromium.org/1915373002 is my attempt at doing so, but it
> > still seems like a lot of plumbing etc for something that might not be
> terribly
> > useful for anything else. But maybe there are many other things I'm not
aware
> of
> > that would be greatly simplified by the presence of a self.internals
available
> > in workers.
> > 
> > So any suggestions on how to get the right balance between being able to
test
> > this and not needlessly exposing test-only code to the web would be
> appreciated.
> > 
> 
> There was some previous discussion about this problem. I created
> crbug.com/570335 to address it, and capture the context. Probably best to
> discuss on that issue?

I don't think we can expose a 'frobulate' property on WorkerGlobalScope in
shipping Chrome.  There's some risk that this name could collide with something
some app somewhere is doing ("window.frobulate=7" etc), and then we'd look
pretty silly :-).  If we made the name long and obviously internal the risk
would go down.  But still polluting the web-exposed API for our own internal
testing purposes is definitely not "the right thing to do".

This particular issue is temporary, and will be resolved by the bindings work
being done to fully hide disabled origin-trials, right?  In that case some
temporary hack is probably acceptable.  Eg. use an uglier / more obviously bogus
name combined with some sort of build hack (#ifdef if necessary) to ensure it's
present only in content_shell and not real chrome.  But that's still short-term
and ugly.

The bigger issue is that we totally need some form of "internals" for workers,
beyond just this use case.  IMHO it seems worth the effort to figure out how to
do that properly.  I don't know the details but I can't imagine it's THAT hard
to re-use the existing logic that injects the 'internals' property and have it
apply to workers also (but giving it a different object than the one given to
WindowGlobalScope).  Let's discuss further on http://crbug.com/570335 ?

[Marijn Kruisselbrink, 2016-05-03 19:02:02]

On 2016/05/03 at 16:09:41, rbyers wrote:
> On 2016/05/03 02:55:32, chasej wrote:
> > There was some previous discussion about this problem. I created
> > crbug.com/570335 to address it, and capture the context. Probably best to
> > discuss on that issue?

Ah, thanks, I somehow missed that issue earlier. That's definitely the best
place to discuss this.

> I don't think we can expose a 'frobulate' property on WorkerGlobalScope in
shipping Chrome.  There's some risk that this name could collide with something
some app somewhere is doing ("window.frobulate=7" etc), and then we'd look
pretty silly :-).  If we made the name long and obviously internal the risk
would go down.  But still polluting the web-exposed API for our own internal
testing purposes is definitely not "the right thing to do".
> 
> This particular issue is temporary, and will be resolved by the bindings work
being done to fully hide disabled origin-trials, right?  In that case some
temporary hack is probably acceptable.  Eg. use an uglier / more obviously bogus
name combined with some sort of build hack (#ifdef if necessary) to ensure it's
present only in content_shell and not real chrome.  But that's still short-term
and ugly.
> 
> The bigger issue is that we totally need some form of "internals" for workers,
beyond just this use case.  IMHO it seems worth the effort to figure out how to
do that properly.  I don't know the details but I can't imagine it's THAT hard
to re-use the existing logic that injects the 'internals' property and have it
apply to workers also (but giving it a different object than the one given to
WindowGlobalScope).  Let's discuss further on http://crbug.com/570335 ?

For now I just cleaned up my WorkerInternals patch, and rebased this patch on
top of that one. I think blocking this on figuring out the correct way to do
worker internals is fine, since as far as I know nobody is actually blocked yet
on having origin trials work in workers.

[Marijn Kruisselbrink, 2016-05-03 19:56:37]

On 2016/05/03 at 16:09:41, rbyers wrote:
> On 2016/05/03 02:55:32, chasej wrote:
> > There was some previous discussion about this problem. I created
> > crbug.com/570335 to address it, and capture the context. Probably best to
> > discuss on that issue?

Ah, thanks, I somehow missed that issue earlier. That's definitely the best
place to discuss this.

> I don't think we can expose a 'frobulate' property on WorkerGlobalScope in
shipping Chrome.  There's some risk that this name could collide with something
some app somewhere is doing ("window.frobulate=7" etc), and then we'd look
pretty silly :-).  If we made the name long and obviously internal the risk
would go down.  But still polluting the web-exposed API for our own internal
testing purposes is definitely not "the right thing to do".
> 
> This particular issue is temporary, and will be resolved by the bindings work
being done to fully hide disabled origin-trials, right?  In that case some
temporary hack is probably acceptable.  Eg. use an uglier / more obviously bogus
name combined with some sort of build hack (#ifdef if necessary) to ensure it's
present only in content_shell and not real chrome.  But that's still short-term
and ugly.
> 
> The bigger issue is that we totally need some form of "internals" for workers,
beyond just this use case.  IMHO it seems worth the effort to figure out how to
do that properly.  I don't know the details but I can't imagine it's THAT hard
to re-use the existing logic that injects the 'internals' property and have it
apply to workers also (but giving it a different object than the one given to
WindowGlobalScope).  Let's discuss further on http://crbug.com/570335 ?

For now I just cleaned up my WorkerInternals patch, and rebased this patch on
top of that one. I think blocking this on figuring out the correct way to do
worker internals is fine, since as far as I know nobody is actually blocked yet
on having origin trials work in workers.

[Rick Byers, 2016-05-04 13:53:06]

On 2016/05/03 19:56:37, Marijn Kruisselbrink wrote:
> On 2016/05/03 at 16:09:41, rbyers wrote:
> > On 2016/05/03 02:55:32, chasej wrote:
> > > There was some previous discussion about this problem. I created
> > > crbug.com/570335 to address it, and capture the context. Probably best to
> > > discuss on that issue?
> 
> Ah, thanks, I somehow missed that issue earlier. That's definitely the best
> place to discuss this.
> 
> > I don't think we can expose a 'frobulate' property on WorkerGlobalScope in
> shipping Chrome.  There's some risk that this name could collide with
something
> some app somewhere is doing ("window.frobulate=7" etc), and then we'd look
> pretty silly :-).  If we made the name long and obviously internal the risk
> would go down.  But still polluting the web-exposed API for our own internal
> testing purposes is definitely not "the right thing to do".
> > 
> > This particular issue is temporary, and will be resolved by the bindings
work
> being done to fully hide disabled origin-trials, right?  In that case some
> temporary hack is probably acceptable.  Eg. use an uglier / more obviously
bogus
> name combined with some sort of build hack (#ifdef if necessary) to ensure
it's
> present only in content_shell and not real chrome.  But that's still
short-term
> and ugly.
> > 
> > The bigger issue is that we totally need some form of "internals" for
workers,
> beyond just this use case.  IMHO it seems worth the effort to figure out how
to
> do that properly.  I don't know the details but I can't imagine it's THAT hard
> to re-use the existing logic that injects the 'internals' property and have it
> apply to workers also (but giving it a different object than the one given to
> WindowGlobalScope).  Let's discuss further on http://crbug.com/570335 ?
> 
> For now I just cleaned up my WorkerInternals patch, and rebased this patch on
> top of that one. I think blocking this on figuring out the correct way to do
> worker internals is fine, since as far as I know nobody is actually blocked
yet
> on having origin trials work in workers.

Thanks, this patch looks great to me!

[Marijn Kruisselbrink, 2016-05-10 17:03:30]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-05-10 17:03:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kinuko@chromium.org, iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/1828063002/#ps200001
(title: "rebase on top of WorkerInternals change")

[commit-bot: I haz the power, 2016-05-10 17:04:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828063002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828063002/200001

[commit-bot: I haz the power, 2016-05-10 18:35:44]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-05-10 18:37:22]

Description was changed from

==========
Add support for origin trials in workers.

This adds support for origin trials in workers (dedicated, shared
and service workers). It does so by adding a list of tokens to 
WorkerThreadStartupData. That list is filled using the response headers
for shared and service workers, and filled with the documents tokens
for dedicated workers.

BUG=582063
BUG=582195
BUG=582197
==========

to

==========
Add support for origin trials in workers.

This adds support for origin trials in workers (dedicated, shared
and service workers). It does so by adding a list of tokens to
WorkerThreadStartupData. That list is filled using the response headers
for shared and service workers, and filled with the documents tokens
for dedicated workers.

BUG=582063
BUG=582195
BUG=582197

Committed: https://crrev.com/de1993ee636b9c8a2ca0f1a817ff3d3ca5c88a6d
Cr-Commit-Position: refs/heads/master@{#392661}
==========

[commit-bot: I haz the power, 2016-05-10 18:37:23]

Patchset 10 (id:??) landed as
https://crrev.com/de1993ee636b9c8a2ca0f1a817ff3d3ca5c88a6d
Cr-Commit-Position: refs/heads/master@{#392661}

[Marijn Kruisselbrink, 2016-05-10 22:07:26]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/1969583002/ by mek@chromium.org.

The reason for reverting is: http/tests/origin_trials/sample-api-workers.html
seems to randomly timeout sometimes, not sure yet if it's just slow or if
something is actually flaky/wrong.

[Marijn Kruisselbrink, 2016-05-10 23:27:11]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-05-10 23:27:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kinuko@chromium.org, iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/1828063002/#ps220001
(title: "fix test flakyness")

[commit-bot: I haz the power, 2016-05-10 23:27:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1828063002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1828063002/220001

[commit-bot: I haz the power, 2016-05-11 00:08:47]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-05-11 00:11:18]

Description was changed from

==========
Add support for origin trials in workers.

This adds support for origin trials in workers (dedicated, shared
and service workers). It does so by adding a list of tokens to
WorkerThreadStartupData. That list is filled using the response headers
for shared and service workers, and filled with the documents tokens
for dedicated workers.

BUG=582063
BUG=582195
BUG=582197

Committed: https://crrev.com/de1993ee636b9c8a2ca0f1a817ff3d3ca5c88a6d
Cr-Commit-Position: refs/heads/master@{#392661}
==========

to

==========
Add support for origin trials in workers.

This adds support for origin trials in workers (dedicated, shared
and service workers). It does so by adding a list of tokens to
WorkerThreadStartupData. That list is filled using the response headers
for shared and service workers, and filled with the documents tokens
for dedicated workers.

BUG=582063
BUG=582195
BUG=582197

Committed: https://crrev.com/de1993ee636b9c8a2ca0f1a817ff3d3ca5c88a6d
Cr-Commit-Position: refs/heads/master@{#392661}

Committed: https://crrev.com/7c46bff409ea163d14af6569b004f7904aa6438e
Cr-Commit-Position: refs/heads/master@{#392778}
==========

[commit-bot: I haz the power, 2016-05-11 00:11:20]

Patchset 11 (id:??) landed as
https://crrev.com/7c46bff409ea163d14af6569b004f7904aa6438e
Cr-Commit-Position: refs/heads/master@{#392778}