Introduce Origin-Trial for Service Worker Navigation Preload

Introduce Origin-Trial for Service Worker Navigation Preload.

There are three possible states:
  - A: Navigation Preload related methods and attributes are available in JS
       and work correctly.
  - B: Navigation Preload related methods and attributes are available in
       JS. But NavigationPreloadManager's enable, disable and setHeaderValue
       methods always return a rejected promise. And FetchEvent's
       preloadResponse attribute returns a promise which always resolve with
       undefined.
  - C: Navigation Preload related methods and attributes are not available
       in JS.
The state B is very weird state. But we can't avoid this, because the generated
code OriginTrials.cpp always return true when the Origin Trial is enabled.
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/origin_trials/OriginTrials.cpp
And according to the discussion, this is the expected behavior.
https://groups.google.com/a/chromium.org/forum/#!topic/experimentation-dev/k6ZdmFp84QE

  * Origin Trial: Have an effective header.
                                 Command line
                             Default  Enable  Disabled
                   Default      B       A        B
      Field trial  Enabled      A       A        B
                   Disabled     B       A        B

  * Origin Trial: No header.
                                 Command line
                             Default  Enable  Disabled
                   Default      C       A        C
      Field trial  Enabled      C       A        C
                   Disabled     C       A        C

There is no way to test the field trial Enabled/Disabled cases in LayoutTest.
So the LayoutTests in this CL test the following combinations.
  (Field trial=Default) * (Command line=Default/Enable/Disabled) * (OriginTrial=On/Off)


BUG=649558
Review-Url: https://codereview.chromium.org/2627023002
Cr-Commit-Position: refs/heads/master@{#443742}
Committed: https://chromium.googlesource.com/chromium/src/+/519589379f0aa766e7fe2357515b0f506f001d12

[horo, 2017-01-11 10:04:36]

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

[commit-bot: I haz the power, 2017-01-11 10:05:03]

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, 2017-01-11 11:17:08]

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

[commit-bot: I haz the power, 2017-01-11 11:17:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2017-01-11 11:34:32]

Description was changed from

==========
[wip] use origin tiral

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

to

==========
[wip] use Origin Trial for Navigation Preload.


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

[horo, 2017-01-11 11:34:46]

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

[commit-bot: I haz the power, 2017-01-11 11:35:07]

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, 2017-01-11 13:15:48]

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

[commit-bot: I haz the power, 2017-01-11 13:15:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[horo, 2017-01-11 13:25:33]

Description was changed from

==========
[wip] use Origin Trial for Navigation Preload.


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

to

==========
[wip] use Origin Trial for Navigation Preload.

BUG=649558
==========

[horo, 2017-01-12 12:37:23]

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

[commit-bot: I haz the power, 2017-01-12 12:37:41]

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

[horo, 2017-01-12 12:43:47]

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

[commit-bot: I haz the power, 2017-01-12 12:43:59]

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, 2017-01-12 14:36:03]

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

[commit-bot: I haz the power, 2017-01-12 14:36:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2017-01-12 15:13:40]

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

[commit-bot: I haz the power, 2017-01-12 15:13:56]

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

[horo, 2017-01-12 15:23:42]

Description was changed from

==========
[wip] use Origin Trial for Navigation Preload.

BUG=649558
==========

to

==========
Introduce Origin-Trial for Service Worker Navigation Preload.

There are three possible states:
  - A: Navigation Preload related methods and attributes are available in JS
       and work correctly.
  - B: Navigation Preload related methods and attributes are available in
       JS. But NavigationPreloadManager's enable, disable and setHeaderValue
       methods always return a rejected promise. And FetchEvent's
       preloadResponse attribute returns a promise which always resolve with
       undefined.
  - C: Navigation Preload related methods and attributes are not available
       in JS.
