Support DevTools for off-main-thread-fetch

Support DevTools for off-main-thread-fetch

After this CL, we can see the requests from workers in DevTool's network tab
even if chrome://flags/#enable-off-main-thread-fetch is enabled.

BUG=443374
Review-Url: https://codereview.chromium.org/2900613002
Cr-Commit-Position: refs/heads/master@{#484009}
Committed: https://chromium.googlesource.com/chromium/src/+/3f61513654aecf07ba4fac786d17689750bf288f

[horo, 2017-05-22 04:53:59]

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

[commit-bot: I haz the power, 2017-05-22 04:54: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-05-22 05:11:24]

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

[commit-bot: I haz the power, 2017-05-22 05:11: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-05-22 05:49:19]

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

[commit-bot: I haz the power, 2017-05-22 05:49: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-05-22 05:49:55]

Patchset #3 (id:40001) has been deleted

[horo, 2017-05-22 05:49:58]

Patchset #2 (id:20001) has been deleted

[horo, 2017-05-22 05:50:00]

Patchset #1 (id:1) has been deleted

[horo, 2017-05-22 05:50:52]

Description was changed from

==========
[WIP] Supprot DevTools for off-main-thread-fetch

BUG=443374
==========

to

==========
Supprot DevTools for off-main-thread-fetch

After this CL, we can see the requests on workers in DevTool's inspector tab.

BUG=443374
==========

[horo, 2017-05-22 05:51:12]

Description was changed from

==========
Supprot DevTools for off-main-thread-fetch

After this CL, we can see the requests on workers in DevTool's inspector tab.

BUG=443374
==========

to

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests on workers in DevTool's inspector tab.

BUG=443374
==========

[horo, 2017-05-22 05:51:38]

Description was changed from

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests on workers in DevTool's inspector tab.

BUG=443374
==========

to

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests from workers in DevTool's network tab.

BUG=443374
==========

[horo, 2017-05-22 05:55:12]

Description was changed from

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests from workers in DevTool's network tab.

BUG=443374
==========

to

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests from workers in DevTool's network tab
even if chrome://flags/#enable-off-main-thread-fetch is enabled.

BUG=443374
==========

[horo, 2017-05-22 05:55:47]

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

[horo, 2017-05-22 05:55:47]

kinuko@
Could you please review this?

[kinuko, 2017-05-22 06:53:40]

Very cool.

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1460:
IsMainThread()) {
DCHECK(IsMainThread()) seems fine

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1513: if
(!cached_resource && IsMainThread()) {
This seems to be only called for document case (for style sheet resource), maybe
just have:

DCHECK(document);
DCHECK(IsMainThread());

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:73: return new
InspectorNetworkAgent(worker_global_scope);
Does it make sense to make it take a generic ExecutionContext instead? (It
doesn't look too WorkerGlobalScope specific, and it's remotely helpful for some
of my other work-- but I can rework after this patch too)

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:131: return
should_block_request;
I think now this can be implemented in BaseFetchContext and we can remove
overrides

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:250:
encoded_data_length);
ditto

[horo, 2017-05-22 07:32:17]

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1460:
IsMainThread()) {
On 2017/05/22 06:53:40, kinuko wrote:
> DCHECK(IsMainThread()) seems fine

Done.

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1513: if
(!cached_resource && IsMainThread()) {
On 2017/05/22 06:53:40, kinuko wrote:
> This seems to be only called for document case (for style sheet resource),
maybe
> just have:
> 
> DCHECK(document);
> DCHECK(IsMainThread());

Done.

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:73: return new
InspectorNetworkAgent(worker_global_scope);
On 2017/05/22 06:53:40, kinuko wrote:
> Does it make sense to make it take a generic ExecutionContext instead? (It
> doesn't look too WorkerGlobalScope specific, and it's remotely helpful for
some
> of my other work-- but I can rework after this patch too)

Done.

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:131: return
should_block_request;
On 2017/05/22 06:53:40, kinuko wrote:
> I think now this can be implemented in BaseFetchContext and we can remove
> overrides

Done.

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:250:
encoded_data_length);
On 2017/05/22 06:53:40, kinuko wrote:
> ditto

Done.

[horo, 2017-05-22 07:32:23]

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

[commit-bot: I haz the power, 2017-05-22 07:32:44]

Dry run: CQ is trying da patch.

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

[kinuko, 2017-05-22 07:51:03]

lgtm for my eyes

https://codereview.chromium.org/2900613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2900613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:72: return new
InspectorNetworkAgent(worker_global_scope);
Could you update the parameter name as well?

https://codereview.chromium.org/2900613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:259:
Member<ExecutionContext> worker_global_scope_;
execution_context_ ?

[commit-bot: I haz the power, 2017-05-22 09:38:28]

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

[commit-bot: I haz the power, 2017-05-22 09:38:28]

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-05-22 10:17:09]

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

