[inspector] merged type and name of async task event

[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name for other events.

BUG=v8:5738
R=jgruber@chromium.org,gsathya@chromium.org
TBR=yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2628173005
Cr-Commit-Position: refs/heads/master@{#42339}
Committed: https://chromium.googlesource.com/v8/v8/+/154cb8542ae1c74122299f59454b04ad1d9354bb

[kozy, 2017-01-13 16:38:18]

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

[commit-bot: I haz the power, 2017-01-13 16:38:27]

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

[kozy, 2017-01-13 16:38:34]

Jacob and Yang, please take a look.

[kozy, 2017-01-13 16:42:28]

Description was changed from

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=yangguo@chromium.org,jgruber@chromium.org
==========

to

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=yangguo@chromium.org,jgruber@chromium.org
==========

[kozy, 2017-01-13 16:42:28]

kozyatinskiy@chromium.org changed reviewers:
+ gsathya@chromium.org

[jgruber, 2017-01-13 16:42:34]

Adding sathya since he did the recent promise builtin work.

[kozy, 2017-01-13 16:42:37]

Description was changed from

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=yangguo@chromium.org,jgruber@chromium.org
==========

to

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=yangguo@chromium.org,jgruber@chromium.org,gsathya@chromium.org
==========

[commit-bot: I haz the power, 2017-01-13 17:19:58]

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

[commit-bot: I haz the power, 2017-01-13 17:19:58]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-13 17:23:41]

very nice cleanup! thanks

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc
File src/isolate.cc (left):

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc#oldcode3288
src/isolate.cc:3288: bool is_debug_active_;
This is going to cause a mismatch if debugger is opened after kDebugWillHandle
is fired and before kDebugDidHandle is fired

[jgruber, 2017-01-13 17:29:06]

lgtm with comments, nice cleanup!

https://codereview.chromium.org/2628173005/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2628173005/diff/1/src/debug/debug.cc#newcode1873
src/debug/debug.cc:1873: handle(Smi::FromInt(id), isolate_))
Nit: Passing Smi handles around doesn't make sense to me.. We could pass in raw
ints and create the handles when creating the args array. But up to you since
this was already here before.

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc#newcode3275
src/isolate.cc:3275: if (isolate_->debug()->is_active() && id_ !=
kDebugPromiseNoID) {
Not 100% sure this change is valid. What if is_active() changes during the
PromiseDebugEventScope lifetime?

https://codereview.chromium.org/2628173005/diff/1/src/js/macros.py
File src/js/macros.py (left):

https://codereview.chromium.org/2628173005/diff/1/src/js/macros.py#oldcode200
src/js/macros.py:200: # [[PromiseState]] values:
Seems like we can remove these 3 as well.

[kozy, 2017-01-13 17:41:58]

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

[commit-bot: I haz the power, 2017-01-13 17:42:06]

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

[kozy, 2017-01-13 17:42:14]

addressed comments, please take another look.

https://codereview.chromium.org/2628173005/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2628173005/diff/1/src/debug/debug.cc#newcode1873
src/debug/debug.cc:1873: handle(Smi::FromInt(id), isolate_))
On 2017/01/13 17:29:06, jgruber wrote:
> Nit: Passing Smi handles around doesn't make sense to me.. We could pass in
raw
> ints and create the handles when creating the args array. But up to you since
> this was already here before.

I hope that we'll remove this code completely in future, so let's left it as is.

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc
File src/isolate.cc (left):

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc#oldcode3288
src/isolate.cc:3288: bool is_debug_active_;
On 2017/01/13 17:23:40, gsathya wrote:
> This is going to cause a mismatch if debugger is opened after kDebugWillHandle
> is fired and before kDebugDidHandle is fired

If debugger is opened after kDebugWillHandle then id_ would be kDebugPromiseNoId
and we won't report kDebugWillHandle.
In following scenario:
- debugger enabled
- promise enqueued (with id_ != kDebugPromiseNoId)
- debugger disabled (inspector will cleanup everything and just ignore events
related to enqueued promise)
- willHandle happens - debugger is not active.
- debugger enabled
- didHandle happens - would be reported to inspector, but inspector will filter
it out and it's probably fine.