The state B is very weird state. But we can't avoid this, because the generated
code OriginTrials.cpp always return true when the Origin Trial is enabled.
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/origin_trials/O...
And according to the discussion, this is the expected behavior.
https://groups.google.com/a/chromium.org/forum/#!topic/experimentation-dev/k6...

  * Origin Trial: Have an effective header.
                                 Command line
                             Default  Enable  Disabled
                   Default      B       A        B
      Field trial  Enabled      A       A        B
                   Disabled     B       A        B

  * Origin Trial: No header.
                                 Command line
                             Default  Enable  Disabled
                   Default      C       A        C
      Field trial  Enabled      C       A        C
                   Disabled     C       A        C

There is no way to test the field trial Enabled/Disabled cases in LayoutTest.
So the LayoutTests in this CL test the following combinations.
  (Field trial=Default) * (Command line=Default/Enable/Disabled) *
(OriginTrial=On/Off)


BUG=649558
==========

[horo, 2017-01-12 15:24:45]

Patchset #7 (id:120001) has been deleted

[horo, 2017-01-12 15:24:49]

Patchset #6 (id:100001) has been deleted

[horo, 2017-01-12 15:26:03]

Patchset #5 (id:80001) has been deleted

[horo, 2017-01-12 15:26:08]

Patchset #4 (id:60001) has been deleted

[horo, 2017-01-12 15:26:30]

Patchset #2 (id:20001) has been deleted

[horo, 2017-01-12 15:26:36]

Patchset #1 (id:1) has been deleted

[horo, 2017-01-12 15:32:32]

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

[horo, 2017-01-12 15:32:33]

falken@
Could you please review?

[commit-bot: I haz the power, 2017-01-12 16:29:51]

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

[commit-bot: I haz the power, 2017-01-12 16:29:52]

Dry run: This issue passed the CQ dry run.

[falken, 2017-01-13 03:43:42]

Is there any way to test the field trial Disabled case in a browser or unit
test? Could we just set the ServiceWorkerNavigationPreload feature to disabled
and verify ServiceWorkerDispatcherHost methods return an Error? Or is that
overkill?

https://codereview.chromium.org/2627023002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2627023002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:68: const char
kFeatureDisabledErrorMessage[] = "This feature is diabled.";
"disabled"

Should we give a more specific error message for when the origin trial tokens
are on the document but not the worker? Maybe we should just be more explicit,
something like:
"This feature is disabled. The service worker script needs an origin trial
token."

https://codereview.chromium.org/2627023002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2627023002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:457: //                 
 Disabled     C       A        C
Great comment.

https://codereview.chromium.org/2627023002/diff/140001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2627023002/diff/140001/content/child/runtime_...
content/child/runtime_features.cc:317: if
(base::FeatureList::GetInstance()->IsFeatureOverriddenFromCommandLine(
Let's add a comment about why this is using IsFeatureOverriddenFromCommand,
which is different from the rest of the code in this file.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html
(right):

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:14:
var intercaces =
interfaces

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:34:
worker.postMessage({port: channel.port2}, [channel.port2]);
nit: No need to use MessageChannel. Just do worker.postMessage(). And the worker
does event.source.postMessage() back.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:50:
'./resources/navigagion-preload-origin-trial-interfaces-scope-normal';
navigation

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:53:
console.log('Interfaces in normal Service Worker\n' + message);
(As talked in person, my instinct was to move the expectations into the test
file and compare with assert_equals, but it gets ugly especially since virtual/
tests require different expectations.) 

To avoid dependency on CONSOLE MESSAGE: line NNN, I think you could log to a
<div> or <pre> directly. It'll get dumped into the text file. WDYT?

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:62:
'./resources/navigagion-preload-origin-trial-interfaces-scope-enabled';
navigation

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods.html
(right):

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods.html:49:
return saw_message;
rm messagechannel

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods.html:53:
return service_worker_unregister(t, scope);
nit: could just call registration.unregister() but not big deal

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/get_interface_names.js
(right):

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/get_interface_names.js:15:
output.push(' setter ' + property_name);
nit: use braces here if the others have it

[horo, 2017-01-13 08:41:33]

I tried to create a test using ScopedFeatureList::InitAndEnableFeature().
But the behavior of this method is different from the expected behavior
of variations server configurations.

For example:
 When I call ScopedFeatureList::InitAndEnableFeature():
 IsFeatureOverriddenFromCommandLine(name, OVERRIDE_ENABLE_FEATURE) returns true
 without setting any command line flags.

So I tested the following combinations using chromium and a local variations
server.

1) Set flags(*) and start a local variations server with:
   1-1) No setting for ServiceWorkerNavigationPreload (= Default)
   1-2) enabling ServiceWorkerNavigationPreload
   1-3) disabling ServiceWorkerNavigationPreload
 (*)--variations-server-url=http://localhost:8080/seed and
    --fake-variations-channel=stable

