Show service worker navigation preload requests in DevTools Network tab

Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


BUG=649558
Review-Url: https://codereview.chromium.org/2620463002
Cr-Original-Commit-Position: refs/heads/master@{#445630}
Committed: https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99dbe94a45a5c622
Review-Url: https://codereview.chromium.org/2620463002
Cr-Commit-Position: refs/heads/master@{#445699}
Committed: https://chromium.googlesource.com/chromium/src/+/23276c71698583a0142e8213bada22e3ae8d0a0e

[horo, 2017-01-06 08:25:13]

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

[commit-bot: I haz the power, 2017-01-06 08:25:26]

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-06 10:09:23]

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

[commit-bot: I haz the power, 2017-01-06 10:09:23]

Dry run: 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-01-06 12:06:10]

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

[commit-bot: I haz the power, 2017-01-06 12:06:21]

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-06 12:41:49]

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

[commit-bot: I haz the power, 2017-01-06 12:42:00]

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-06 14:14:13]

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

[commit-bot: I haz the power, 2017-01-06 14:14:14]

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-06 17:42:32]

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

[commit-bot: I haz the power, 2017-01-06 17:43:17]

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-06 17:46:45]

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

[commit-bot: I haz the power, 2017-01-06 17:47:13]

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-06 19:56:02]

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

[commit-bot: I haz the power, 2017-01-06 19:56:03]

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-10 08:27:00]

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

[commit-bot: I haz the power, 2017-01-10 08:27:06]

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-10 10:04:50]

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

[commit-bot: I haz the power, 2017-01-10 10:04:51]

Dry run: This issue passed the CQ dry run.

[horo, 2017-01-10 10:15:14]

Patchset #5 (id:80001) has been deleted

[horo, 2017-01-10 10:15:20]

Patchset #4 (id:60001) has been deleted

[horo, 2017-01-10 10:15:25]

Patchset #3 (id:40001) has been deleted

[horo, 2017-01-10 10:15:28]

Patchset #2 (id:20001) has been deleted

[horo, 2017-01-10 10:16:04]

Description was changed from

==========
use WebURLResponse for Navigation Preload

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

to

==========
[WIP] Show service worker navigation preload in DevTools network tab

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

[horo, 2017-01-16 08:56:53]

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

[commit-bot: I haz the power, 2017-01-16 08:57:06]

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-16 10:19:22]

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

[commit-bot: I haz the power, 2017-01-16 10:19:33]

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-16 11:34:22]

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

[commit-bot: I haz the power, 2017-01-16 11:34:24]

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-17 00:48:59]

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

[commit-bot: I haz the power, 2017-01-17 00:49:15]

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-17 00:59:41]

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

[commit-bot: I haz the power, 2017-01-17 00:59:42]

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

[horo, 2017-01-17 01:48:28]

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

[commit-bot: I haz the power, 2017-01-17 01:48:39]

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-17 01:52:41]

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

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

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-17 02:08:58]

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

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

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-17 03:26:24]

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

[commit-bot: I haz the power, 2017-01-17 03:26:25]

Dry run: This issue passed the CQ dry run.

[horo, 2017-01-17 07:42:08]

Patchset #8 (id:220001) has been deleted

[horo, 2017-01-17 07:42:10]

Patchset #6 (id:180001) has been deleted

[horo, 2017-01-17 07:42:14]

Patchset #5 (id:160001) has been deleted

[horo, 2017-01-17 07:42:16]

Patchset #4 (id:140001) has been deleted

[horo, 2017-01-17 07:42:18]

Patchset #3 (id:120001) has been deleted

[horo, 2017-01-17 07:42:21]

Patchset #2 (id:100001) has been deleted

[horo, 2017-01-17 07:42:23]

Patchset #1 (id:1) has been deleted

[horo, 2017-01-17 07:42:58]

Patchset #1 (id:200001) has been deleted

[horo, 2017-01-17 07:49:12]

Description was changed from

==========
[WIP] Show service worker navigation preload in DevTools network tab

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

to

==========
Show service worker navigation preload in DevTools network tab

Demo: https://youtu.be/I-Qe_Y-xYxE

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

[horo, 2017-01-17 08:10:19]

Description was changed from

==========
Show service worker navigation preload in DevTools network tab

Demo: https://youtu.be/I-Qe_Y-xYxE

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

to

