Make Foreign Fetch an origin trial.

Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

BUG=540509, 582310, 590873
Committed: https://crrev.com/33bf998f2e0bc85a90016744f77c5a01d448cefe
Cr-Commit-Position: refs/heads/master@{#411759}

[Marijn Kruisselbrink, 2016-07-01 23:44:53]

Patchset #4 (id:60001) has been deleted

[Marijn Kruisselbrink, 2016-07-01 23:52:16]

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

[Marijn Kruisselbrink, 2016-07-01 23:52:17]

This is my initial attempt at exposing foreign fetch behind an origin trial; a
couple questions:

- any idea what I'm doing wrong with the HTMLLinkElement scope attribute? The
webexposed layout tests are failing because the attribute somehow ended up on
HTMLElement instead.

- does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make sense?
Does that actually give me the remote kill switch I want (disabling both
bindings and browser side logic)?

- any ideas about testing? By adding the .htaccess files to inject origin trial
tokens I can at least manually test things with `content_shell --run-layout-test
--stable-release-mode`. I don't think I actually want to land those .htacess
files though, and maybe having automated tests to make sure features behind
origin trials work when tokens are provided also isn't actually necesary.

[Marijn Kruisselbrink, 2016-07-01 23:56:14]

On 2016/07/01 at 23:52:17, Marijn Kruisselbrink wrote:
> This is my initial attempt at exposing foreign fetch behind an origin trial; a
couple questions:
> 
> - any idea what I'm doing wrong with the HTMLLinkElement scope attribute? The
webexposed layout tests are failing because the attribute somehow ended up on
HTMLElement instead.
> 
> - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make sense?
Does that actually give me the remote kill switch I want (disabling both
bindings and browser side logic)?
> 
> - any ideas about testing? By adding the .htaccess files to inject origin
trial tokens I can at least manually test things with `content_shell
--run-layout-test --stable-release-mode`. I don't think I actually want to land
those .htacess files though, and maybe having automated tests to make sure
features behind origin trials work when tokens are provided also isn't actually
necesary.

oh, one more question: currently blink code loops over all tokens and calls
WebTrialTokenValidator::validateToken for each one. I wonder if it might make
sense to instead have the blink code just pass all tokens it knows about in one
call to a WebTrialTokenValidator::valideTokens method? That way all the logic
about validation result priorities etc could live in content as well, making it
easier to share that logic with something like the
TrialTokenValidator::RequestEnablesFeature method I added here.

[falken, 2016-07-04 03:53:17]

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

[falken, 2016-07-04 03:53:18]

Just to clarify the origin trial. If a page on a.com makes a request to b.com,
then only b.com needs to have foreign fetch enabled for foreign fetch to happen?

[iclelland, 2016-07-05 13:47:36]

On 2016/07/01 23:56:14, Marijn Kruisselbrink wrote:
> On 2016/07/01 at 23:52:17, Marijn Kruisselbrink wrote:
> > This is my initial attempt at exposing foreign fetch behind an origin trial;
a
> couple questions:
> > 
> > - any idea what I'm doing wrong with the HTMLLinkElement scope attribute?
The
> webexposed layout tests are failing because the attribute somehow ended up on
> HTMLElement instead.

This is happening because the generated installForeignFetch method takes an
object instance, but is being passed the interface object. Since scope is
supposed to be installed OnPrototype, it looks up the prototype chain for
HTMLLinkElement and finds HTMLElement. That's my fault; I originally wrote those
install* methods to take instances like 'navigator', 'window', or 'internals';
they're not designed to take the prototype or interface directly. I think I can
fix that easily, if you can wait for a patch to rebase against.

> > 
> > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make
sense?
> Does that actually give me the remote kill switch I want (disabling both
> bindings and browser side logic)?

As long as StoragePartitionImplMap::Get is being called late enough the the
OriginTrialPolicy (if present at all) will be available, then that part makes
sense. Is kEnableExperimentalWebPlatformFeatures really the finest-grained
command-line switch that we can use for this, though? I expected there to be a
base::Feature for Foreign Fetch, that would have its own command-line switch,
like WebUSB, but there may be reasons that I'm not seeing why that wouldn't
work. 

> > 
> > - any ideas about testing? By adding the .htaccess files to inject origin
> trial tokens I can at least manually test things with `content_shell
> --run-layout-test --stable-release-mode`. I don't think I actually want to
land
> those .htacess files though, and maybe having automated tests to make sure
> features behind origin trials work when tokens are provided also isn't
actually
> necesary.

1 - Can you use the PHP module of the Apache server to add the headers that you
need?
2 - You're probably right, we have layout tests that check whether the origin
trials system is working at all, with different combinations of good, bad and
missing tokens; You can probably get away with enabling features through
individual flags, or through experimental-web-platform-features and testing in
that configuration.

> 
> oh, one more question: currently blink code loops over all tokens and calls
> WebTrialTokenValidator::validateToken for each one. I wonder if it might make
> sense to instead have the blink code just pass all tokens it knows about in
one
> call to a WebTrialTokenValidator::valideTokens method? That way all the logic
> about validation result priorities etc could live in content as well, making
it
> easier to share that logic with something like the
> TrialTokenValidator::RequestEnablesFeature method I added here.

That's a good idea; I know that it's not very efficient, the way we're doing it
right now -- especially since we loop over the tokens on every
<feature>Enabled() call, until we find one.

[Marijn Kruisselbrink, 2016-07-06 22:52:01]

On 2016/07/04 at 03:53:18, falken wrote:
> Just to clarify the origin trial. If a page on a.com makes a request to b.com,
then only b.com needs to have foreign fetch enabled for foreign fetch to happen?

That's what this patch implements at least, yes. There's good arguments for both
sides (requiring a.com to opt-in kind eliminates some use cases foreign fetch
might be useful for, but on the other hand not requiring a.com to opt-in makes
it a bit weird that experimental features end up effecting a.com). In the end I
think we settled on only requiring b.com to opt-in, but we'll need good UMA
stats (I haven't quite figured out what those would be though) and a kill switch
to be able to turn things off easily if things go wrong.

On 2016/07/05 at 13:47:36, iclelland wrote:
> On 2016/07/01 23:56:14, Marijn Kruisselbrink wrote:
> > On 2016/07/01 at 23:52:17, Marijn Kruisselbrink wrote:
> > > This is my initial attempt at exposing foreign fetch behind an origin
trial; a
> > couple questions:
> > > 
> > > - any idea what I'm doing wrong with the HTMLLinkElement scope attribute?
The
> > webexposed layout tests are failing because the attribute somehow ended up
on
> > HTMLElement instead.
> 
> This is happening because the generated installForeignFetch method takes an
object instance, but is being passed the interface object. Since scope is
supposed to be installed OnPrototype, it looks up the prototype chain for
HTMLLinkElement and finds HTMLElement. That's my fault; I originally wrote those
install* methods to take instances like 'navigator', 'window', or 'internals';
they're not designed to take the prototype or interface directly. I think I can
fix that easily, if you can wait for a patch to rebase against.

Ah yeah, of course. It definitely seems unfortunate to require having an object
instance to be able to install origin trial code. And even looking up the global
"HTMLLinkElement" like this patch is currently doing and installing the origin
trial on that seems like it wouldn't always work (interfaces with
[NoInterfaceObject] aren't actually exposed anywhere in the javascript world).
Waiting is no problem.

> > > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make
sense?
> > Does that actually give me the remote kill switch I want (disabling both
> > bindings and browser side logic)?
> 
> As long as StoragePartitionImplMap::Get is being called late enough the the
OriginTrialPolicy (if present at all) will be available, then that part makes
sense. Is kEnableExperimentalWebPlatformFeatures really the finest-grained
command-line switch that we can use for this, though? I expected there to be a
base::Feature for Foreign Fetch, that would have its own command-line switch,
like WebUSB, but there may be reasons that I'm not seeing why that wouldn't
work. 

There wasn't really any reason to have a separate feature/command-line switch
for foreign fetch until now. Maybe it makes sense to add one for the purpose of
the origin trial though. I'm not sure how that would integrate with the origin
trial policy though? I.e. how do we keep the feature being en/disabled in sync
with the origin trial being en/disabled? I don't really want to have to deal
with weird edge cases were the trial is enabled but the feature isn't, or the
other way around. So it seems that whatever way the kill switch for foreign
fetch (or any other origin trial feature) works, that kill switch should kill
both the origin trial and other parts that need to be disabled.

> > > - any ideas about testing? By adding the .htaccess files to inject origin
> > trial tokens I can at least manually test things with `content_shell
> > --run-layout-test --stable-release-mode`. I don't think I actually want to
land
> > those .htacess files though, and maybe having automated tests to make sure
> > features behind origin trials work when tokens are provided also isn't
actually
> > necesary.
> 
> 1 - Can you use the PHP module of the Apache server to add the headers that
you need?

Not sure how that's any better? This way I at least don't have to modify any of
my tests, with these .htaccess files in place I can locally run whichever test I
want and make sure it passes when I expect it to.

> 2 - You're probably right, we have layout tests that check whether the origin
trials system is working at all, with different combinations of good, bad and
missing tokens; You can probably get away with enabling features through
individual flags, or through experimental-web-platform-features and testing in
that configuration.

Certainly once the origin trial framework is a bit more robust (not having to
replace RuntimeEnabled with OriginTrialEnabled all over the place, not having to
write manual code in V8Bindings, etc) it seems fine to not explicitly test a
specific feature being enabled by an origin trial, as you'd essentially be
testing auto-generated code (certainly for simple features). I'm just not
entirely sure how we make sure that until we reach that point (and any
refactorings that are done to reach that point) nothing breaks. But I'm fine
with "manually test to make sure origin trials still work" being the answer.

> > 
> > oh, one more question: currently blink code loops over all tokens and calls
> > WebTrialTokenValidator::validateToken for each one. I wonder if it might
make
> > sense to instead have the blink code just pass all tokens it knows about in
one
> > call to a WebTrialTokenValidator::valideTokens method? That way all the
logic
> > about validation result priorities etc could live in content as well, making
it
> > easier to share that logic with something like the
> > TrialTokenValidator::RequestEnablesFeature method I added here.
> 
> That's a good idea; I know that it's not very efficient, the way we're doing
it right now -- especially since we loop over the tokens on every
<feature>Enabled() call, until we find one.

Okay, I'll try to do something like that. Not sure if it really would change
anything with efficiency though. It'll mostly be moving code around. For
efficiency it seems like something like OriginTrialContext::isFeatureEnabled
caching the result would be more helpful (although that method really shouldn't
be called that much for any particular execution context anyway, so not sure
there will be much to gain there).

[falken, 2016-07-07 00:21:45]

On 2016/07/06 22:52:01, Marijn Kruisselbrink wrote:
> On 2016/07/04 at 03:53:18, falken wrote:
> > Just to clarify the origin trial. If a page on a.com makes a request to
b.com,
> then only b.com needs to have foreign fetch enabled for foreign fetch to
happen?
> 
> That's what this patch implements at least, yes. There's good arguments for
both
> sides (requiring a.com to opt-in kind eliminates some use cases foreign fetch
> might be useful for, but on the other hand not requiring a.com to opt-in makes
> it a bit weird that experimental features end up effecting a.com). In the end
I
> think we settled on only requiring b.com to opt-in, but we'll need good UMA
> stats (I haven't quite figured out what those would be though) and a kill
switch
> to be able to turn things off easily if things go wrong.

Makes sense to me. It'd be good to add a comment about this in the CL
description before committing.

[iclelland, 2016-07-08 19:35:47]

On 2016/07/06 22:52:01, Marijn Kruisselbrink wrote:
> On 2016/07/04 at 03:53:18, falken wrote:
> > Just to clarify the origin trial. If a page on a.com makes a request to
b.com,
> then only b.com needs to have foreign fetch enabled for foreign fetch to
happen?
> 
> That's what this patch implements at least, yes. There's good arguments for
both
> sides (requiring a.com to opt-in kind eliminates some use cases foreign fetch
> might be useful for, but on the other hand not requiring a.com to opt-in makes
> it a bit weird that experimental features end up effecting a.com). In the end
I
> think we settled on only requiring b.com to opt-in, but we'll need good UMA
> stats (I haven't quite figured out what those would be though) and a kill
switch
> to be able to turn things off easily if things go wrong.
> 
> On 2016/07/05 at 13:47:36, iclelland wrote:
> > On 2016/07/01 23:56:14, Marijn Kruisselbrink wrote:
> > > On 2016/07/01 at 23:52:17, Marijn Kruisselbrink wrote:
> > > > This is my initial attempt at exposing foreign fetch behind an origin
> trial; a
> > > couple questions:
> > > > 
> > > > - any idea what I'm doing wrong with the HTMLLinkElement scope
attribute?
> The
> > > webexposed layout tests are failing because the attribute somehow ended up
> on
> > > HTMLElement instead.
> > 
> > This is happening because the generated installForeignFetch method takes an
> object instance, but is being passed the interface object. Since scope is
> supposed to be installed OnPrototype, it looks up the prototype chain for
> HTMLLinkElement and finds HTMLElement. That's my fault; I originally wrote
those
> install* methods to take instances like 'navigator', 'window', or 'internals';
> they're not designed to take the prototype or interface directly. I think I
can
> fix that easily, if you can wait for a patch to rebase against.
> 
> Ah yeah, of course. It definitely seems unfortunate to require having an
object
> instance to be able to install origin trial code. And even looking up the
global
> "HTMLLinkElement" like this patch is currently doing and installing the origin
> trial on that seems like it wouldn't always work (interfaces with
> [NoInterfaceObject] aren't actually exposed anywhere in the javascript world).
> Waiting is no problem.

You can do this now, as of 4d48d54 -- if you don't declare any members in IDL
which need to be installed OnInstance, then we don't require an instance object
in the generated methods.

> 
> > > > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make
> sense?
> > > Does that actually give me the remote kill switch I want (disabling both
> > > bindings and browser side logic)?
> > 
> > As long as StoragePartitionImplMap::Get is being called late enough the the
> OriginTrialPolicy (if present at all) will be available, then that part makes
> sense. Is kEnableExperimentalWebPlatformFeatures really the finest-grained
> command-line switch that we can use for this, though? I expected there to be a
> base::Feature for Foreign Fetch, that would have its own command-line switch,
> like WebUSB, but there may be reasons that I'm not seeing why that wouldn't
> work. 
> 
> There wasn't really any reason to have a separate feature/command-line switch
> for foreign fetch until now. Maybe it makes sense to add one for the purpose
of
> the origin trial though. I'm not sure how that would integrate with the origin
> trial policy though? I.e. how do we keep the feature being en/disabled in sync
> with the origin trial being en/disabled? I don't really want to have to deal
> with weird edge cases were the trial is enabled but the feature isn't, or the
> other way around. So it seems that whatever way the kill switch for foreign
> fetch (or any other origin trial feature) works, that kill switch should kill
> both the origin trial and other parts that need to be disabled.

Right -- the kill switch for foreign fetch would kill the origin trial, and
everything else.

> 
> > > > - any ideas about testing? By adding the .htaccess files to inject
origin
> > > trial tokens I can at least manually test things with `content_shell
> > > --run-layout-test --stable-release-mode`. I don't think I actually want to
> land
> > > those .htacess files though, and maybe having automated tests to make sure
> > > features behind origin trials work when tokens are provided also isn't
> actually
> > > necesary.
> > 
> > 1 - Can you use the PHP module of the Apache server to add the headers that
> you need?
> 
> Not sure how that's any better? This way I at least don't have to modify any
of
> my tests, with these .htaccess files in place I can locally run whichever test
I
> want and make sure it passes when I expect it to.

Sorry, I thought your concern was specifically with uploading .htaccess files.
You're right, it is not any better.

> 
> > 2 - You're probably right, we have layout tests that check whether the
origin
> trials system is working at all, with different combinations of good, bad and
> missing tokens; You can probably get away with enabling features through
> individual flags, or through experimental-web-platform-features and testing in
> that configuration.
> 
> Certainly once the origin trial framework is a bit more robust (not having to
> replace RuntimeEnabled with OriginTrialEnabled all over the place, not having
to
> write manual code in V8Bindings, etc) it seems fine to not explicitly test a
> specific feature being enabled by an origin trial, as you'd essentially be
> testing auto-generated code (certainly for simple features). I'm just not
> entirely sure how we make sure that until we reach that point (and any
> refactorings that are done to reach that point) nothing breaks. But I'm fine
> with "manually test to make sure origin trials still work" being the answer.
> 
> > > 
> > > oh, one more question: currently blink code loops over all tokens and
calls
> > > WebTrialTokenValidator::validateToken for each one. I wonder if it might
> make
> > > sense to instead have the blink code just pass all tokens it knows about
in
> one
> > > call to a WebTrialTokenValidator::valideTokens method? That way all the
> logic
> > > about validation result priorities etc could live in content as well,
making
> it
> > > easier to share that logic with something like the
> > > TrialTokenValidator::RequestEnablesFeature method I added here.
> > 
> > That's a good idea; I know that it's not very efficient, the way we're doing
> it right now -- especially since we loop over the tokens on every
> <feature>Enabled() call, until we find one.
> 
> Okay, I'll try to do something like that. Not sure if it really would change
> anything with efficiency though. It'll mostly be moving code around. For
> efficiency it seems like something like OriginTrialContext::isFeatureEnabled
> caching the result would be more helpful (although that method really
shouldn't
> be called that much for any particular execution context anyway, so not sure
> there will be much to gain there).

[Marijn Kruisselbrink, 2016-07-11 23:47:29]

On 2016/07/08 at 19:35:47, iclelland wrote:
> On 2016/07/06 22:52:01, Marijn Kruisselbrink wrote:
> > On 2016/07/04 at 03:53:18, falken wrote:
> > > Just to clarify the origin trial. If a page on a.com makes a request to
b.com,
> > then only b.com needs to have foreign fetch enabled for foreign fetch to
happen?
> > 
> > That's what this patch implements at least, yes. There's good arguments for
both
> > sides (requiring a.com to opt-in kind eliminates some use cases foreign
fetch
> > might be useful for, but on the other hand not requiring a.com to opt-in
makes
> > it a bit weird that experimental features end up effecting a.com). In the
end I
> > think we settled on only requiring b.com to opt-in, but we'll need good UMA
> > stats (I haven't quite figured out what those would be though) and a kill
switch
> > to be able to turn things off easily if things go wrong.
> > 
> > On 2016/07/05 at 13:47:36, iclelland wrote:
> > > On 2016/07/01 23:56:14, Marijn Kruisselbrink wrote:
> > > > On 2016/07/01 at 23:52:17, Marijn Kruisselbrink wrote:
> > > > > This is my initial attempt at exposing foreign fetch behind an origin
> > trial; a
> > > > couple questions:
> > > > > 
> > > > > - any idea what I'm doing wrong with the HTMLLinkElement scope
attribute?
> > The
> > > > webexposed layout tests are failing because the attribute somehow ended
up
> > on
> > > > HTMLElement instead.
> > > 
> > > This is happening because the generated installForeignFetch method takes
an
> > object instance, but is being passed the interface object. Since scope is
> > supposed to be installed OnPrototype, it looks up the prototype chain for
> > HTMLLinkElement and finds HTMLElement. That's my fault; I originally wrote
those
> > install* methods to take instances like 'navigator', 'window', or
'internals';
> > they're not designed to take the prototype or interface directly. I think I
can
> > fix that easily, if you can wait for a patch to rebase against.
> > 
> > Ah yeah, of course. It definitely seems unfortunate to require having an
object
> > instance to be able to install origin trial code. And even looking up the
global
> > "HTMLLinkElement" like this patch is currently doing and installing the
origin
> > trial on that seems like it wouldn't always work (interfaces with
> > [NoInterfaceObject] aren't actually exposed anywhere in the javascript
world).
> > Waiting is no problem.
> 
> You can do this now, as of 4d48d54 -- if you don't declare any members in IDL
which need to be installed OnInstance, then we don't require an instance object
in the generated methods.

Thanks. That seems to work (other than the one bug you have in 4d48d54 where you
call itertools.group_by on an unsorted container).

> > > > > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make
> > sense?
> > > > Does that actually give me the remote kill switch I want (disabling both
> > > > bindings and browser side logic)?
> > > 
> > > As long as StoragePartitionImplMap::Get is being called late enough the
the
> > OriginTrialPolicy (if present at all) will be available, then that part
makes
> > sense. Is kEnableExperimentalWebPlatformFeatures really the finest-grained
> > command-line switch that we can use for this, though? I expected there to be
a
> > base::Feature for Foreign Fetch, that would have its own command-line
switch,
> > like WebUSB, but there may be reasons that I'm not seeing why that wouldn't
> > work. 
> > 
> > There wasn't really any reason to have a separate feature/command-line
switch
> > for foreign fetch until now. Maybe it makes sense to add one for the purpose
of
> > the origin trial though. I'm not sure how that would integrate with the
origin
> > trial policy though? I.e. how do we keep the feature being en/disabled in
sync
> > with the origin trial being en/disabled? I don't really want to have to deal
> > with weird edge cases were the trial is enabled but the feature isn't, or
the
> > other way around. So it seems that whatever way the kill switch for foreign
> > fetch (or any other origin trial feature) works, that kill switch should
kill
> > both the origin trial and other parts that need to be disabled.
> 
> Right -- the kill switch for foreign fetch would kill the origin trial, and
everything else.

I'm not sure if you're saying that what I'm doing now is right, as it does
achieve that, or if you're saying that there somehow is a way to have my own
base::Feature for foreign fetch and hook that up to OriginTrialPolicy to disable
the origin trial when the feature is disabled?

Also not sure what you mean with "late enough the the OriginTrialPolicy (if
present at all) will be available,"? I assume you mean "after
content::SetContentClient is called" (in chrome at the end of
"BasicStartupComplete"). I'm pretty sure that is the case as StoragePartitions
are created on-demand when a site needs them.

[Marijn Kruisselbrink, 2016-07-13 19:46:37]

Description was changed from

==========
WIP Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

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

to

==========
WIP Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

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

[Marijn Kruisselbrink, 2016-07-22 18:12:25]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 18:12:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 19:38:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 19:38:22]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2016-07-25 19:24:50]