2) Set commandline flags:
   2-1) No flag
   2-2) --enable-features=ServiceWorkerNavigationPreload
   2-3) --disable-features=ServiceWorkerNavigationPreload

3) Set flag(*) to change origin-trial-public-key, and access to
   3-1) The page with the origin-trial token.
       https://horo-test.appspot.com/origintrial/on/
      1) With the SW script with the origin-trial token. (sw-on.js)
      2) With the SW script without the origin-trial token. (sw-off.js)
   3-2) The page without the origin-trial token.
       https://horo-test.appspot.com/origintrial/off/
      1) With the SW script with the origin-trial token. (sw-on.js)
      2) With the SW script without the origin-trial token. (sw-off.js)
 (*) --origin-trial-public-key=dRCs+TocuKkocNKa0AtZ4awrt9XKH2SQCI6o4FY6BNA=

https://codereview.chromium.org/2627023002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2627023002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:68: const char
kFeatureDisabledErrorMessage[] = "This feature is diabled.";
On 2017/01/13 03:43:41, falken wrote:
> "disabled"
> 
> Should we give a more specific error message for when the origin trial tokens
> are on the document but not the worker? Maybe we should just be more explicit,
> something like:
> "This feature is disabled. The service worker script needs an origin trial
> token."

Done.

https://codereview.chromium.org/2627023002/diff/140001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2627023002/diff/140001/content/child/runtime_...
content/child/runtime_features.cc:317: if
(base::FeatureList::GetInstance()->IsFeatureOverriddenFromCommandLine(
On 2017/01/13 03:43:41, falken wrote:
> Let's add a comment about why this is using IsFeatureOverriddenFromCommand,
> which is different from the rest of the code in this file.

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html
(right):

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:14:
var intercaces =
On 2017/01/13 03:43:41, falken wrote:
> interfaces

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:34:
worker.postMessage({port: channel.port2}, [channel.port2]);
On 2017/01/13 03:43:41, falken wrote:
> nit: No need to use MessageChannel. Just do worker.postMessage(). And the
worker
> does event.source.postMessage() back.

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:50:
'./resources/navigagion-preload-origin-trial-interfaces-scope-normal';
On 2017/01/13 03:43:41, falken wrote:
> navigation

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:53:
console.log('Interfaces in normal Service Worker\n' + message);
On 2017/01/13 03:43:41, falken wrote:
> (As talked in person, my instinct was to move the expectations into the test
> file and compare with assert_equals, but it gets ugly especially since
virtual/
> tests require different expectations.) 
> 
> To avoid dependency on CONSOLE MESSAGE: line NNN, I think you could log to a
> <div> or <pre> directly. It'll get dumped into the text file. WDYT?

Unfortunately it's impossible.
The body content will be removed by testharnessreport.js.

https://chromium.googlesource.com/chromium/src/+/70eb799/third_party/WebKit/L...

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-interfaces.html:62:
'./resources/navigagion-preload-origin-trial-interfaces-scope-enabled';
On 2017/01/13 03:43:41, falken wrote:
> navigation

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods.html
(right):

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods.html:49:
return saw_message;
On 2017/01/13 03:43:41, falken wrote:
> rm messagechannel

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/navigation-preload-origin-trial-methods.html:53:
return service_worker_unregister(t, scope);
On 2017/01/13 03:43:42, falken wrote:
> nit: could just call registration.unregister() but not big deal

Done.

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/get_interface_names.js
(right):

https://codereview.chromium.org/2627023002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/get_interface_names.js:15:
output.push(' setter ' + property_name);
On 2017/01/13 03:43:42, falken wrote:
> nit: use braces here if the others have it

Done.

[horo, 2017-01-13 08:42:18]

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

[commit-bot: I haz the power, 2017-01-13 08:42:38]

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

[falken, 2017-01-13 09:13:21]

lgtm. This is really thorough work.

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:92: return
"Navigation Preload field trial is stopped.";
nit: is "field trial" a term we use in public? Maybe "The Navigation Preload
origin trial has ended."

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:98: return "No
valid Navigation Preload Orignin Trial token for the script.";
nit: "Origin"

May be good to say "service worker script" or even dynamically add the script
URL. If I understand correctly, if you do this from a document:

navigator.serviceWorker.register('sw.js').then(reg =>
reg.navigationPreload.enable());

you'd see "No valid Navigation Preload Orignin Trial token for the script" which
could be confusing.

My preferred error message would be something like "The service worker script
does not have a valid Navigation Preload Origin Trial token."

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:106:
NOT_SUPPORTED_FILELD_TRIAL_STOPPED,
nit: typo for "FIELD"

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:454: //     B1 =
NOT_SUPPORTED_FILELD_TRIAL_STOPPED
nit: typo for "field"

https://codereview.chromium.org/2627023002/diff/160001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2627023002/diff/160001/content/child/runtime_...
content/child/runtime_features.cc:321: // returns true. But this feature should
be availabe only when an valid origin
nit: available

https://codereview.chromium.org/2627023002/diff/160001/content/child/runtime_...
content/child/runtime_features.cc:322: // trial token is set. This check is done
by the generated code of
maybe clearer to say: "But, unless the command line explicitly enabled the
feature, this feature should be available only.."

[haraken, 2017-01-13 09:19:19]

owner LGTM

[horo, 2017-01-13 09:27:57]

Thank you!

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:92: return
"Navigation Preload field trial is stopped.";
On 2017/01/13 09:13:20, falken wrote:
> nit: is "field trial" a term we use in public? Maybe "The Navigation Preload
> origin trial has ended."

Done.

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:98: return "No
valid Navigation Preload Orignin Trial token for the script.";
On 2017/01/13 09:13:20, falken wrote:
> nit: "Origin"
> 
> May be good to say "service worker script" or even dynamically add the script
> URL. If I understand correctly, if you do this from a document:
> 
> navigator.serviceWorker.register('sw.js').then(reg =>
> reg.navigationPreload.enable());
> 
> you'd see "No valid Navigation Preload Orignin Trial token for the script"
which
> could be confusing.
> 
> My preferred error message would be something like "The service worker script
> does not have a valid Navigation Preload Origin Trial token." 

Done.

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:106:
NOT_SUPPORTED_FILELD_TRIAL_STOPPED,
On 2017/01/13 09:13:20, falken wrote:
> nit: typo for "FIELD"

Done.

https://codereview.chromium.org/2627023002/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:454: //     B1 =
NOT_SUPPORTED_FILELD_TRIAL_STOPPED
On 2017/01/13 09:13:21, falken wrote:
> nit: typo for "field"

Done.

https://codereview.chromium.org/2627023002/diff/160001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2627023002/diff/160001/content/child/runtime_...
content/child/runtime_features.cc:321: // returns true. But this feature should
be availabe only when an valid origin
On 2017/01/13 09:13:21, falken wrote:
> nit: available

Done.

https://codereview.chromium.org/2627023002/diff/160001/content/child/runtime_...
content/child/runtime_features.cc:322: // trial token is set. This check is done
by the generated code of
On 2017/01/13 09:13:21, falken wrote:
> maybe clearer to say: "But, unless the command line explicitly enabled the
> feature, this feature should be available only.."

Done.

[horo, 2017-01-13 09:32:19]

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

[horo, 2017-01-13 09:32:22]

horo@chromium.org changed reviewers:
+ clamy@chromium.org

[horo, 2017-01-13 09:32:23]

clamy@
Could you please review content/child/runtime_features.cc?

[commit-bot: I haz the power, 2017-01-13 09:32:44]

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, 2017-01-13 10:46:27]

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

[commit-bot: I haz the power, 2017-01-13 10:46:28]

Dry run: This issue passed the CQ dry run.

[chasej, 2017-01-13 16:28:31]

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

[chasej, 2017-01-13 16:28:32]

Drive-by code review. A few nits below.

https://codereview.chromium.org/2627023002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2627023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:111:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
Nit: This line is not needed.

The installServiceWorkerNavigationPreload()method does not need/use
an instance object. It is making changes to the prototype object. An
instance object is usually only needed for methods that install members
on the global objects (Window, *WorkerGlobalScope, .etc).

Instead, just pass v8::Local<v8::Object>(), like Foreign Fetch does
for the V8InstallEvent.

https://codereview.chromium.org/2627023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:117:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
Nit: This line is not needed. See my comment above.

[horo, 2017-01-13 16:44:16]

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

[commit-bot: I haz the power, 2017-01-13 16:44:48]

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

[horo, 2017-01-13 16:45:22]

Thank you!

https://codereview.chromium.org/2627023002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp
(right):

https://codereview.chromium.org/2627023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:111:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
On 2017/01/13 16:28:32, chasej wrote:
> Nit: This line is not needed.
> 
> The installServiceWorkerNavigationPreload()method does not need/use
> an instance object. It is making changes to the prototype object. An
> instance object is usually only needed for methods that install members
> on the global objects (Window, *WorkerGlobalScope, .etc).
> 
> Instead, just pass v8::Local<v8::Object>(), like Foreign Fetch does
> for the V8InstallEvent.

Done.

https://codereview.chromium.org/2627023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/ConditionalFeaturesForModules.cpp:117:
v8::Local<v8::Object> instanceObject = scriptState->context()->Global();
On 2017/01/13 16:28:32, chasej wrote:
> Nit: This line is not needed. See my comment above.

Done.

[clamy, 2017-01-13 17:44:10]

Thanks! content/child lgtm.

https://codereview.chromium.org/2627023002/diff/200001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2627023002/diff/200001/content/child/runtime_...
content/child/runtime_features.cc:322: // this feature should be available only
when an valid origin trial token is
nit: s/an/a

[commit-bot: I haz the power, 2017-01-13 19:06:10]

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

[commit-bot: I haz the power, 2017-01-13 19:06:11]

Dry run: This issue passed the CQ dry run.

[horo, 2017-01-13 23:01:24]

Thank you.

https://codereview.chromium.org/2627023002/diff/200001/content/child/runtime_...
File content/child/runtime_features.cc (right):

https://codereview.chromium.org/2627023002/diff/200001/content/child/runtime_...
content/child/runtime_features.cc:322: // this feature should be available only
when an valid origin trial token is
On 2017/01/13 17:44:09, clamy wrote:
> nit: s/an/a

Done.

[horo, 2017-01-13 23:01:32]

The CQ bit was checked by horo@chromium.org

[horo, 2017-01-13 23:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
haraken@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2627023002/#ps220001
(title: "s/an/a")

[commit-bot: I haz the power, 2017-01-13 23:02:25]

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, 2017-01-14 00:34:37]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484348492140260,
"parent_rev": "f9fa5b802cecdc2dfff1221ac70c8aa652129535", "commit_rev":
"519589379f0aa766e7fe2357515b0f506f001d12"}

