Support redirect responses for NavigationPreload

Support redirect responses for NavigationPreload

BUG=691503
Review-Url: https://codereview.chromium.org/2790433003
Cr-Commit-Position: refs/heads/master@{#461333}
Committed: https://chromium.googlesource.com/chromium/src/+/96efe9c104f0618c637c648bd12728418286f254

[horo, 2017-03-30 08:21:53]

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

[commit-bot: I haz the power, 2017-03-30 08:22:19]

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

[horo, 2017-03-30 08:45:53]

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

[commit-bot: I haz the power, 2017-03-30 08:46:09]

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

[horo, 2017-03-30 08:46:34]

Description was changed from

==========
support redirect response for NavigationPreload

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

to

==========
support redirect response for NavigationPreload

BUG=691503
==========

[commit-bot: I haz the power, 2017-03-30 09:30:11]

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

[commit-bot: I haz the power, 2017-03-30 09:30:12]

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

[horo, 2017-03-30 10:32:11]

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

[commit-bot: I haz the power, 2017-03-30 10:32:19]

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

[horo, 2017-03-30 10:37:17]

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

[commit-bot: I haz the power, 2017-03-30 10:37:34]

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

[horo, 2017-03-30 11:28:59]

Patchset #6 (id:100001) has been deleted

[horo, 2017-03-30 11:29:01]

Patchset #5 (id:80001) has been deleted

[horo, 2017-03-30 11:29:03]

Patchset #4 (id:60001) has been deleted

[horo, 2017-03-30 11:29:06]

Patchset #3 (id:40001) has been deleted

[horo, 2017-03-30 11:29:08]

Patchset #2 (id:20001) has been deleted

[horo, 2017-03-30 11:29:10]

Patchset #1 (id:1) has been deleted

[horo, 2017-03-30 11:29:16]

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

[commit-bot: I haz the power, 2017-03-30 11:29:34]

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-03-30 11:32:45]

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

[commit-bot: I haz the power, 2017-03-30 11:32:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2017-03-30 11:34:22]

Patchset #2 (id:140001) has been deleted

[horo, 2017-03-30 11:34:58]

Patchset #1 (id:120001) has been deleted

[horo, 2017-03-30 11:35:13]

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

[horo, 2017-03-30 11:35:23]

Description was changed from

==========
support redirect response for NavigationPreload

BUG=691503
==========

to

==========
Support redirect responses for NavigationPreload

BUG=691503
==========

[commit-bot: I haz the power, 2017-03-30 11:35:30]

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

[horo, 2017-03-30 11:36:40]

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

[horo, 2017-03-30 11:36:41]

falken@
Could you please review this?

[horo, 2017-03-30 12:59:54]

On 2017/03/30 11:36:41, horo wrote:
> falken@
> Could you please review this?

Ah, sorry.
Patch Set 1 doesn't work well with DevTools.
Please wait until I fix it.

[commit-bot: I haz the power, 2017-03-30 13:24:58]

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

[commit-bot: I haz the power, 2017-03-30 13:25:00]

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-03-30 14:12:35]

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

[commit-bot: I haz the power, 2017-03-30 14:12:50]

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-03-30 15:37:17]

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

[commit-bot: I haz the power, 2017-03-30 15:37:17]

Dry run: This issue passed the CQ dry run.

[horo, 2017-03-30 15:46:51]

On 2017/03/30 12:59:54, horo wrote:
> On 2017/03/30 11:36:41, horo wrote:
> > falken@
> > Could you please review this?
> 
> Ah, sorry.
> Patch Set 1 doesn't work well with DevTools.
> Please wait until I fix it.

Uploaded Patch Set 2 that fixed DevTools problem.
Please review it.

Thank you.

[falken, 2017-03-31 07:36:03]

If possible the browser tests seem better suited as WPT tests, but lgtm.

https://codereview.chromium.org/2790433003/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2790433003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:2142: // an error.
Make this a TODO with bug?

[horo, 2017-03-31 09:49:05]

Patchset #3 (id:200001) has been deleted

[horo, 2017-03-31 09:50:12]

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

[commit-bot: I haz the power, 2017-03-31 09:50:22]

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

[horo, 2017-03-31 09:53:28]

Patchset #3 (id:220001) has been deleted

[horo, 2017-03-31 09:53:59]

The CQ bit was unchecked by horo@chromium.org

[horo, 2017-03-31 09:54:21]

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

[commit-bot: I haz the power, 2017-03-31 09:54:32]

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

