DevTools: Move network request instrumentation points for timeline down the stack.

DevTools: Move network request instrumentation points for timeline down the stack.

This way the events cover more CPU time attribution.
Also it allows link network activity to the JS execution caused by it.

[alph, 2017-02-18 19:05:46]

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

[alph, 2017-02-18 19:05:53]

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

[commit-bot: I haz the power, 2017-02-18 19:06:07]

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

[commit-bot: I haz the power, 2017-02-18 19:40:03]

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

[commit-bot: I haz the power, 2017-02-18 19:40:03]

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

[dgozman, 2017-02-19 18:56:34]

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

[dgozman, 2017-02-19 18:56:35]

Exciting! What's the whole plan with platform/instrumentation? Is there a brief
description or a design doc anywhere?

[pfeldman, 2017-02-19 20:17:29]

you need to put it under instrumentatio/inspector to avoid confusion - we don't
want your code to be used.

[alph, 2017-02-20 19:52:07]

On 2017/02/19 20:17:29, pfeldman wrote:
> you need to put it under instrumentatio/inspector to avoid confusion - we
don't
> want your code to be used.

done

On 2017/02/19 18:56:35, dgozman wrote:
> Exciting! What's the whole plan with platform/instrumentation? Is there a
brief
> description or a design doc anywhere?

Not really. This is a minor change. The patch just moves some trace events down
the stack from core to platform

[alph, 2017-02-20 19:52:22]

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

[commit-bot: I haz the power, 2017-02-20 19:52:32]

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-02-20 20:55:46]

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

[commit-bot: I haz the power, 2017-02-20 20:55:47]

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

[alph, 2017-02-21 18:09:10]

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

[commit-bot: I haz the power, 2017-02-21 18:10: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-02-21 22:14:00]

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

[commit-bot: I haz the power, 2017-02-21 22:14:01]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[caseq, 2017-02-21 22:53:30]

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/IdentifiersFactory.h (right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/IdentifiersFactory.h:40: class
CORE_EXPORT IdentifiersFactory : public PlatformIdentifiersFactory {
Why?

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:776:
TRACE_EVENT1("devtools.timeline", "ResourceReceiveResponse", "data",
We really need them properly attributed on the front-end, I think you're
regressing things here. This should include (or happen after) a change that
takes care of propagating frame through these on the front-end.

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:34: #include
<memory>
add blank line

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/instrumentation/inspector/PlatformInspectorTraceEvents.cpp
(right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/instrumentation/inspector/PlatformInspectorTraceEvents.cpp:82:
String requestId = PlatformIdentifiersFactory::requestId(identifier);
Let's avoid doing this -- perhaps use the original request identifier here and
pair it to the network one once in the vicinity of willSendRequest(). This will
let you keep IdentifiersFactory where it is now and will give us some
flexibility in terms of ids on either side.

[alph, 2017-02-22 00:31:32]

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/IdentifiersFactory.h (right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/IdentifiersFactory.h:40: class
CORE_EXPORT IdentifiersFactory : public PlatformIdentifiersFactory {
On 2017/02/21 22:53:30, caseq wrote:
> Why?

That was ok, but it's gone anyway.

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:776:
TRACE_EVENT1("devtools.timeline", "ResourceReceiveResponse", "data",
On 2017/02/21 22:53:30, caseq wrote:
> We really need them properly attributed on the front-end, I think you're
> regressing things here. This should include (or happen after) a change that
> takes care of propagating frame through these on the front-end.

It is possible to reconstruct the frame from sendRequest on the client side.

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:34: #include
<memory>
On 2017/02/21 22:53:30, caseq wrote:
> add blank line

Done.

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/instrumentation/inspector/PlatformInspectorTraceEvents.cpp
(right):

https://codereview.chromium.org/2706643002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/instrumentation/inspector/PlatformInspectorTraceEvents.cpp:82:
String requestId = PlatformIdentifiersFactory::requestId(identifier);
On 2017/02/21 22:53:30, caseq wrote:
> Let's avoid doing this -- perhaps use the original request identifier here and
> pair it to the network one once in the vicinity of willSendRequest(). This
will
> let you keep IdentifiersFactory where it is now and will give us some
> flexibility in terms of ids on either side.

Done.

[caseq, 2017-02-22 00:45:33]

>> We really need them properly attributed on the front-end, I think you're
>> regressing things here. This should include (or happen after) a change that
>> takes care of propagating frame through these on the front-end.

> It is possible to reconstruct the frame from sendRequest on the client side.

Yeah. What I meant is that we should do it either before or within this change.

Otherwise things looks good now.

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

https://codereview.chromium.org/2706643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:501:
value->setDouble("requestId", identifier);
let's keep them strings for consistency.

https://codereview.chromium.org/2706643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:511:
value->setDouble("requestId", identifier);
ditto.

https://codereview.chromium.org/2706643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:791:
value->setDouble("requestId", identifier);
ditto.

[alph, 2017-02-22 00:53:18]

all done

[caseq, 2017-02-22 21:33:08]

lgtm

[pfeldman, 2017-02-23 01:01:57]

We need to define the right platform-level instrumentation instead.

[dgozman, 2017-07-15 05:45:58]

Should we close this?