And I'm not sure how we can avoid sending this event in this case right here -
we only can cleanup id_ field for all Promise*JobInfo on debugger disable but it
looks to much complicated.

Or I just don't understand something :) Could you describe issue in more
details?

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc#newcode3275
src/isolate.cc:3275: if (isolate_->debug()->is_active() && id_ !=
kDebugPromiseNoID) {
On 2017/01/13 17:29:06, jgruber wrote:
> Not 100% sure this change is valid. What if is_active() changes during the
> PromiseDebugEventScope lifetime?

Inspector is ready for "invalid" events. And we can trust inspector in this
aspect instead of introducing complicated logic for each asyncTaskEvent* clients
(e.g. blink setTimeout).

https://codereview.chromium.org/2628173005/diff/1/src/js/macros.py
File src/js/macros.py (left):

https://codereview.chromium.org/2628173005/diff/1/src/js/macros.py#oldcode200
src/js/macros.py:200: # [[PromiseState]] values:
On 2017/01/13 17:29:06, jgruber wrote:
> Seems like we can remove these 3 as well.

looks so, removed.

[commit-bot: I haz the power, 2017-01-13 18:13:50]

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

[commit-bot: I haz the power, 2017-01-13 18:13:50]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-13 18:20:35]

lgtm

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc
File src/isolate.cc (left):

https://codereview.chromium.org/2628173005/diff/1/src/isolate.cc#oldcode3288
src/isolate.cc:3288: bool is_debug_active_;
On 2017/01/13 17:42:13, kozyatinskiy wrote:
> On 2017/01/13 17:23:40, gsathya wrote:
> > This is going to cause a mismatch if debugger is opened after
kDebugWillHandle
> > is fired and before kDebugDidHandle is fired
> 
> If debugger is opened after kDebugWillHandle then id_ would be
kDebugPromiseNoId
> and we won't report kDebugWillHandle.
> In following scenario:
> - debugger enabled
> - promise enqueued (with id_ != kDebugPromiseNoId)
> - debugger disabled (inspector will cleanup everything and just ignore events
> related to enqueued promise)
> - willHandle happens - debugger is not active.
> - debugger enabled
> - didHandle happens - would be reported to inspector, but inspector will
filter
> it out and it's probably fine.
> 
> And I'm not sure how we can avoid sending this event in this case right here -
> we only can cleanup id_ field for all Promise*JobInfo on debugger disable but
it
> looks to much complicated.
> 
> Or I just don't understand something :) Could you describe issue in more
> details?

sgtm if inspector will filter it out, i was mostly worried about that.

[kozy, 2017-01-13 18:23:41]

Description was changed from

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=yangguo@chromium.org,jgruber@chromium.org,gsathya@chromium.org
==========

to

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=jgruber@chromium.org,gsathya@chromium.org
TBR=yangguo@chromium.org
==========

[kozy, 2017-01-13 19:11:11]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-01-13 19:11:11]

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

[commit-bot: I haz the power, 2017-01-13 19:11:26]

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-01-13 19:13:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484334671698650,
"parent_rev": "b90822950004553ecaef48ea04977c8f14bc013e", "commit_rev":
"154cb8542ae1c74122299f59454b04ad1d9354bb"}

[commit-bot: I haz the power, 2017-01-13 19:13:48]

Description was changed from

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=jgruber@chromium.org,gsathya@chromium.org
TBR=yangguo@chromium.org
==========

to

==========
[inspector] merged type and name of async task event

Inspector uses event name only for enqueue* events and doesn't really need name
for other events.

BUG=v8:5738
R=jgruber@chromium.org,gsathya@chromium.org
TBR=yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2628173005
Cr-Commit-Position: refs/heads/master@{#42339}
Committed:
https://chromium.googlesource.com/v8/v8/+/154cb8542ae1c74122299f59454b04ad1d9...
==========

[commit-bot: I haz the power, 2017-01-13 19:13:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/154cb8542ae1c74122299f59454b04ad1d9...