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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. It
has introduced several DCHECK crashes. So I reverted it.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

BUG=649558
Review-Url: https://codereview.chromium.org/2654993006
Cr-Commit-Position: refs/heads/master@{#446560}
Committed: https://chromium.googlesource.com/chromium/src/+/6f9e15e126016658be28422100f9c4d1d4da6804

[horo, 2017-01-26 15:50:22]

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

[commit-bot: I haz the power, 2017-01-26 15:50:40]

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-26 16:12:16]

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

[commit-bot: I haz the power, 2017-01-26 16:12:17]

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...)

[horo, 2017-01-26 16:12:41]

Description was changed from

==========
[WIP]send Navigation Preload to dev tools from browser process

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

to

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/

But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

[horo, 2017-01-26 16:12:55]

Patchset #1 (id:1) has been deleted

[horo, 2017-01-26 16:13:50]

Description was changed from

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/

But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

to

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

[horo, 2017-01-26 16:25:12]

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

[commit-bot: I haz the power, 2017-01-26 16:25: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-26 16:25:56]

Patchset #1 (id:20001) has been deleted

[horo, 2017-01-26 16:26:20]

Description was changed from

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

to

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

[horo, 2017-01-26 16:26:42]

Description was changed from

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

to

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes. So I reverted it.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

[horo, 2017-01-26 16:38:10]

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

[horo, 2017-01-26 16:38:11]

pfeldman@
Could you please review this?

[horo, 2017-01-26 16:50:38]

Description was changed from

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. So it
has introduced several DCHECK crashes. So I reverted it.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

to

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. It
has introduced several DCHECK crashes. So I reverted it.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

[pfeldman, 2017-01-26 18:10:20]

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:553:
std::unique_ptr<DictionaryValue> headers_dict(DictionaryValue::create());
Check for enabled_ first, you might be not enabled yet.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:559: request_id, "" /*
frameId */, "" /* loaderId */, "" /* documentURL */,
What is this request_id? You should not clash with the renderer-initiated ones,
so you should prefix it.

You might also want to add some values for frameId and loaderId, those could be
supplied by your service worker. It is nice to know the origin of the request.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:581:
std::unique_ptr<DictionaryValue> headers_dict(DictionaryValue::create());
if (!enabled_)
  return;

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:603: if
(head.devtools_info) {
I don't see where you are setting report_raw_headers = true to the request so
that you could actually receive this info. It should be set conditionally to the
agent being there and being enabled.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:613: if
(head.was_fetched_via_spdy) {
nit: You extracted the rest converters, you should extract this as well.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.h (right):

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.h:21: struct
ResourceRequestCompletionStatus;
nit: alphabetic order is off

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol_config.json (right):

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol_config.json:54: "domain": "Runtime",
Did it not compiler without these because of the initiators? I would not think
you need all of these.

[commit-bot: I haz the power, 2017-01-26 19:13:37]

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

[commit-bot: I haz the power, 2017-01-26 19:13:38]

Dry run: This issue passed the CQ dry run.

[horo, 2017-01-26 22:52:45]

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

[horo, 2017-01-26 22:53:20]

Thank you.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:553:
std::unique_ptr<DictionaryValue> headers_dict(DictionaryValue::create());
On 2017/01/26 18:10:20, pfeldman wrote:
> Check for enabled_ first, you might be not enabled yet.

Done.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:559: request_id, "" /*
frameId */, "" /* loaderId */, "" /* documentURL */,
On 2017/01/26 18:10:20, pfeldman wrote:
> What is this request_id? You should not clash with the renderer-initiated
ones,
> so you should prefix it.
> 
> You might also want to add some values for frameId and loaderId, those could
be
> supplied by your service worker. It is nice to know the origin of the request.

Done.

request_id is created by MayBeReportToDevTools() in
service_worker_fetch_dispatcher.cc.

base::StringPrintf("preload-%d", fetch_event_id);

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:581:
std::unique_ptr<DictionaryValue> headers_dict(DictionaryValue::create());
On 2017/01/26 18:10:20, pfeldman wrote:
> if (!enabled_)
>   return;

Done.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:603: if
(head.devtools_info) {
On 2017/01/26 18:10:20, pfeldman wrote:
> I don't see where you are setting report_raw_headers = true to the request so
> that you could actually receive this info. It should be set conditionally to
the
> agent being there and being enabled.

The |report_raw_headers| flag is copied from the original navigation request in
ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload().

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:613: if
(head.was_fetched_via_spdy) {
On 2017/01/26 18:10:20, pfeldman wrote:
> nit: You extracted the rest converters, you should extract this as well.

Done.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.h (right):

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.h:21: struct
ResourceRequestCompletionStatus;
On 2017/01/26 18:10:20, pfeldman wrote:
> nit: alphabetic order is off

Done.

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol_config.json (right):

https://codereview.chromium.org/2654993006/diff/40001/content/browser/devtool...
content/browser/devtools/protocol_config.json:54: "domain": "Runtime",
On 2017/01/26 18:10:20, pfeldman wrote:
> Did it not compiler without these because of the initiators? I would not think
> you need all of these.

We need this.
It is because
 - requestWillBeSent requires Request.Initiator
 - Request.Initiator requires Runtime.StackTrace
 - Runtime.StackTrace requires Runtime.CallFrame
 ...

[commit-bot: I haz the power, 2017-01-26 22:53:48]

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

[pfeldman, 2017-01-27 00:06:40]

lgtm

[commit-bot: I haz the power, 2017-01-27 00:37:51]

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

[commit-bot: I haz the power, 2017-01-27 00:37:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[horo, 2017-01-27 01:53:54]

The CQ bit was checked by horo@chromium.org

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

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-27 03:07:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485482034770610,
"parent_rev": "66019590aaaba940c5fdd563f3e0c901122727d5", "commit_rev":
"6f9e15e126016658be28422100f9c4d1d4da6804"}

[commit-bot: I haz the power, 2017-01-27 03:08:22]

Description was changed from

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. It
has introduced several DCHECK crashes. So I reverted it.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

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

to

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

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

Previously I tried to implement this feature by adding InspectorNetworkAgent in
the WorkerInspectorController of Service Worker.
https://codereview.chromium.org/2620463002/
But InspectorNetworkAgent is not designed to be used at worker threads. It
has introduced several DCHECK crashes. So I reverted it.

This CL sends the notifications to the DevTools from the browser process. This
design was proposed by pfeldman@.
https://codereview.chromium.org/2620463002/#msg115 

BUG=649558

Review-Url: https://codereview.chromium.org/2654993006
Cr-Commit-Position: refs/heads/master@{#446560}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f9e15e126016658be28422100f9...
==========

[commit-bot: I haz the power, 2017-01-27 03:08:24]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6f9e15e126016658be28422100f9...