==========
Show service worker navigation preload in DevTools network tab

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


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

[horo, 2017-01-17 08:10:19]

Description was changed from

==========
Show service worker navigation preload in DevTools network tab

Demo: https://youtu.be/I-Qe_Y-xYxE

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

to

==========
Show service worker navigation preload in DevTools network tab

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


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

[horo, 2017-01-17 08:15:18]

Description was changed from

==========
Show service worker navigation preload in DevTools network tab

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


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

to

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


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

[horo, 2017-01-17 08:16:20]

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

[horo, 2017-01-17 08:16:20]

falken@
Could you please review this?

[falken, 2017-01-17 14:38:36]

https://codereview.chromium.org/2620463002/diff/240001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/240001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1505:
!requester_info->IsNavigationPreload()) {
Can you add a comment about why navigation preload is exempted from the
ReadRawCookies permission?

https://codereview.chromium.org/2620463002/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2620463002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:474:
static_cast<double>(base::Time::kMicrosecondsPerSecond);
Is this converting microseconds to seconds? ToIntervalValue() docu says "please
don't do arithmetic on this". Can we avoid that?

https://codereview.chromium.org/2620463002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:475:
preload_handle_->sent_wall_time = base::Time::Now().ToDoubleT();
Could we use Mojo time types instead of these, and then convert whatever
DevTools needs on the renderer side? Seems a bit nicer and more clear why we're
doing the conversions if it's immediately where the conversion is needed.

https://codereview.chromium.org/2620463002/diff/240001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2620463002/diff/240001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:28: double
sent_wall_time;
Could these use mojo.common.mojom.Time?

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:274:
"respondWith or waitUntil.");
FWIW this error message confused me, since I was not trying to use navigation
preload except for certain URLs. I think DevTools didn't really highlight that
the error came from the FetchEvent.preloadResponse being rejected. Maybe a
better error message is "The service worker navigation preload request was
aborted because the FetchEvent finished before FetchEvent.preloadResponse could
be resolved. If you intend to use preloadResponse, it must be guarded by
respondWith or waitUntil."

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:298: "Service
Worker navigation preload doesn't suport redirect.");
nit: "support redirects"

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:380: bool
completion_reported_ = false;
I didn't look too carefully but can we turn this into an enum instead of three
bools? So we know what's really possible of the 8 possible states.

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-worker.php
(right):

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-worker.php:26:
});
nit: service worker JS style indents would indent lines 8-11 and 14-26 by two
spaces.

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
(right):

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html:121:
<p>Tests the navigation request related events are availale in the DevTools.</p>
nit: available

[horo, 2017-01-18 14:22:52]

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

[commit-bot: I haz the power, 2017-01-18 14:23: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-01-18 14:25:02]

https://codereview.chromium.org/2620463002/diff/240001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/240001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1505:
!requester_info->IsNavigationPreload()) {
On 2017/01/17 14:38:36, falken wrote:
> Can you add a comment about why navigation preload is exempted from the
> ReadRawCookies permission?

Done.

https://codereview.chromium.org/2620463002/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2620463002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:474:
static_cast<double>(base::Time::kMicrosecondsPerSecond);
On 2017/01/17 14:38:36, falken wrote:
> Is this converting microseconds to seconds? ToIntervalValue() docu says
"please
> don't do arithmetic on this". Can we avoid that?

Humm...I think there is no other way.
There is the same logic in Blink code.
https://chromium.googlesource.com/chromium/src/+/57f212f/third_party/WebKit/S...

https://codereview.chromium.org/2620463002/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:475:
preload_handle_->sent_wall_time = base::Time::Now().ToDoubleT();
On 2017/01/17 14:38:36, falken wrote:
> Could we use Mojo time types instead of these, and then convert whatever
> DevTools needs on the renderer side? Seems a bit nicer and more clear why
we're
> doing the conversions if it's immediately where the conversion is needed.

Done.

https://codereview.chromium.org/2620463002/diff/240001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2620463002/diff/240001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:28: double
sent_wall_time;
On 2017/01/17 14:38:36, falken wrote:
> Could these use mojo.common.mojom.Time?

Done.

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:274:
"respondWith or waitUntil.");
On 2017/01/17 14:38:36, falken wrote:
> FWIW this error message confused me, since I was not trying to use navigation
> preload except for certain URLs. I think DevTools didn't really highlight that
> the error came from the FetchEvent.preloadResponse being rejected. Maybe a
> better error message is "The service worker navigation preload request was
> aborted because the FetchEvent finished before FetchEvent.preloadResponse
could
> be resolved. If you intend to use preloadResponse, it must be guarded by
> respondWith or waitUntil."

