Show v8.parseOnBackground in DevTools Timeline

Show v8.parseOnBackground in DevTools Timeline

BUG=595985
Committed: https://crrev.com/23441ef2a1903a977eea7feef9160ef80cf19e8c
Cr-Commit-Position: refs/heads/master@{#383259}

[horo, 2016-03-18 07:21:04]

Description was changed from

==========
Show v8.parseOnBackground in DevTools Timeline

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

to

==========
Show v8.parseOnBackground in DevTools Timeline

BUG=595985
==========

[horo, 2016-03-18 07:31:24]

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

[horo, 2016-03-18 07:31:25]

caseq@
Could you please review this CL?

[caseq, 2016-03-18 23:36:04]

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h:86: const KURL&
scriptURL() const { return m_scriptURL; }
We only care for the string representation of URL, so perhaps keep and return
just the string?

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:681:
PassOwnPtr<TracedValue> InspectorParseScriptEvent::data(const String& url)
It would be nice to have some way to attribute these events to a frame -- I
assume LocalFrame* that we use in other events would be difficult to get hold of
on the call site, but resource identifier should be available, so perhaps add it
so this activity is traceable to its causes, i.e. a network resource and the
owning frame.

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:542:
details = linkifyLocation("", url, 0, 0);
nit: drop line/column numbers, these are default parameters

[alph, 2016-03-19 03:14:37]

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

[alph, 2016-03-19 03:14:38]

Can we have a test for this?

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:539:
case recordType.ParseScriptOnBackground:
nit: you can merge it with the previous cases.

[horo, 2016-03-23 07:06:06]

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h:86: const KURL&
scriptURL() const { return m_scriptURL; }
On 2016/03/18 23:36:04, caseq wrote:
> We only care for the string representation of URL, so perhaps keep and return
> just the string?

Done.

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp:681:
PassOwnPtr<TracedValue> InspectorParseScriptEvent::data(const String& url)
On 2016/03/18 23:36:04, caseq wrote:
> It would be nice to have some way to attribute these events to a frame -- I
> assume LocalFrame* that we use in other events would be difficult to get hold
of
> on the call site, but resource identifier should be available, so perhaps add
it
> so this activity is traceable to its causes, i.e. a network resource and the
> owning frame.

I added requestId here.
But I don't know how to use this in the Timeline tab.
Is it OK?

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js
(right):

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:539:
case recordType.ParseScriptOnBackground:
On 2016/03/19 03:14:38, alph wrote:
> nit: you can merge it with the previous cases.

Done.

https://codereview.chromium.org/1807193003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/timeline/TimelineUIUtils.js:542:
details = linkifyLocation("", url, 0, 0);
On 2016/03/18 23:36:04, caseq wrote:
> nit: drop line/column numbers, these are default parameters

Fixed WebInspector.linkifyResourceAsNode().

[horo, 2016-03-25 01:38:59]

caseq@

Ping?



alph@
> Can we have a test for this?
I added a layouttest (timeline-script-parse.html) in Patch Set 2.

[caseq, 2016-03-25 02:00:59]

apologies for delay -- lgtm % comments.

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/tracing/timeline-script-parse.html
(right):

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/tracing/timeline-script-parse.html:10:
var script = document.createElement('script');
double quotes please!

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/tracing/timeline-script-parse.html:11:
script.src = 'resources/timeline-script-parse.php';
ditto.

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js
(right):

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:559: if
(typeof lineNumber === "number" && !isNaN(lineNumber))
this looks quite unfortunate, how do we get a NaN here? is this because we
merged switch cases in TimelineUIUtils? If so, I'd rather have separate cases.

[horo, 2016-03-25 04:51:45]

Thank you

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/tracing/timeline-script-parse.html
(right):

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/tracing/timeline-script-parse.html:10:
var script = document.createElement('script');
On 2016/03/25 02:00:59, caseq wrote:
> double quotes please!

Done.

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/tracing/timeline-script-parse.html:11:
script.src = 'resources/timeline-script-parse.php';
On 2016/03/25 02:00:59, caseq wrote:
> ditto.

Done.

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js
(right):

https://codereview.chromium.org/1807193003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:559: if
(typeof lineNumber === "number" && !isNaN(lineNumber))
On 2016/03/25 02:00:59, caseq wrote:
> this looks quite unfortunate, how do we get a NaN here? is this because we
> merged switch cases in TimelineUIUtils? If so, I'd rather have separate cases.

Done.

linkifyLocation() calls linkifier.linkifyScriptLocation with "lineNumber - 1"
which is NaN because lineNumber is undefined.

[horo, 2016-03-25 04:54:55]

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

[horo, 2016-03-25 04:54:56]

haraken@

Could you please review third_party/WebKit/Source/bindings/core/v8/*?

[haraken, 2016-03-25 05:51:26]

LGTM

[horo, 2016-03-25 06:45:08]

The CQ bit was checked by horo@chromium.org

[horo, 2016-03-25 06:45:09]

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

[commit-bot: I haz the power, 2016-03-25 06:45:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1807193003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1807193003/40001

[commit-bot: I haz the power, 2016-03-25 06:52:41]

Description was changed from

==========
Show v8.parseOnBackground in DevTools Timeline

BUG=595985
==========

to

==========
Show v8.parseOnBackground in DevTools Timeline

BUG=595985
==========

[commit-bot: I haz the power, 2016-03-25 06:52:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-25 06:53:56]

Description was changed from

==========
Show v8.parseOnBackground in DevTools Timeline

BUG=595985
==========

to

==========
Show v8.parseOnBackground in DevTools Timeline

BUG=595985

Committed: https://crrev.com/23441ef2a1903a977eea7feef9160ef80cf19e8c
Cr-Commit-Position: refs/heads/master@{#383259}
==========

[commit-bot: I haz the power, 2016-03-25 06:53:57]

Patchset 3 (id:??) landed as
https://crrev.com/23441ef2a1903a977eea7feef9160ef80cf19e8c
Cr-Commit-Position: refs/heads/master@{#383259}