[horo, 2017-03-31 09:56:30]

Moved to WPT tests.
Could you please review WPT tests?

https://codereview.chromium.org/2790433003/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2790433003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:2142: // an error.
On 2017/03/31 07:36:03, falken wrote:
> Make this a TODO with bug?

Done.

[commit-bot: I haz the power, 2017-03-31 11:37:22]

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

[commit-bot: I haz the power, 2017-03-31 11:37:23]

Dry run: This issue passed the CQ dry run.

[falken, 2017-03-31 12:42:14]

lgtm, thank you

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html
(right):

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html:29:
function onMessage(e) {
nit: wait_for_message and on_message

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html:46:
.concat(with_iframe(scope))))
seems this doesn't need to be a new .then() and return Promise.all(...) can be
moved to after line 43?

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html:52:
}
nit: frame.remove(). the WPT version of with_iframe() doesn't remove the frame
automatically, and we got feedback from Mozilla that they prefer removing the
frame explicitly to ensure it happens before the unregister() call.

Also they prefer adding return registration.unregister() here instead of relying
on the completion callback, since it doesn't wait for the promise to be
resolved.

(I think we'll have to clean up a lot of tests or change the test infra somehow
to do this consistently.)

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-worker.js
(right):

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-worker.js:15:
r.headers.forEach((v, n) => { info.headers.push([n,v]); });
nit: Can you expand the v and n abbreviations (I guess "value" and "name")? Are
they supposed to be reversed when pushing to |info|?

[horo, 2017-04-01 15:17:12]

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html
(right):

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html:29:
function onMessage(e) {
On 2017/03/31 12:42:13, falken wrote:
> nit: wait_for_message and on_message

Done.

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html:46:
.concat(with_iframe(scope))))
On 2017/03/31 12:42:13, falken wrote:
> seems this doesn't need to be a new .then() and return Promise.all(...) can be
> moved to after line 43?

Done.

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html:52:
}
On 2017/03/31 12:42:13, falken wrote:
> nit: frame.remove(). the WPT version of with_iframe() doesn't remove the frame
> automatically, and we got feedback from Mozilla that they prefer removing the
> frame explicitly to ensure it happens before the unregister() call.
> 
> Also they prefer adding return registration.unregister() here instead of
relying
> on the completion callback, since it doesn't wait for the promise to be
> resolved.
> 
> (I think we'll have to clean up a lot of tests or change the test infra
somehow
> to do this consistently.)

Done.

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-worker.js
(right):

https://codereview.chromium.org/2790433003/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigation-preload/resources/redirect-worker.js:15:
r.headers.forEach((v, n) => { info.headers.push([n,v]); });
On 2017/03/31 12:42:13, falken wrote:
> nit: Can you expand the v and n abbreviations (I guess "value" and "name")?
Are
> they supposed to be reversed when pushing to |info|?

Done.
Ah we don't need to reverse them.

[horo, 2017-04-01 15:17:17]

The CQ bit was checked by horo@chromium.org

[horo, 2017-04-01 15:17:17]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2790433003/#ps260001
(title: "incorporated falken's comment")

[commit-bot: I haz the power, 2017-04-01 15:17:23]

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-04-01 18:07:04]

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

[commit-bot: I haz the power, 2017-04-01 18:07:06]

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

[horo, 2017-04-01 23:18:18]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-04-01 23:18:33]

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-04-02 00:08:36]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1491088698774970,
"parent_rev": "71e44249f1bddfa5cfe90041b4b4cf497f39ecc0", "commit_rev":
"96efe9c104f0618c637c648bd12728418286f254"}

[commit-bot: I haz the power, 2017-04-02 00:10:33]

Description was changed from

==========
Support redirect responses for NavigationPreload

BUG=691503
==========

to

==========
Support redirect responses for NavigationPreload

BUG=691503

Review-Url: https://codereview.chromium.org/2790433003
Cr-Commit-Position: refs/heads/master@{#461333}
Committed:
https://chromium.googlesource.com/chromium/src/+/96efe9c104f0618c637c648bd127...
==========

[commit-bot: I haz the power, 2017-04-02 00:10:34]

Committed patchset #4 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/96efe9c104f0618c637c648bd127...

[jeffcarp, 2017-04-03 17:59:25]

On 2017/04/02 at 00:10:34, commit-bot wrote:
> Committed patchset #4 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/96efe9c104f0618c637c648bd127...

GitHub PR created: https://github.com/w3c/web-platform-tests/pull/5325