[commit-bot: I haz the power, 2017-01-14 00:35:07]

Description was changed from

==========
Introduce Origin-Trial for Service Worker Navigation Preload.

There are three possible states:
  - A: Navigation Preload related methods and attributes are available in JS
       and work correctly.
  - B: Navigation Preload related methods and attributes are available in
       JS. But NavigationPreloadManager's enable, disable and setHeaderValue
       methods always return a rejected promise. And FetchEvent's
       preloadResponse attribute returns a promise which always resolve with
       undefined.
  - C: Navigation Preload related methods and attributes are not available
       in JS.
The state B is very weird state. But we can't avoid this, because the generated
code OriginTrials.cpp always return true when the Origin Trial is enabled.
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/origin_trials/O...
And according to the discussion, this is the expected behavior.
https://groups.google.com/a/chromium.org/forum/#!topic/experimentation-dev/k6...

  * Origin Trial: Have an effective header.
                                 Command line
                             Default  Enable  Disabled
                   Default      B       A        B
      Field trial  Enabled      A       A        B
                   Disabled     B       A        B

  * Origin Trial: No header.
                                 Command line
                             Default  Enable  Disabled
                   Default      C       A        C
      Field trial  Enabled      C       A        C
                   Disabled     C       A        C

There is no way to test the field trial Enabled/Disabled cases in LayoutTest.
So the LayoutTests in this CL test the following combinations.
  (Field trial=Default) * (Command line=Default/Enable/Disabled) *