Description was changed from

==========
WIP Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

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

to

==========
WIP Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

BUG=540509,582310,590873
==========

[Marijn Kruisselbrink, 2016-07-26 20:07:26]

Still not sure if what I'm doing is the right/best thing to do with regard to
kill switches (but not sure if there is any other way either to have one switch
kill both the trial and the feature).

And would it make sense to split off the content/common/origin_trials/ into a
separate CL? Or are you fine with leaving it all in one CL?

[iclelland, 2016-07-26 20:32:06]

> > > > > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled() make
> > > sense?
> > > > > Does that actually give me the remote kill switch I want (disabling
both
> > > > > bindings and browser side logic)?
> > > > 
> > > > As long as StoragePartitionImplMap::Get is being called late enough the
the
> > > OriginTrialPolicy (if present at all) will be available, then that part
makes
> > > sense. Is kEnableExperimentalWebPlatformFeatures really the finest-grained
> > > command-line switch that we can use for this, though? I expected there to
be
a
> > > base::Feature for Foreign Fetch, that would have its own command-line
switch,
> > > like WebUSB, but there may be reasons that I'm not seeing why that
wouldn't
> > > work. 
> > > 
> > > There wasn't really any reason to have a separate feature/command-line
switch
> > > for foreign fetch until now. Maybe it makes sense to add one for the
purpose
of
> > > the origin trial though. I'm not sure how that would integrate with the
origin
> > > trial policy though? I.e. how do we keep the feature being en/disabled in
sync
> > > with the origin trial being en/disabled? I don't really want to have to
deal
> > > with weird edge cases were the trial is enabled but the feature isn't, or
the
> > > other way around. So it seems that whatever way the kill switch for
foreign
> > > fetch (or any other origin trial feature) works, that kill switch should
kill
> > > both the origin trial and other parts that need to be disabled.
> > 
> > Right -- the kill switch for foreign fetch would kill the origin trial, and
everything else.