[commit-bot: I haz the power, 2017-05-22 10:17:24]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-22 10:42:39]

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp (right):

https://codereview.chromium.org/2900613002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp:250:
encoded_data_length);
On 2017/05/22 07:32:17, horo wrote:
> On 2017/05/22 06:53:40, kinuko wrote:
> > ditto
> 
> Done.

Ah, BaseFetchContext's |execution_context_| is null while loading iframes.
It is set in FrameFetchContext::ProvideDocumentToContext().

So virtual/mojo-loading/http/tests/inspector/network/network-datareceived.html
failed.

And also we have to keep WorkerFetchContext::ShouldBlockRequestByInspector() to
block loading iframe html file by inspector.

Patch set 4 revived FrameFetchContext::DispatchDidReceiveEncodedData() and
ShouldBlockRequestByInspector().

https://codereview.chromium.org/2900613002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2900613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:72: return new
InspectorNetworkAgent(worker_global_scope);
On 2017/05/22 07:51:02, kinuko wrote:
> Could you update the parameter name as well?

Done.

https://codereview.chromium.org/2900613002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:259:
Member<ExecutionContext> worker_global_scope_;
On 2017/05/22 07:51:02, kinuko wrote:
> execution_context_ ?

Done.

[horo, 2017-05-22 10:43:54]

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

[horo, 2017-05-22 10:43:54]

dgozman@
Could you please review  third_party/WebKit/Source/devtools/front_end/?

[horo, 2017-05-22 11:31:10]

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

[commit-bot: I haz the power, 2017-05-22 11:31:27]

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-05-22 15:12:43]

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

[commit-bot: I haz the power, 2017-05-22 15:12:44]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-05-22 19:04:17]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org, pfeldman@chromium.org

[dgozman, 2017-05-22 19:04:18]

Is there a design doc or some plan outlined?
Are we going to support everything network-related in workers separately, e.g.
replaying XHRs? 
We also have some instrumentation in content/browser/devtools/network_handler,
which doesn't know about workers. Will that be a problem?

+caseq for tracing, +pfeldman in general.

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

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:612: String
loader_id = loader ? IdentifiersFactory::LoaderId(loader) : "";
Should we have a loader id for worker (not necessarily the document loader id)?

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:624: String
frame_id = loader && loader->GetFrame()
We should instead not send frame id at all, and mark it optional in protocol.

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:658: loader ?
UrlWithoutFragment(loader->Url()).GetString() : "",
I'm not sure clients are ready for empty url here.

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/probe/CoreProbes.pidl (right):

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/probe/CoreProbes.pidl:96: void
didFinishLoading(ExecutionContext* frame, unsigned long identifier,
DocumentLoader*, double finishTime, int64_t encodedDataLength, int64_t
decodedBodyLength);
|ExectionContext* frame| seems suspicious.

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js
(right):

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/timeline_model/TimelineModel.js:1011:
var events = this.inspectedTargetEvents();
+caseq for this one.

[horo, 2017-05-23 08:55:45]

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

[commit-bot: I haz the power, 2017-05-23 08:55:58]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-23 09:01:27]

Patchset #6 (id:160001) has been deleted

[horo, 2017-05-23 09:05:46]

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

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

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-05-23 09:12:47]

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

[commit-bot: I haz the power, 2017-05-23 09:12:47]

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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2017-05-23 11:26:45]

Thank you.

> Is there a design doc or some plan outlined?