Removed this error in https://codereview.chromium.org/2645493002.

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:298: "Service
Worker navigation preload doesn't suport redirect.");
On 2017/01/17 14:38:36, falken wrote:
> nit: "support redirects"

Done.

https://codereview.chromium.org/2620463002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:380: bool
completion_reported_ = false;
On 2017/01/17 14:38:36, falken wrote:
> I didn't look too carefully but can we turn this into an enum instead of three
> bools? So we know what's really possible of the 8 possible states.

Removed this in https://codereview.chromium.org/2645493002.

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-worker.php
(right):

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-worker.php:26:
});
On 2017/01/17 14:38:36, falken wrote:
> nit: service worker JS style indents would indent lines 8-11 and 14-26 by two
> spaces.

Done.

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
(right):

https://codereview.chromium.org/2620463002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html:121:
<p>Tests the navigation request related events are availale in the DevTools.</p>
On 2017/01/17 14:38:36, falken wrote:
> nit: available

Done.

[falken, 2017-01-18 14:50:08]

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1507: // the original
request. So this check has already been carried out.
I don't really understand this. It's possible that
policy->CanReadRawCookies(child_id) is true for the original request but false
for the navigation preload request? Could you explain more why it's still OK to
bypass the check?

This looks a bit scary, so I want to understand.

https://codereview.chromium.org/2620463002/diff/260001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2620463002/diff/260001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:8: import
"mojo/common/time.mojom";
nit: alphabetical order?

https://codereview.chromium.org/2620463002/diff/260001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:970:
preload_handle->sent_wall_time.ToDoubleT());
nit: I'd pass these as standard base/Time.h objects and just convert when we
finally pass the value to DevTools, which needs the double. Or else I'd comment
in |onNavigationPreloadSent| header what units the |double| is in.

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-scope.php
(right):

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-scope.php:5:
} if (isset($_GET['BrokenCunked'])) {
Chunked

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
(right):

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html:101:
"loadIframe", [ scope + "?BrokenCunked"]);
Chunked

[commit-bot: I haz the power, 2017-01-18 14:53:36]

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

[commit-bot: I haz the power, 2017-01-18 14:53:37]

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

[horo, 2017-01-19 09:22:42]

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

[commit-bot: I haz the power, 2017-01-19 09:23:03]

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-19 09:54:07]

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

[commit-bot: I haz the power, 2017-01-19 09:54:26]

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-19 09:57:09]

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1507: // the original
request. So this check has already been carried out.
On 2017/01/18 14:50:07, falken wrote:
> I don't really understand this. It's possible that
> policy->CanReadRawCookies(child_id) is true for the original request but false
> for the navigation preload request? Could you explain more why it's still OK
to
> bypass the check?
> 
> This looks a bit scary, so I want to understand.

If my understanding is correct, this check is protecting the sensitive
information from malicious renderer processes. The browser process reports the
raw headers only when the DevTools is attached to the renderer process.

When a malicious renderer process without a DevTools sends a request with
report_raw_headers flag, CanReadRawCookies() returns true, so the flag is
changed to false.

Even if a new NavigationPreload request is created for the request, the flag of 
the new request is copied from the original request in 
ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). So it must be
false.

https://codereview.chromium.org/2620463002/diff/260001/content/common/service...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2620463002/diff/260001/content/common/service...
content/common/service_worker/service_worker_event_dispatcher.mojom:8: import
"mojo/common/time.mojom";
On 2017/01/18 14:50:07, falken wrote:
> nit: alphabetical order?

Removed.

https://codereview.chromium.org/2620463002/diff/260001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:970:
preload_handle->sent_wall_time.ToDoubleT());
On 2017/01/18 14:50:07, falken wrote:
> nit: I'd pass these as standard base/Time.h objects and just convert when we
> finally pass the value to DevTools, which needs the double. Or else I'd
comment
> in |onNavigationPreloadSent| header what units the |double| is in.

Done.

I added public/web/modules/serviceworker/DEPS and changed Source/web/DEPS and
changed the code to use base::Time until
ServiceWorkerGlobalScopeProxy::onNavigationPreloadSent().