> I'm not sure if you're saying that what I'm doing now is right, as it does
> achieve that, or if you're saying that there somehow is a way to have my own
> base::Feature for foreign fetch and hook that up to OriginTrialPolicy to
disable
> the origin trial when the feature is disabled?

I mean that's the way it *should* work with base::Feature. I think that if there
were a base::Feature for Foreign Fetch, then you would be able to disable that
remotely (through the Finch kill switch), and that would disable everything. If
the feature is in the default (un-killed) state, then it would be considered
available in the browser process, and turned on or off in the renderer by origin
trials. And finally, if the feature is explicitly turned on (say through the
command line), then it would be available in the browser, and the origin trial
context would always enable it.

Your code right now will definitely work -- as it is, it allows us to turn off
ForeignFetch through the origin trial policy, by adding it to the disabled
experiment list, and allows it to be turned on globally with the Experimental
Features flag. It will just require some changes to
storage_partition_impl_map.cc when it goes to stable. (But that might be
inevitable anyway)

> Also not sure what you mean with "late enough the the OriginTrialPolicy (if
> present at all) will be available,"? I assume you mean "after
> content::SetContentClient is called" (in chrome at the end of
> "BasicStartupComplete"). I'm pretty sure that is the case as StoragePartitions
> are created on-demand when a site needs them.