https://goo.gl/hgmQbP is the design doc of the off-main-thread-fetch.

> Are we going to support everything network-related in workers separately, e.g.
> replaying XHRs? 

I think we should.
But.. replaying XHRs on worker is not currently working?

> We also have some instrumentation in content/browser/devtools/network_handler,
> which doesn't know about workers. Will that be a problem?

I think network_handler.cc is handling Cookies and NavigationPreload.
So there is no problem with this cl.

Ah, the way of setting |loader_id| in network_handler.cc looks wrong.

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

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:612: String
loader_id = loader ? IdentifiersFactory::LoaderId(loader) : "";
On 2017/05/22 19:04:18, dgozman wrote:
> Should we have a loader id for worker (not necessarily the document loader
id)?

If my understanding is correct, this loader ID is used only to keep requests
which are fetched during the page load (ex: main resource, sync XHR).
https://chromium.googlesource.com/chromium/src/+/da4917d189eadfe643253b19ef17...
So we don't need the loader id for the requests from workers.

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:624: String
frame_id = loader && loader->GetFrame()
On 2017/05/22 19:04:18, dgozman wrote:
> We should instead not send frame id at all, and mark it optional in protocol.

Done.

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:658: loader ?
UrlWithoutFragment(loader->Url()).GetString() : "",
On 2017/05/22 19:04:18, dgozman wrote:
> I'm not sure clients are ready for empty url here.

Done.
Changed to get the url from the ExecutionContext.

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/probe/CoreProbes.pidl (right):

https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/probe/CoreProbes.pidl:96: void
didFinishLoading(ExecutionContext* frame, unsigned long identifier,
DocumentLoader*, double finishTime, int64_t encodedDataLength, int64_t
decodedBodyLength);
On 2017/05/22 19:04:18, dgozman wrote:
> |ExectionContext* frame| seems suspicious.

Done.

[horo, 2017-05-23 11:26:52]

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

[commit-bot: I haz the power, 2017-05-23 11:27:04]

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-05-23 13:42:13]

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

[commit-bot: I haz the power, 2017-05-23 13:42:14]

Dry run: This issue passed the CQ dry run.

[horo, 2017-05-24 22:35:09]

dgozman@, caseq@, pfeldman@

Could you please review Patch Set 6?

[dgozman, 2017-05-25 21:42:15]

On 2017/05/24 22:35:09, horo wrote:
> dgozman@, caseq@, pfeldman@
> 
> Could you please review Patch Set 6?

Sorry for being slow, I'll take a look tomorrow.

[dgozman, 2017-05-26 23:46:48]

I'm thinking whether we should extract main-thread-only bits of
InspectorNetworkAgent into FrameNetworkAgent, and have a separate
WorkerNetworkAgent which does worker-specific stuff. WDYT?