But according to PRESUBMIT.py, we can't use base::Time in Blink core code.
https://chromium.googlesource.com/chromium/src/+/7055702/third_party/WebKit/P...

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-scope.php
(right):

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/resources/navigation-preload-scope.php:5:
} if (isset($_GET['BrokenCunked'])) {
On 2017/01/18 14:50:07, falken wrote:
> Chunked

Done.

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
(right):

https://codereview.chromium.org/2620463002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html:101:
"loadIframe", [ scope + "?BrokenCunked"]);
On 2017/01/18 14:50:07, falken wrote:
> Chunked

Done.

[commit-bot: I haz the power, 2017-01-19 10:18:25]

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

[commit-bot: I haz the power, 2017-01-19 10:18:26]

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

[falken, 2017-01-19 13:40:58]

lgtm for service worker. Looks like there's the redness is just an expected
string that needs updating.

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1507: // the original
request. So this check has already been carried out.
On 2017/01/19 09:57:09, horo wrote:
> On 2017/01/18 14:50:07, falken wrote:
> > I don't really understand this. It's possible that
> > policy->CanReadRawCookies(child_id) is true for the original request but
false
> > for the navigation preload request? Could you explain more why it's still OK
> to
> > bypass the check?
> > 
> > This looks a bit scary, so I want to understand.
> 
> If my understanding is correct, this check is protecting the sensitive
> information from malicious renderer processes. The browser process reports the
> raw headers only when the DevTools is attached to the renderer process.
> 
> When a malicious renderer process without a DevTools sends a request with
> report_raw_headers flag, CanReadRawCookies() returns true, so the flag is
> changed to false.
> 
> Even if a new NavigationPreload request is created for the request, the flag
of 
> the new request is copied from the original request in 
> ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). So it must be
> false.

I understand the request is copied, but don't understand why we can't just do
the check again. If in the original request, report_raw_headers was true, then
CanReadRawCookies was true when the check happened. The only reason to avoid the
check again for nav preload, is if CanReadRawCookies could now be false for nav
preload. Why would CanReadRawCookies have changed... and is it ok to still
bypass the check even if it changed? I guess it happens if the user closed
DevTools between when the request was created and nav preload was created?

https://codereview.chromium.org/2620463002/diff/300001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
(right):

https://codereview.chromium.org/2620463002/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h:105:
virtual void onNavigationPreloadSent(int fetchEventID,
nit: probably worth a comment that sentTimestamp and sentWallTime are for
reporting to DevTools.

[horo, 2017-01-19 14:14:19]

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

[commit-bot: I haz the power, 2017-01-19 14:14:33]

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-19 14:33:04]

Thank you.

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1507: // the original
request. So this check has already been carried out.
On 2017/01/19 13:40:58, falken wrote:
> On 2017/01/19 09:57:09, horo wrote:
> > On 2017/01/18 14:50:07, falken wrote:
> > > I don't really understand this. It's possible that
> > > policy->CanReadRawCookies(child_id) is true for the original request but
> false
> > > for the navigation preload request? Could you explain more why it's still
OK
> > to
> > > bypass the check?
> > > 
> > > This looks a bit scary, so I want to understand.
> > 
> > If my understanding is correct, this check is protecting the sensitive
> > information from malicious renderer processes. The browser process reports
the
> > raw headers only when the DevTools is attached to the renderer process.
> > 
> > When a malicious renderer process without a DevTools sends a request with
> > report_raw_headers flag, CanReadRawCookies() returns true, so the flag is
> > changed to false.
> > 
> > Even if a new NavigationPreload request is created for the request, the flag
> of 
> > the new request is copied from the original request in 
> > ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). So it must be
> > false.
> 
> I understand the request is copied, but don't understand why we can't just do
> the check again. If in the original request, report_raw_headers was true, then
> CanReadRawCookies was true when the check happened. The only reason to avoid
the
> check again for nav preload, is if CanReadRawCookies could now be false for
nav
> preload. Why would CanReadRawCookies have changed... and is it ok to still
> bypass the check even if it changed? I guess it happens if the user closed
> DevTools between when the request was created and nav preload was created?