(OriginTrial=On/Off)


BUG=649558
==========

to

==========
Introduce Origin-Trial for Service Worker Navigation Preload.

There are three possible states:
  - A: Navigation Preload related methods and attributes are available in JS
       and work correctly.
  - B: Navigation Preload related methods and attributes are available in
       JS. But NavigationPreloadManager's enable, disable and setHeaderValue
       methods always return a rejected promise. And FetchEvent's
       preloadResponse attribute returns a promise which always resolve with
       undefined.
  - C: Navigation Preload related methods and attributes are not available
       in JS.
The state B is very weird state. But we can't avoid this, because the generated
code OriginTrials.cpp always return true when the Origin Trial is enabled.
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/origin_trials/O...
And according to the discussion, this is the expected behavior.
https://groups.google.com/a/chromium.org/forum/#!topic/experimentation-dev/k6...

  * Origin Trial: Have an effective header.
                                 Command line
                             Default  Enable  Disabled
                   Default      B       A        B
      Field trial  Enabled      A       A        B
                   Disabled     B       A        B

  * Origin Trial: No header.
                                 Command line
                             Default  Enable  Disabled
                   Default      C       A        C
      Field trial  Enabled      C       A        C
                   Disabled     C       A        C

There is no way to test the field trial Enabled/Disabled cases in LayoutTest.
So the LayoutTests in this CL test the following combinations.
  (Field trial=Default) * (Command line=Default/Enable/Disabled) *