https://codereview.chromium.org/2900613002/diff/180001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2900613002/diff/180001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:624: request_id, "" /*
loader_id */, request.url.spec(),
It was 4 parameters, and now 3?

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

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:614: String
loader_id = loader ? IdentifiersFactory::LoaderId(loader) : "";
I am worried that this class can now operate on different threads. For example,
calling LoaderId() is main-thread-only, and I'm not sure everyone touching this
code would remember that. Can we introduce some safety mechanism? I'll think
about it.

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1429:
GetNetworkStateNotifier().SetOverride(
Is this safe to call from worker thread?

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h (right):

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h:79: void
LoadEventFired(LocalFrame*);
Where is this implemented?

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:472: return
SDK.Target.Capability.JS | SDK.Target.Capability.Log |
SDK.Target.Capability.Network;
We should guard this by experiment or something to avoid sending messages which
will all produce errors.

[kinuko, 2017-05-29 09:30:10]

On 2017/05/26 23:46:48, dgozman wrote:
> I'm thinking whether we should extract main-thread-only bits of
> InspectorNetworkAgent into FrameNetworkAgent, and have a separate
> WorkerNetworkAgent which does worker-specific stuff. WDYT?

Wouldn't we have lots of code duplication then? (Just asking, recalling the past
discussion we had in https://codereview.chromium.org/2620463002/#msg83)

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

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:614: String
loader_id = loader ? IdentifiersFactory::LoaderId(loader) : "";
On 2017/05/26 23:46:47, dgozman wrote:
> I am worried that this class can now operate on different threads. For
example,
> calling LoaderId() is main-thread-only, and I'm not sure everyone touching
this
> code would remember that. Can we introduce some safety mechanism? I'll think
> about it.

One good point is that LoaderId takes DocumentLoader, which is only available if
we have Document (which basically means we're on main thread).

(+1 for putting a little clearer separation for both cases in general though)

[dgozman, 2017-05-30 23:34:47]

> > I'm thinking whether we should extract main-thread-only bits of
> > InspectorNetworkAgent into FrameNetworkAgent, and have a separate
> > WorkerNetworkAgent which does worker-specific stuff. WDYT?
> 
> Wouldn't we have lots of code duplication then? (Just asking, recalling the
past
> discussion we had in https://codereview.chromium.org/2620463002/#msg83)
We could start with what this patch does, and then maybe extract things into
different classes. I think most of the logic would still be in common base
class, so code duplication wouldn't be a problem

[horo, 2017-06-20 11:14:31]

I'm so sorry for the late response.
Currently I have to work on other P1 issue.
I will continue this CL after I finish the other issue.

[horo, 2017-06-27 07:34:58]

Patchset #7 (id:200001) has been deleted

[horo, 2017-06-27 07:43:03]

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

[commit-bot: I haz the power, 2017-06-27 07:43:12]

Dry run: CQ is trying da patch.

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

[horo, 2017-06-27 09:29:14]

https://codereview.chromium.org/2900613002/diff/180001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2900613002/diff/180001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:624: request_id, "" /*
loader_id */, request.url.spec(),
On 2017/05/26 23:46:47, dgozman wrote:
> It was 4 parameters, and now 3?

This is because frame_id is now optional as you mentioned at
https://codereview.chromium.org/2900613002/diff/140001/third_party/WebKit/Sou....

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

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:614: String
loader_id = loader ? IdentifiersFactory::LoaderId(loader) : "";
> > > I'm thinking whether we should extract main-thread-only bits of
> > > InspectorNetworkAgent into FrameNetworkAgent, and have a separate
> > > WorkerNetworkAgent which does worker-specific stuff. WDYT?
> > 
> > Wouldn't we have lots of code duplication then? (Just asking, recalling the
> past
> > discussion we had in https://codereview.chromium.org/2620463002/#msg83)
> We could start with what this patch does, and then maybe extract things into
> different classes. I think most of the logic would still be in common base
> class, so code duplication wouldn't be a problem

Added TODO comment in InspectorNetworkAgent::CreateForWorker().

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1429:
GetNetworkStateNotifier().SetOverride(
On 2017/05/26 23:46:47, dgozman wrote:
> Is this safe to call from worker thread?

Done.

Added IsMainThread() check.

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h (right):

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h:79: void
LoadEventFired(LocalFrame*);
On 2017/05/26 23:46:47, dgozman wrote:
> Where is this implemented?

Removed.
It was accidentally added while rebasing.

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2900613002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:472: return
SDK.Target.Capability.JS | SDK.Target.Capability.Log |
SDK.Target.Capability.Network;
On 2017/05/26 23:46:47, dgozman wrote:
> We should guard this by experiment or something to avoid sending messages
which
> will all produce errors.

Done.

[commit-bot: I haz the power, 2017-06-27 09:58:41]

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

[commit-bot: I haz the power, 2017-06-27 09:58:42]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-06-28 22:50:32]

Overall looks good, mostly nits below. I will take one more look tomorrow just
to be sure :)

Also, we will need some test coverage in the future.

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

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:679: }
I think it's possible to pass Maybe<String>() here:

Maybe<String> maybe_frame_id;
if (!frame_id.IsEmpty())
  maybe_frame_id = frame_id;