CanReadRawCookies checks the child_id of ResourceRequesterInfo.
But we are using -1 for child_id of NavigationPreload.
https://chromium.googlesource.com/chromium/src/+/7aac46c/content/browser/load...
So CanReadRawCookies returns false.

https://codereview.chromium.org/2620463002/diff/300001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
(right):

https://codereview.chromium.org/2620463002/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h:105:
virtual void onNavigationPreloadSent(int fetchEventID,
On 2017/01/19 13:40:58, falken wrote:
> nit: probably worth a comment that sentTimestamp and sentWallTime are for
> reporting to DevTools.

Done.

[horo, 2017-01-19 14:36:01]

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

[horo, 2017-01-19 14:36:02]

pfeldman@
Could you please review this CL?

[commit-bot: I haz the power, 2017-01-19 15:42:07]

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

[commit-bot: I haz the power, 2017-01-19 15:42:08]

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-19 23:03:51]

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

[commit-bot: I haz the power, 2017-01-19 23:04: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-01-19 23:05:23]

Ah, I forgot to upload inspector-test.js after Patch Set 4.
So some layout tests are falling.

I uploaded Patch Set 6 with fixed inspector-test.js.

[pfeldman, 2017-01-19 23:23:48]

Is there a way we could reuse existing instrumentation for network? I.e. create
dummy ResourceRequest, ResourceResponse objects, make NetworkAgent respect
nullable LocalFrame and DocumentLoader, also apply other minor changes if
necessary.

The rationale here is that as long as you'd like to surface information as a
part of the network grid, it would be easier to instrument it as such from the
beginning.

https://codereview.chromium.org/2620463002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
(right):

https://codereview.chromium.org/2620463002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:321: void
didSendNavigationPreload(ExecutionContext*, int identifier, const KURL&
requestURL, double timestamp, double wallTime);
Can we reuse existing network instrumentation and plumbing for preload events?
After all they are surfaced as regular requests on the front-end side.

[pfeldman, 2017-01-19 23:38:16]

Also, are there any actual network requests under the hood? You are showing it
in the 'Network' tab...

[falken, 2017-01-20 01:38:54]

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1507: // the original
request. So this check has already been carried out.
On 2017/01/19 14:33:04, horo wrote:
> On 2017/01/19 13:40:58, falken wrote:
> > On 2017/01/19 09:57:09, horo wrote:
> > > On 2017/01/18 14:50:07, falken wrote:
> > > > I don't really understand this. It's possible that
> > > > policy->CanReadRawCookies(child_id) is true for the original request but
> > false
> > > > for the navigation preload request? Could you explain more why it's
still
> OK
> > > to
> > > > bypass the check?
> > > > 
> > > > This looks a bit scary, so I want to understand.
> > > 
> > > If my understanding is correct, this check is protecting the sensitive
> > > information from malicious renderer processes. The browser process reports
> the
> > > raw headers only when the DevTools is attached to the renderer process.
> > > 
> > > When a malicious renderer process without a DevTools sends a request with
> > > report_raw_headers flag, CanReadRawCookies() returns true, so the flag is
> > > changed to false.
> > > 
> > > Even if a new NavigationPreload request is created for the request, the
flag
> > of 
> > > the new request is copied from the original request in 
> > > ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). So it must be
> > > false.
> > 
> > I understand the request is copied, but don't understand why we can't just
do
> > the check again. If in the original request, report_raw_headers was true,
then
> > CanReadRawCookies was true when the check happened. The only reason to avoid
> the
> > check again for nav preload, is if CanReadRawCookies could now be false for
> nav
> > preload. Why would CanReadRawCookies have changed... and is it ok to still
> > bypass the check even if it changed? I guess it happens if the user closed
> > DevTools between when the request was created and nav preload was created?
> 
> CanReadRawCookies checks the child_id of ResourceRequesterInfo.
> But we are using -1 for child_id of NavigationPreload.
>
https://chromium.googlesource.com/chromium/src/+/7aac46c/content/browser/load...
> So CanReadRawCookies returns false.

That's the important detail I missed. Let's make that clear in the comment: "For
navigation preload, the child_id is -1 so CanReadRawCookies would return false.
But |report_raw_headers| of the navigation preload request was copied from the
original request, so this check has already been carried out."

BTW doesn't PlzNavigate also have child_id == -1? Does it not need DevTools
support? Might it make sense for CanReadRawCookies to return true for child_id
-1?

