[inspector] add async instrumentation for setTimeout in tests

[inspector] add async instrumentation for setTimeout in tests

BUG=v8:5738
R=dgozman@chromium.org

Committed: https://crrev.com/73ac1d3877a263b021c1d9989e19ffa86a63978b
Cr-Commit-Position: refs/heads/master@{#41680}

[kozy, 2016-12-13 17:18:43]

Dmitry, please take a look.

[dgozman, 2016-12-13 17:31:54]

https://codereview.chromium.org/2574803002/diff/1/test/inspector/debugger/asy...
File test/inspector/debugger/async-set-timeout.js (right):

https://codereview.chromium.org/2574803002/diff/1/test/inspector/debugger/asy...
test/inspector/debugger/async-set-timeout.js:26: //# sourceURL=test.js`, 7, 26)
missing semicolon

https://codereview.chromium.org/2574803002/diff/1/test/inspector/debugger/asy...
test/inspector/debugger/async-set-timeout.js:32:
urlByScriptId.set(message.params.scriptId, message.params.url);
Let's have a helper for this, something like InspectorTest.SetUpScriptUrlMap.

https://codereview.chromium.org/2574803002/diff/1/test/inspector/inspector-te...
File test/inspector/inspector-test.cc (right):

https://codereview.chromium.org/2574803002/diff/1/test/inspector/inspector-te...
test/inspector/inspector-test.cc:256: inspector->asyncTaskScheduled(
Why doesn't ExecuteStringTask do this in constructor if last parameter is true?
We can change that to be "char* asyncTaskName".

You can also inherit both SetTimeoutTask and ExecuteStringTask from AsyncTask if
that would make it easier.

[kozy, 2016-12-13 18:24:26]

all done, please take another look.

https://codereview.chromium.org/2574803002/diff/1/test/inspector/debugger/asy...
File test/inspector/debugger/async-set-timeout.js (right):

https://codereview.chromium.org/2574803002/diff/1/test/inspector/debugger/asy...
test/inspector/debugger/async-set-timeout.js:26: //# sourceURL=test.js`, 7, 26)
On 2016/12/13 17:31:54, dgozman wrote:
> missing semicolon

Done.

https://codereview.chromium.org/2574803002/diff/1/test/inspector/debugger/asy...
test/inspector/debugger/async-set-timeout.js:32:
urlByScriptId.set(message.params.scriptId, message.params.url);
On 2016/12/13 17:31:54, dgozman wrote:
> Let's have a helper for this, something like InspectorTest.SetUpScriptUrlMap.

Done.

https://codereview.chromium.org/2574803002/diff/1/test/inspector/inspector-te...
File test/inspector/inspector-test.cc (right):

https://codereview.chromium.org/2574803002/diff/1/test/inspector/inspector-te...
test/inspector/inspector-test.cc:256: inspector->asyncTaskScheduled(
On 2016/12/13 17:31:54, dgozman wrote:
> Why doesn't ExecuteStringTask do this in constructor if last parameter is
true?
> We can change that to be "char* asyncTaskName".
> 
> You can also inherit both SetTimeoutTask and ExecuteStringTask from AsyncTask
if
> that would make it easier.

Done.

[dgozman, 2016-12-13 18:33:25]

lgtm

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/async-set-timeout.js (right):

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/debugger...
test/inspector/debugger/async-set-timeout.js:32: var urlByScriptId = new Map();
Unused.

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/debugger...
test/inspector/debugger/async-set-timeout.js:35:
InspectorTest.logCallFrames(message.params.callFrames, urlByScriptId);
Remove second parameter.

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/task-run...
File test/inspector/task-runner.h (right):

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/task-run...
test/inspector/task-runner.h:72: AsyncTask(const char* task_name,
v8_inspector::V8Inspector* inspector);
Looks strange now to pass both inspector here and context in Run, since they
must match.
Not sure how to make this better though.

[kozy, 2016-12-13 19:15:27]

thanks!

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/async-set-timeout.js (right):

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/debugger...
test/inspector/debugger/async-set-timeout.js:32: var urlByScriptId = new Map();
On 2016/12/13 18:33:24, dgozman wrote:
> Unused.

Done.

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/debugger...
test/inspector/debugger/async-set-timeout.js:35:
InspectorTest.logCallFrames(message.params.callFrames, urlByScriptId);
On 2016/12/13 18:33:24, dgozman wrote:
> Remove second parameter.

Done.

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/task-run...
File test/inspector/task-runner.h (right):

https://codereview.chromium.org/2574803002/diff/40001/test/inspector/task-run...
test/inspector/task-runner.h:72: AsyncTask(const char* task_name,
v8_inspector::V8Inspector* inspector);
On 2016/12/13 18:33:24, dgozman wrote:
> Looks strange now to pass both inspector here and context in Run, since they
> must match.
> Not sure how to make this better though.

Agreed.

[kozy, 2016-12-13 19:15:38]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-12-13 19:15:39]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2574803002/#ps80001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-12-13 19:15:40]

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, 2016-12-13 19:40:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481656538382480,
"parent_rev": "8e943187e984799ab64cdbde8f6e2a4d4bb450ef", "commit_rev":
"e473f79360047c935da170782aadb428b363c433"}

[commit-bot: I haz the power, 2016-12-13 19:40:21]

Description was changed from

==========
[inspector] add async instrumentation for setTimeout in tests

BUG=v8:5738
R=dgozman@chromium.org
==========

to

==========
[inspector] add async instrumentation for setTimeout in tests

BUG=v8:5738
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2574803002
==========

[commit-bot: I haz the power, 2016-12-13 19:40:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-13 19:41:31]

Description was changed from

==========
[inspector] add async instrumentation for setTimeout in tests

BUG=v8:5738
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2574803002
==========

to

==========
[inspector] add async instrumentation for setTimeout in tests

BUG=v8:5738
R=dgozman@chromium.org

Committed: https://crrev.com/73ac1d3877a263b021c1d9989e19ffa86a63978b
Cr-Commit-Position: refs/heads/master@{#41680}
==========

[commit-bot: I haz the power, 2016-12-13 19:41:32]

Patchset 5 (id:??) landed as
https://crrev.com/73ac1d3877a263b021c1d9989e19ffa86a63978b
Cr-Commit-Position: refs/heads/master@{#41680}