GetFrontend()->requestWillBeSent(...., maybe_frame_id);

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1584:
InspectorNetworkAgent::InspectorNetworkAgent(InspectedFrames* inspected_frames)
nit: I'd just add second parameter to constructor, and only pass one non-null.
It's private anyway, but we can avoid duplicating fairly tricky lines below.

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:125:
Runtime.experiments.register('offMainThreadFetch', 'Network inspection for
off-main-thread fetch', true);
'networkInWorkers', 'Network in dedicated workers'

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:365:
this._createNetworkRequest(requestId, frameId ? frameId : '', loaderId,
request.url, documentURL, initiator);
nit: frameId || ''

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:403:
eventData.frameId = frameId ? frameId : '';
ditto

[horo, 2017-06-29 02:14:23]

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

[horo, 2017-06-29 02:14:28]

Thank you.

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

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:679: }
On 2017/06/28 22:50:32, dgozman wrote:
> I think it's possible to pass Maybe<String>() here:
> 
> Maybe<String> maybe_frame_id;
> if (!frame_id.IsEmpty())
>   maybe_frame_id = frame_id;
> GetFrontend()->requestWillBeSent(...., maybe_frame_id);

Done.

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:1584:
InspectorNetworkAgent::InspectorNetworkAgent(InspectedFrames* inspected_frames)
On 2017/06/28 22:50:32, dgozman wrote:
> nit: I'd just add second parameter to constructor, and only pass one non-null.
> It's private anyway, but we can avoid duplicating fairly tricky lines below.

Done.

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:125:
Runtime.experiments.register('offMainThreadFetch', 'Network inspection for
off-main-thread fetch', true);
On 2017/06/28 22:50:32, dgozman wrote:
> 'networkInWorkers', 'Network in dedicated workers'

This is not only for dedicated workers, but also for shared workers and service
workers.
So I changed it to "Netowork in workers"

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:365:
this._createNetworkRequest(requestId, frameId ? frameId : '', loaderId,
request.url, documentURL, initiator);
On 2017/06/28 22:50:32, dgozman wrote:
> nit: frameId || ''

Done.

https://codereview.chromium.org/2900613002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:403:
eventData.frameId = frameId ? frameId : '';
On 2017/06/28 22:50:32, dgozman wrote:
> ditto

Done.

[commit-bot: I haz the power, 2017-06-29 02:14:41]

Dry run: CQ is trying da patch.

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

[horo, 2017-06-29 02:23:03]

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

[commit-bot: I haz the power, 2017-06-29 02:23:22]

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-06-29 04:28:42]

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

[commit-bot: I haz the power, 2017-06-29 04:28:44]

Dry run: This issue passed the CQ dry run.

[caseq, 2017-06-29 22:20:24]

front_end/timeline_model lgtm

[horo, 2017-07-03 06:19:56]

On 2017/06/28 22:50:32, dgozman wrote:
> Overall looks good, mostly nits below. I will take one more look tomorrow just
> to be sure :)

Ping?

[horo, 2017-07-03 06:52:35]

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

[commit-bot: I haz the power, 2017-07-03 06:52:48]

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-07-03 10:26:01]

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

[commit-bot: I haz the power, 2017-07-03 10:26:03]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-07-03 15:02:40]

lgtm

[horo, 2017-07-03 15:45:36]

The CQ bit was checked by horo@chromium.org

[horo, 2017-07-03 15:45:37]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2900613002/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2017-07-03 15:45:48]

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-07-03 15:49:40]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499096736951140,
"parent_rev": "5a994d09242441a18faf5bbf6554ac7d82faa064", "commit_rev":
"3f61513654aecf07ba4fac786d17689750bf288f"}

[commit-bot: I haz the power, 2017-07-03 15:49:56]

Description was changed from

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests from workers in DevTool's network tab
even if chrome://flags/#enable-off-main-thread-fetch is enabled.

BUG=443374
==========

to

==========
Support DevTools for off-main-thread-fetch

After this CL, we can see the requests from workers in DevTool's network tab
even if chrome://flags/#enable-off-main-thread-fetch is enabled.

BUG=443374

Review-Url: https://codereview.chromium.org/2900613002
Cr-Commit-Position: refs/heads/master@{#484009}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f61513654aecf07ba4fac786d17...
==========

[commit-bot: I haz the power, 2017-07-03 15:50:00]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/3f61513654aecf07ba4fac786d17...