[commit-bot: I haz the power, 2017-01-20 01:53:59]

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

[commit-bot: I haz the power, 2017-01-20 01:54:01]

Dry run: This issue passed the CQ dry run.

[horo, 2017-01-20 13:08:17]

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

[commit-bot: I haz the power, 2017-01-20 13:08: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-01-20 13:09:49]

Patchset #7 (id:360001) has been deleted

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[horo, 2017-01-20 13:13:29]

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

[commit-bot: I haz the power, 2017-01-20 13:13:36]

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-20 13:18:36]

> Also, are there any actual network requests under the hood? You are showing it
> in the 'Network' tab...

Yes, it acutually send the request to the server from
ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload().
https://chromium.googlesource.com/chromium/src/+/6d38a91/content/browser/serv...

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2620463002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1507: // the original
request. So this check has already been carried out.
On 2017/01/20 01:38:54, falken wrote:
> On 2017/01/19 14:33:04, horo wrote:
> > On 2017/01/19 13:40:58, falken wrote:
> > > On 2017/01/19 09:57:09, horo wrote:
> > > > On 2017/01/18 14:50:07, falken wrote:
> > > > > I don't really understand this. It's possible that
> > > > > policy->CanReadRawCookies(child_id) is true for the original request
but
> > > false
> > > > > for the navigation preload request? Could you explain more why it's
> still
> > OK
> > > > to
> > > > > bypass the check?
> > > > > 
> > > > > This looks a bit scary, so I want to understand.
> > > > 
> > > > If my understanding is correct, this check is protecting the sensitive
> > > > information from malicious renderer processes. The browser process
reports
> > the
> > > > raw headers only when the DevTools is attached to the renderer process.
> > > > 
> > > > When a malicious renderer process without a DevTools sends a request
with
> > > > report_raw_headers flag, CanReadRawCookies() returns true, so the flag
is
> > > > changed to false.
> > > > 
> > > > Even if a new NavigationPreload request is created for the request, the
> flag
> > > of 
> > > > the new request is copied from the original request in 
> > > > ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). So it must
be
> > > > false.
> > > 
> > > I understand the request is copied, but don't understand why we can't just
> do
> > > the check again. If in the original request, report_raw_headers was true,
> then
> > > CanReadRawCookies was true when the check happened. The only reason to
avoid
> > the
> > > check again for nav preload, is if CanReadRawCookies could now be false
for
> > nav
> > > preload. Why would CanReadRawCookies have changed... and is it ok to still
> > > bypass the check even if it changed? I guess it happens if the user closed
> > > DevTools between when the request was created and nav preload was created?
> > 
> > CanReadRawCookies checks the child_id of ResourceRequesterInfo.
> > But we are using -1 for child_id of NavigationPreload.
> >
>
https://chromium.googlesource.com/chromium/src/+/7aac46c/content/browser/load...
> > So CanReadRawCookies returns false.
> 
> That's the important detail I missed. Let's make that clear in the comment:
"For
> navigation preload, the child_id is -1 so CanReadRawCookies would return
false.
> But |report_raw_headers| of the navigation preload request was copied from the
> original request, so this check has already been carried out."

Done.

> BTW doesn't PlzNavigate also have child_id == -1? Does it not need DevTools
> support? Might it make sense for CanReadRawCookies to return true for child_id
> -1?

ContinuePendingBeginRequest() is not called for BrowserSideNavigation request.
BrowserSideNavigation type ResourceRequesterInfo is created in
BeginNavigationRequest() and it directly calls BeginRequestInternal().

https://codereview.chromium.org/2620463002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl
(right):

https://codereview.chromium.org/2620463002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:321: void
didSendNavigationPreload(ExecutionContext*, int identifier, const KURL&
requestURL, double timestamp, double wallTime);
On 2017/01/19 23:23:47, pfeldman wrote:
> Can we reuse existing network instrumentation and plumbing for preload events?
> After all they are surfaced as regular requests on the front-end side.

Done.

[commit-bot: I haz the power, 2017-01-20 14:53:36]

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

[commit-bot: I haz the power, 2017-01-20 14:53:37]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-01-20 20:32:22]

> > Also, are there any actual network requests under the hood? You are showing
it
> > in the 'Network' tab...
> 
> Yes, it acutually send the request to the server from
> ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload().
>
https://chromium.googlesource.com/chromium/src/+/6d38a91/content/browser/serv...