Yes, that's what I meant there. If the StoragePartition could be created early
in the startup process, then the OriginTrialPolicy might not exist yet.

https://codereview.chromium.org/2116503004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2116503004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:819: if
(!originTrialContext->featureBindingsInstalled("LinkServiceWorker") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
I thing you could replace this with

    if (!originTrialContext->featureBindingsInstalled("ForeignFetch") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch")))

and then remove the lines that set FeatureBindingsInstalled("LinkServiceWorker")
below. This all still needs to be generated code, but the intention is that the
FeatureBindingsInstalled set has keys that are tied to the origin trial feature
names, since each origin trial should only be installed once per context.

[Marijn Kruisselbrink, 2016-07-26 20:52:48]

On 2016/07/26 at 20:32:06, iclelland wrote:
> > > > > > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled()
make
> > > > sense?
> > > > > > Does that actually give me the remote kill switch I want (disabling
both
> > > > > > bindings and browser side logic)?
> > > > > 
> > > > > As long as StoragePartitionImplMap::Get is being called late enough
the
> the
> > > > OriginTrialPolicy (if present at all) will be available, then that part
> makes
> > > > sense. Is kEnableExperimentalWebPlatformFeatures really the
finest-grained
> > > > command-line switch that we can use for this, though? I expected there
to be
> a
> > > > base::Feature for Foreign Fetch, that would have its own command-line
> switch,
> > > > like WebUSB, but there may be reasons that I'm not seeing why that
wouldn't
> > > > work. 
> > > > 
> > > > There wasn't really any reason to have a separate feature/command-line
> switch
> > > > for foreign fetch until now. Maybe it makes sense to add one for the
purpose
> of
> > > > the origin trial though. I'm not sure how that would integrate with the
> origin
> > > > trial policy though? I.e. how do we keep the feature being en/disabled
in
> sync
> > > > with the origin trial being en/disabled? I don't really want to have to
deal
> > > > with weird edge cases were the trial is enabled but the feature isn't,
or
> the
> > > > other way around. So it seems that whatever way the kill switch for
foreign
> > > > fetch (or any other origin trial feature) works, that kill switch should
> kill
> > > > both the origin trial and other parts that need to be disabled.
> > > 
> > > Right -- the kill switch for foreign fetch would kill the origin trial,
and
> everything else.
> 
> > I'm not sure if you're saying that what I'm doing now is right, as it does
> > achieve that, or if you're saying that there somehow is a way to have my own
> > base::Feature for foreign fetch and hook that up to OriginTrialPolicy to
disable
> > the origin trial when the feature is disabled?
> 
> I mean that's the way it *should* work with base::Feature. I think that if
there were a base::Feature for Foreign Fetch, then you would be able to disable
that remotely (through the Finch kill switch), and that would disable
everything. If the feature is in the default (un-killed) state, then it would be
considered available in the browser process, and turned on or off in the
renderer by origin trials. And finally, if the feature is explicitly turned on
(say through the command line), then it would be available in the browser, and
the origin trial context would always enable it.

So are you saying that yes, eventually features should have their own
base::Feature and there will be a way to hook that up to origin trials to
disable the origin trial when the feature is disabled? Or are you saying that
there already is some way (that I'm missing) to somehow kill the origin trial
for a feature when the base::Feature is explicitly disabled?

> Your code right now will definitely work -- as it is, it allows us to turn off
ForeignFetch through the origin trial policy, by adding it to the disabled
experiment list, and allows it to be turned on globally with the Experimental
Features flag. It will just require some changes to
storage_partition_impl_map.cc when it goes to stable. (But that might be
inevitable anyway)
> 
> > Also not sure what you mean with "late enough the the OriginTrialPolicy (if
> > present at all) will be available,"? I assume you mean "after
> > content::SetContentClient is called" (in chrome at the end of
> > "BasicStartupComplete"). I'm pretty sure that is the case as
StoragePartitions
> > are created on-demand when a site needs them.
> 
> Yes, that's what I meant there. If the StoragePartition could be created early
in the startup process, then the OriginTrialPolicy might not exist yet.

Okay. What had me confused is that OriginTrialPolicy is created on demand, so
it's not really possible for it not to exist. What you actually meant is that it
matters that ContentClient exists.

>
https://codereview.chromium.org/2116503004/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):
> 
>
https://codereview.chromium.org/2116503004/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:819: if
(!originTrialContext->featureBindingsInstalled("LinkServiceWorker") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
> I thing you could replace this with
> 
>     if (!originTrialContext->featureBindingsInstalled("ForeignFetch") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch")))
> 
> and then remove the lines that set
FeatureBindingsInstalled("LinkServiceWorker") below. This all still needs to be
generated code, but the intention is that the FeatureBindingsInstalled set has
keys that are tied to the origin trial feature names, since each origin trial
should only be installed once per context.

"origin trial feature names"? We have origin trials (in my case "ForeignFetch")
and feature names (in my case "ForeignFetch" and "LinkServiceWorker"). Since all
existing origin trials used the same name for both the trial and the feature
name, it wasn't entirely clear to me which name needed to be used where. But it
makes somewhat sense that it is the trial name that I should have used here and
not the feature name, although not entirely clear how that would work with
multiple features behind one origin trial like I'm doing. In particular the code
that sets the featureBindingInstalled also has a check for the
RuntimeEnabledFeature::fooEnabled, which seems to imply that these are indeed
feature names and not origin trial names that should be used here...

[Marijn Kruisselbrink, 2016-07-26 20:52:51]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 20:53:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 22:19:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 22:19:40]

Dry run: This issue passed the CQ dry run.

[iclelland, 2016-07-27 18:08:33]

On 2016/07/26 20:52:48, Marijn Kruisselbrink wrote:
> On 2016/07/26 at 20:32:06, iclelland wrote:
> > > > > > > - does the way I'm calling OriginTrialPolicy::IsFeatureDisabled()
> make
> > > > > sense?
> > > > > > > Does that actually give me the remote kill switch I want
(disabling
> both
> > > > > > > bindings and browser side logic)?
> > > > > > 
> > > > > > As long as StoragePartitionImplMap::Get is being called late enough
> the
> > the
> > > > > OriginTrialPolicy (if present at all) will be available, then that
part
> > makes
> > > > > sense. Is kEnableExperimentalWebPlatformFeatures really the
> finest-grained
> > > > > command-line switch that we can use for this, though? I expected there
> to be
> > a
> > > > > base::Feature for Foreign Fetch, that would have its own command-line
> > switch,
> > > > > like WebUSB, but there may be reasons that I'm not seeing why that
> wouldn't
> > > > > work. 
> > > > > 
> > > > > There wasn't really any reason to have a separate feature/command-line
> > switch
> > > > > for foreign fetch until now. Maybe it makes sense to add one for the
> purpose
> > of
> > > > > the origin trial though. I'm not sure how that would integrate with
the
> > origin
> > > > > trial policy though? I.e. how do we keep the feature being en/disabled
> in
> > sync
> > > > > with the origin trial being en/disabled? I don't really want to have
to
> deal
> > > > > with weird edge cases were the trial is enabled but the feature isn't,
> or
> > the
> > > > > other way around. So it seems that whatever way the kill switch for
> foreign
> > > > > fetch (or any other origin trial feature) works, that kill switch
should
> > kill
> > > > > both the origin trial and other parts that need to be disabled.
> > > > 
> > > > Right -- the kill switch for foreign fetch would kill the origin trial,
> and
> > everything else.
> > 
> > > I'm not sure if you're saying that what I'm doing now is right, as it does
> > > achieve that, or if you're saying that there somehow is a way to have my
own
> > > base::Feature for foreign fetch and hook that up to OriginTrialPolicy to
> disable
> > > the origin trial when the feature is disabled?
> > 
> > I mean that's the way it *should* work with base::Feature. I think that if
> there were a base::Feature for Foreign Fetch, then you would be able to
disable
> that remotely (through the Finch kill switch), and that would disable
> everything. If the feature is in the default (un-killed) state, then it would
be
> considered available in the browser process, and turned on or off in the
> renderer by origin trials. And finally, if the feature is explicitly turned on
> (say through the command line), then it would be available in the browser, and
> the origin trial context would always enable it.
> 
> So are you saying that yes, eventually features should have their own
> base::Feature and there will be a way to hook that up to origin trials to
> disable the origin trial when the feature is disabled? Or are you saying that
> there already is some way (that I'm missing) to somehow kill the origin trial
> for a feature when the base::Feature is explicitly disabled?

The first -- that's the goal.

> 
> > Your code right now will definitely work -- as it is, it allows us to turn
off
> ForeignFetch through the origin trial policy, by adding it to the disabled
> experiment list, and allows it to be turned on globally with the Experimental
> Features flag. It will just require some changes to
> storage_partition_impl_map.cc when it goes to stable. (But that might be
> inevitable anyway)
> > 
> > > Also not sure what you mean with "late enough the the OriginTrialPolicy
(if
> > > present at all) will be available,"? I assume you mean "after
> > > content::SetContentClient is called" (in chrome at the end of
> > > "BasicStartupComplete"). I'm pretty sure that is the case as
> StoragePartitions
> > > are created on-demand when a site needs them.
> > 
> > Yes, that's what I meant there. If the StoragePartition could be created
early
> in the startup process, then the OriginTrialPolicy might not exist yet.
> 
> Okay. What had me confused is that OriginTrialPolicy is created on demand, so
> it's not really possible for it not to exist. What you actually meant is that
it
> matters that ContentClient exists.
> 
> >
>
https://codereview.chromium.org/2116503004/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2116503004/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:819: if
> (!originTrialContext->featureBindingsInstalled("LinkServiceWorker") &&
> (RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
> originTrialContext->isFeatureEnabled("ForeignFetch"))) {
> > I thing you could replace this with
> > 
> >     if (!originTrialContext->featureBindingsInstalled("ForeignFetch") &&
> (RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
> originTrialContext->isFeatureEnabled("ForeignFetch")))
> > 
> > and then remove the lines that set
> FeatureBindingsInstalled("LinkServiceWorker") below. This all still needs to
be
> generated code, but the intention is that the FeatureBindingsInstalled set has
> keys that are tied to the origin trial feature names, since each origin trial
> should only be installed once per context.
> 
> "origin trial feature names"? We have origin trials (in my case
"ForeignFetch")
> and feature names (in my case "ForeignFetch" and "LinkServiceWorker"). Since
all
> existing origin trials used the same name for both the trial and the feature
> name, it wasn't entirely clear to me which name needed to be used where. But
it
> makes somewhat sense that it is the trial name that I should have used here
and
> not the feature name, although not entirely clear how that would work with
> multiple features behind one origin trial like I'm doing. In particular the
code
> that sets the featureBindingInstalled also has a check for the
> RuntimeEnabledFeature::fooEnabled, which seems to imply that these are indeed
> feature names and not origin trial names that should be used here...

Sorry, I should have been more precise; I meant the trial name. One origin trial
can easily map to multiple features, as it does in this case. (The reverse is
not yet possible). My intention was for the strings used in
FeatureBindingsInstalled to correspond to the trial names, so that we can ask
"are the bindings for the Foo trial configured?" and if not, then we run the
corresponding "install*" methods on all of the interfaces.

You're right though, that it isn't all clear -- In particular, what should
happen when runtime-enabled features are enabled individually, outside of the
origin trial? The way I suggested relies on them always being installed
together, which wouldn't be correct in that case.

I'm convinced that your way is correct (aside from one issue I'm also commenting
on).

[iclelland, 2016-07-27 18:09:17]

Everything LGTM, other than this one question.

https://codereview.chromium.org/2116503004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2116503004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:572: if
(!originTrialContext->featureBindingsInstalled("ForeignFetch") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
Given the rest of the discussion, should this be testing
RuntimeEnabledFeatures::foreignFetchEnabled()?

[Marijn Kruisselbrink, 2016-08-09 20:35:06]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 20:36:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-08-09 20:39:25]

Description was changed from

==========
WIP Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

BUG=540509,582310,590873
==========

to

==========
Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

BUG=540509,582310,590873
==========

[Marijn Kruisselbrink, 2016-08-09 23:12:30]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 23:13:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-08-09 23:30:58]

https://codereview.chromium.org/2116503004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2116503004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:572: if
(!originTrialContext->featureBindingsInstalled("ForeignFetch") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
On 2016/07/27 at 18:09:17, iclelland wrote:
> Given the rest of the discussion, should this be testing
RuntimeEnabledFeatures::foreignFetchEnabled()?

Yes, done.

[Marijn Kruisselbrink, 2016-08-09 23:33:12]

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

[Marijn Kruisselbrink, 2016-08-09 23:33:13]

+pfeldman for OWNERS review

Of course I won't actually land this until the intent-to-experiment
(https://groups.google.com/a/chromium.org/d/msg/blink-dev/sIzHpZVhmBE/fF0Qf15q...)
is approved.

[commit-bot: I haz the power, 2016-08-10 01:14:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 01:14:08]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2016-08-11 00:55:22]

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:819: if
(!originTrialContext->featureBindingsInstalled("LinkServiceWorker") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
Here and below: these are hard to parse, do you mind splitting them into
intermediate expressions with sound variable names or introduce a helper method?

Also, you want constants for the trial names.

[Marijn Kruisselbrink, 2016-08-11 16:49:45]

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:819: if
(!originTrialContext->featureBindingsInstalled("LinkServiceWorker") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
On 2016/08/11 at 00:55:22, pfeldman wrote:
> Here and below: these are hard to parse, do you mind splitting them into
intermediate expressions with sound variable names or introduce a helper method?
> 
> Also, you want constants for the trial names.

I'd rather not change anything here, but instead stay consistent with the
existing code (here and in V8BindingForModules). All of this should be
auto-generated anyway (crbug.com/615060), not sure why the origin trial team
hasn't done so yet. I imagine it is harder to fully auto-generate this then it
looks...

[Marijn Kruisselbrink, 2016-08-11 18:11:19]

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:819: if
(!originTrialContext->featureBindingsInstalled("LinkServiceWorker") &&
(RuntimeEnabledFeatures::linkServiceWorkerEnabled() ||
originTrialContext->isFeatureEnabled("ForeignFetch"))) {
On 2016/08/11 at 16:49:45, Marijn Kruisselbrink wrote:
> On 2016/08/11 at 00:55:22, pfeldman wrote:
> > Here and below: these are hard to parse, do you mind splitting them into
intermediate expressions with sound variable names or introduce a helper method?
> > 
> > Also, you want constants for the trial names.
> 
> I'd rather not change anything here, but instead stay consistent with the
existing code (here and in V8BindingForModules). All of this should be
auto-generated anyway (crbug.com/615060), not sure why the origin trial team
hasn't done so yet. I imagine it is harder to fully auto-generate this then it
looks...

Having said that, I'm not against changing the shape of this semi-mechanically
generated code. Especially if auto-generating this isn't going to happen in the
near future, at least improving the code that needs to be manually written is of
course a good thing. But it still seems like that should be a separate change,
changing all the existing code, rather than trying to shoe-horn it into this CL.

[Marijn Kruisselbrink, 2016-08-12 17:33:50]

pfeldman: ping (if at all possible I'd like to land this today, being feature
freeze and all...)

[pfeldman, 2016-08-12 18:49:23]

https://codereview.chromium.org/2116503004/diff/260001/content/browser/servic...
File content/browser/service_worker/foreign_fetch_request_handler.cc (right):

https://codereview.chromium.org/2116503004/diff/260001/content/browser/servic...
content/browser/service_worker/foreign_fetch_request_handler.cc:69: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
If I did not specify the experimental-web-platform command line flag and there
is origin trial policy and it has foreign fetch disabled, we should return.

So missing origin_trial_policy allows foreign fetch?

https://codereview.chromium.org/2116503004/diff/260001/content/browser/storag...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/2116503004/diff/260001/content/browser/storag...
content/browser/storage_partition_impl_map.cc:453: !origin_trial_policy ||
ditto

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:813:
v8::Local<v8::Context> context = scriptState->context();
HandleScope is missing.

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:814: ExecutionContext*
executionContext = toExecutionContext(context);
Or did you want scriptState's getExecutionContext()?

[Marijn Kruisselbrink, 2016-08-12 19:14:45]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 19:15:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-08-12 19:15:40]

addressed your comments

https://codereview.chromium.org/2116503004/diff/260001/content/browser/servic...
File content/browser/service_worker/foreign_fetch_request_handler.cc (right):

https://codereview.chromium.org/2116503004/diff/260001/content/browser/servic...
content/browser/service_worker/foreign_fetch_request_handler.cc:69: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/08/12 at 18:49:23, pfeldman wrote:
> If I did not specify the experimental-web-platform command line flag and there
is origin trial policy and it has foreign fetch disabled, we should return.
> 
> So missing origin_trial_policy allows foreign fetch?

Ah, good point. I was trying to mimic what TrialTokenValidator does, but that
indeed disables all origin trials if no origin_trial_policy is present. To clean
this all up a bit I moved this whole check (after fixing it) to a separate
IsForeignFetchEnabled method, and call that both from here and
StoragePartitionImplMap.

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:813:
v8::Local<v8::Context> context = scriptState->context();
On 2016/08/12 at 18:49:23, pfeldman wrote:
> HandleScope is missing.

OriginTrialContext::initializePendingFeatures(), the only caller of this method
creates a HandleScope specifically for calling this method. Not sure why that
would be better than just creating the handle scope here though.

https://codereview.chromium.org/2116503004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp:814: ExecutionContext*
executionContext = toExecutionContext(context);
On 2016/08/12 at 18:49:23, pfeldman wrote:
> Or did you want scriptState's getExecutionContext()?

Yeah, that's probably cleaner yes, since the v8::Context isn't used in this
method anyway. Changed it to that.

[pfeldman, 2016-08-12 19:27:08]

lgtm

[Marijn Kruisselbrink, 2016-08-12 19:29:14]

The CQ bit was unchecked by mek@chromium.org

[Marijn Kruisselbrink, 2016-08-12 19:29:16]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-08-12 19:29:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2116503004/#ps280001
(title: "address comments")

[commit-bot: I haz the power, 2016-08-12 19:29:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 20:42:38]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-08-12 20:44:05]

Description was changed from

==========
Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

BUG=540509,582310,590873
==========

to

==========
Make Foreign Fetch an origin trial.

This makes both Foreign Fetch and Link rel=serviceworker available behind
the ForeignFetch origin trial.

For foreign fetch only the service worker registering for and handling
foreign fetch events needs to opt in to the origin trial. It can then
intercept requests made from any website, including websites that haven't
opted in to the origin trial.

BUG=540509,582310,590873

Committed: https://crrev.com/33bf998f2e0bc85a90016744f77c5a01d448cefe
Cr-Commit-Position: refs/heads/master@{#411759}
==========

[commit-bot: I haz the power, 2016-08-12 20:44:07]

Patchset 14 (id:??) landed as
https://crrev.com/33bf998f2e0bc85a90016744f77c5a01d448cefe
Cr-Commit-Position: refs/heads/master@{#411759}