(OriginTrial=On/Off)


BUG=649558

Review-Url: https://codereview.chromium.org/2627023002
Cr-Commit-Position: refs/heads/master@{#443742}
Committed:
https://chromium.googlesource.com/chromium/src/+/519589379f0aa766e7fe2357515b...
==========

[commit-bot: I haz the power, 2017-01-14 00:35:08]

Committed patchset #6 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/519589379f0aa766e7fe2357515b...

[sof, 2017-01-16 18:18:44]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-01-16 18:18:45]

Looks like some of the tests added here are flakily crashing on the Windows
(dbg) bot, e.g.,
 

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

[horo, 2017-01-17 01:59:16]

On 2017/01/16 18:18:45, sof wrote:
> Looks like some of the tests added here are flakily crashing on the Windows
> (dbg) bot, e.g.,
>  
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...

Thank you.

I think these crashes are caused by the string thread restriction issue.
https://crbug.com/681581

[sof, 2017-01-17 07:49:10]

On 2017/01/17 01:59:16, horo wrote:
> On 2017/01/16 18:18:45, sof wrote:
> > Looks like some of the tests added here are flakily crashing on the Windows
> > (dbg) bot, e.g.,
> >  
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg...
> 
> Thank you.
> 
> I think these crashes are caused by the string thread restriction issue.
> https://crbug.com/681581

Thanks, yes - with the other CL landing just after this.

[pkalinnikov, 2017-01-17 15:06:05]

A revert of this CL (patchset #6 id:220001) has been created in
https://codereview.chromium.org/2634373002/ by pkalinnikov@chromium.org.

The reason for reverting is:
virtual/service-worker-navigation-preload-disabled/http/tests/serviceworker/chromium/resolve-after-window-close.html
(and 1 more introduced test) is consistently failing on "WebKit Linux Trusty
Leak" builder..