Add source line info for JS Profiler ticks to debug protocol.

Add source line info for JS Profiler ticks to debug protocol.

Contributed by Denis Pravdin <denis.pravdin@intel.com>;

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185428

[Pan, 2014-11-13 08:58:37]

pan.deng@intel.com changed reviewers:
+ yurys@chromium.org

[Pan, 2014-11-13 08:58:37]

@yurys,

This is the follow up patch of https://codereview.chromium.org/682143003/, could
you please have a look?

thanks
Pan

[apavlov, 2014-11-13 09:08:29]

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

[apavlov, 2014-11-13 09:08:31]

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
File Source/core/inspector/ScriptProfile.cpp (right):

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:71: if (lineCount) {
We prefer early bail-outs (this saves indentation and helps the readability):

if (!lineCount)
    return array;

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:73: bool res =
node->GetLineTicks(&entries[0], lineCount);
This can be inlined into the conditional.

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:79: array->addItem(line);
line.release()

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:80: line.release();
Remove this line, as it makes no sense this way. line.release() derefs line and
returns a PassRefPtr (anyway, the deref will happen automatically when leaving
the scope.)

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3643: { "name": "lineTicks", "type": "array",
"items": { "$ref": "LineTick" }, "description": "A set of source line ticks." }
"An array", to be precise

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3659: "id": "LineTick",
LineTickInfo? It is weird for a "LineTick" object to contain the "ticks" field.

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3663: { "name": "line", "type": "integer",
"description": "Source line number." },
The description should specify whether the line number is 0-based or 1-based
(see Debugger.Location for an example)

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3664: { "name": "ticks", "type": "integer",
"description": "Number of samples attributed to a source line number." }
"Number of samples attributed to the source line."

[Pan, 2014-11-17 01:14:22]

thanks for the comments, PTAL.

Pan

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
File Source/core/inspector/ScriptProfile.cpp (right):

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:71: if (lineCount) {
On 2014/11/13 09:08:30, apavlov wrote:
> We prefer early bail-outs (this saves indentation and helps the readability):
> 
> if (!lineCount)
>     return array;

Done.

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:73: bool res =
node->GetLineTicks(&entries[0], lineCount);
On 2014/11/13 09:08:30, apavlov wrote:
> This can be inlined into the conditional.

Done.

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:79: array->addItem(line);
On 2014/11/13 09:08:30, apavlov wrote:
> line.release()

Done.

https://codereview.chromium.org/719853003/diff/1/Source/core/inspector/Script...
Source/core/inspector/ScriptProfile.cpp:80: line.release();
On 2014/11/13 09:08:30, apavlov wrote:
> Remove this line, as it makes no sense this way. line.release() derefs line
and
> returns a PassRefPtr (anyway, the deref will happen automatically when leaving
> the scope.)

Done.

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3643: { "name": "lineTicks", "type": "array",
"items": { "$ref": "LineTick" }, "description": "A set of source line ticks." }
On 2014/11/13 09:08:31, apavlov wrote:
> "An array", to be precise

Done.

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3659: "id": "LineTick",
On 2014/11/13 09:08:30, apavlov wrote:
> LineTickInfo? It is weird for a "LineTick" object to contain the "ticks"
field.

Done.

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3663: { "name": "line", "type": "integer",
"description": "Source line number." },
On 2014/11/13 09:08:31, apavlov wrote:
> The description should specify whether the line number is 0-based or 1-based
> (see Debugger.Location for an example)

Done.

https://codereview.chromium.org/719853003/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:3664: { "name": "ticks", "type": "integer",
"description": "Number of samples attributed to a source line number." }
On 2014/11/13 09:08:30, apavlov wrote:
> "Number of samples attributed to the source line."

Done.

[yurys, 2014-11-17 08:12:57]

lgtm

https://codereview.chromium.org/719853003/diff/20001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/719853003/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:3651: { "name": "lineTicks", "type": "array",
"items": { "$ref": "LineTickInfo" }, "description": "An array of source line
ticks." }
Since we'd like to eventually have column info as well (for the purpose of
profiling minified scripts) I'd rather name it something like positionTicks and
PositionTickInfo so that we don't have to rename it later.

[Pan, 2014-11-17 08:49:52]

https://codereview.chromium.org/719853003/diff/20001/Source/devtools/protocol...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/719853003/diff/20001/Source/devtools/protocol...
Source/devtools/protocol.json:3651: { "name": "lineTicks", "type": "array",
"items": { "$ref": "LineTickInfo" }, "description": "An array of source line
ticks." }
On 2014/11/17 08:12:57, yurys wrote:
> Since we'd like to eventually have column info as well (for the purpose of
> profiling minified scripts) I'd rather name it something like positionTicks
and
> PositionTickInfo so that we don't have to rename it later.
thanks, done.

[Pan, 2014-11-17 08:50:27]

The CQ bit was checked by pan.deng@intel.com

[commit-bot: I haz the power, 2014-11-17 08:51:14]

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

[commit-bot: I haz the power, 2014-11-17 10:02:24]

Committed patchset #3 (id:40001) as 185428