You know what, we could try instrumenting it there, right in the browser if we
can attribute it to the service worker. I'll complete the review of the change,
but let's get back to this bit.

[pfeldman, 2017-01-20 20:41:46]

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js
(right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js:1000:
return target && !target.hasBrowserCapability() && target.hasJSCapability() &&
target.hasNetworkCapability() && !target.hasTargetCapability();
drop the network capability check altogether - it no longer differentiates
workers.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
(right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html:42:
InspectorTest.addSniffer(SDK.NetworkDispatcher.prototype,
Could you instead listen to the NetworkManager events? That way you are based on
API, not the sniffers.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:657:
std::move(requestInfo), timestamp, wallTime, std::move(initiatorObject),
We report blink time as a part of the willSendRequest instrumentation, but we
then follow up with the precise timing information as a part of ResourceResponse
(ResourceLoadTiming struct). You should do the same. I.e. this line should not
change, you should not pass these timers here.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:669: double
timestamp,
ditto

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp
(right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp:75:
m_session->append(InspectorNetworkAgent::create(nullptr));
You just created one for all dedicated workers. Not sure the consequences of
this, just noticing.

[horo, 2017-01-23 07:55:11]

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

[commit-bot: I haz the power, 2017-01-23 07:55:27]

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-23 08:02:56]

Patchset #8 (id:400001) has been deleted

[horo, 2017-01-23 08:04:37]

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

[commit-bot: I haz the power, 2017-01-23 08:04:54]

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-23 08:05:32]

> You know what, we could try instrumenting it there, right in the browser if we
> can attribute it to the service worker. I'll complete the review of the
change,
> but let's get back to this bit.

Ah, yes. But is it possible to send the existing Netowrk events like
requestWillBeSent from the browser process?

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js
(right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/inspector-test.js:1000:
return target && !target.hasBrowserCapability() && target.hasJSCapability() &&
target.hasNetworkCapability() && !target.hasTargetCapability();
On 2017/01/20 20:41:46, pfeldman wrote:
> drop the network capability check altogether - it no longer differentiates
> workers.

Done.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html
(right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-workers-navigation-preload.html:42:
InspectorTest.addSniffer(SDK.NetworkDispatcher.prototype,
On 2017/01/20 20:41:46, pfeldman wrote:
> Could you instead listen to the NetworkManager events? That way you are based
on
> API, not the sniffers.

Done.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:657:
std::move(requestInfo), timestamp, wallTime, std::move(initiatorObject),
On 2017/01/20 20:41:46, pfeldman wrote:
> We report blink time as a part of the willSendRequest instrumentation, but we
> then follow up with the precise timing information as a part of
ResourceResponse
> (ResourceLoadTiming struct). You should do the same. I.e. this line should not
> change, you should not pass these timers here.

Done.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:669: double
timestamp,
On 2017/01/20 20:41:46, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp
(right):

https://codereview.chromium.org/2620463002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/WorkerInspectorController.cpp:75:
m_session->append(InspectorNetworkAgent::create(nullptr));
On 2017/01/20 20:41:46, pfeldman wrote:
> You just created one for all dedicated workers. Not sure the consequences of
> this, just noticing.

I introduces inspectorNetworkCapability flag.
So InspectorNetworkAgent will be created only for ServiceWorker.

[commit-bot: I haz the power, 2017-01-23 08:19:42]

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

[commit-bot: I haz the power, 2017-01-23 08:19:43]

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

[horo, 2017-01-23 08:27:15]

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

[commit-bot: I haz the power, 2017-01-23 08:27: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-01-23 08:28:20]

Patchset #9 (id:440001) has been deleted

[commit-bot: I haz the power, 2017-01-23 08:28:44]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[horo, 2017-01-23 08:32:03]

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

[commit-bot: I haz the power, 2017-01-23 08:32:20]

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-23 10:26:08]

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

[commit-bot: I haz the power, 2017-01-23 10:26:09]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-01-23 18:30:22]

>> But is it possible to send the existing Netowork events like
requestWillBeSent from the browser process?

It is, content/browser/devtools/protocol/network_handler.cc is the browser-side
network agent, it can emit notifications just fine. We need to make sure id
spaces of network in blink and browser don't intersect, but we can figure
something out here. We have an unsigned long on the native side and number of
the JS side, so we could introduce ranges for different instrumentations.

lgtm for this patch, but if you can issue notifications from the browser, that'd
be closer to the plans of kicking network further away from the renderers...

[horo, 2017-01-24 03:28:46]

Thank you.


Ah, if we send loadingFinished() event from the browser process, we can't known
when the service worker finishes loading the response from DevTools.

While the render process is loading normal responses, loadingFinished() event is
dispatched when the render process finishes loading the response. So we should
do the same.

[horo, 2017-01-24 03:28:54]

The CQ bit was checked by horo@chromium.org

[horo, 2017-01-24 03:28:54]

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/2620463002/#ps460001
(title: "fix compile failure")

[commit-bot: I haz the power, 2017-01-24 03:29:26]

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-24 03:37:19]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1485228534073840,
"parent_rev": "12cbf878e211324afdaf454623ef004d1aa82d41", "commit_rev":
"4c6b3b0f11b62e069d9d84ba99dbe94a45a5c622"}

[commit-bot: I haz the power, 2017-01-24 03:37:54]

Description was changed from

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


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

to

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


BUG=649558

Review-Url: https://codereview.chromium.org/2620463002
Cr-Commit-Position: refs/heads/master@{#445630}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99db...
==========

[commit-bot: I haz the power, 2017-01-24 03:37:56]

Committed patchset #9 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99db...

[horo, 2017-01-24 08:47:12]

A revert of this CL (patchset #9 id:460001) has been created in
https://codereview.chromium.org/2649923007/ by horo@chromium.org.

The reason for reverting is: Caused layout test failures.


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=....

[horo, 2017-01-24 09:41:56]

Description was changed from

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


BUG=649558

Review-Url: https://codereview.chromium.org/2620463002
Cr-Commit-Position: refs/heads/master@{#445630}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99db...
==========

to

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


BUG=649558

Review-Url: https://codereview.chromium.org/2620463002
Cr-Commit-Position: refs/heads/master@{#445630}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99db...
==========

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

The CQ bit was checked by horo@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2620463002/#ps480001
(title: "fix crash")

[commit-bot: I haz the power, 2017-01-24 09:45:26]

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-24 11:24:25]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1485251115304630,
"parent_rev": "a486c29cc95da9fc1ef8fbbb7aff8332e75dd2a1", "commit_rev":
"23276c71698583a0142e8213bada22e3ae8d0a0e"}

[commit-bot: I haz the power, 2017-01-24 11:24:58]

Description was changed from

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


BUG=649558

Review-Url: https://codereview.chromium.org/2620463002
Cr-Commit-Position: refs/heads/master@{#445630}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99db...
==========

to

==========
Show service worker navigation preload requests in DevTools Network tab.

Demo: https://youtu.be/I-Qe_Y-xYxE

Navigation Preload requests are initiated from the browser process.
This is different from the normal network requests which are initiated from the
renderer process.

When the DevTools show the normal requests in the Network tab, DevTool's
Network events (requestWillBeSent, responseReceived, loadingFinished etc) are
dispatched via InspectorInstrumentation and InspectorNetworkAgent.

This CL introduces new DevTool's Network events (navigationPreloadSent,
navigationPreloadResponseReceived, navigationPreloadFailed,
navigationPreloadFinished) which are dispatched via InspectorInstrumentation and
InspectorNetworkAgent from ServiceWorkerContextClient.

In the normal requests case, we record the request sending timestamp when the
renderer process will send the request in InspectorNetworkAgent::
willSendRequestInternal(). But in the navigation preload case, we record the
timestamp in the browser process, and send it to the service worker's renderer
process using FetchEventPreloadHandle.


BUG=649558

Review-Url: https://codereview.chromium.org/2620463002
Cr-Original-Commit-Position: refs/heads/master@{#445630}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c6b3b0f11b62e069d9d84ba99db...
Review-Url: https://codereview.chromium.org/2620463002
Cr-Commit-Position: refs/heads/master@{#445699}
Committed:
https://chromium.googlesource.com/chromium/src/+/23276c71698583a0142e8213bada...
==========

[commit-bot: I haz the power, 2017-01-24 11:25:00]

Committed patchset #10 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/23276c71698583a0142e8213bada...

[horo, 2017-01-26 03:45:57]

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

The reason for reverting is: Introduced DCHECK crash.

BUG=685493
.