[debugger] implement legacy debug event listeners via debug delegate.

[debugger] implement legacy debug event listeners via debug delegate.

R=jgruber@chromium.org
BUG=v8:5530
Review-Url: https://codereview.chromium.org/2682593003
Cr-Commit-Position: refs/heads/master@{#43059}
Committed: https://chromium.googlesource.com/v8/v8/+/d0d4189dc094b9d6d7750ddb3ad250f3518c2225

[Yang, 2017-02-07 12:34:42]

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

[commit-bot: I haz the power, 2017-02-07 12:34:44]

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-07 12:39:54]

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

[commit-bot: I haz the power, 2017-02-07 12:39:55]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/22148)

[Yang, 2017-02-08 08:27:28]

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

[commit-bot: I haz the power, 2017-02-08 08:27:34]

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

[Yang, 2017-02-08 08:29:48]

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

[Yang, 2017-02-08 08:29:48]

Please take a look.

[Yang, 2017-02-08 08:30:19]

Description was changed from

==========
[debug] implement legacy debug event listeners via debug delegate.

R=jgruber@chromium.org
BUG=v8:5530
==========

to

==========
[debugger] implement legacy debug event listeners via debug delegate.

R=jgruber@chromium.org
BUG=v8:5530
==========

[commit-bot: I haz the power, 2017-02-08 09:03:40]

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

[commit-bot: I haz the power, 2017-02-08 09:03:41]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Yang, 2017-02-08 09:50:52]

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

[commit-bot: I haz the power, 2017-02-08 09:51:03]

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-08 10:17:16]

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

[commit-bot: I haz the power, 2017-02-08 10:17:17]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-02-09 10:33:29]

👏 this is great, lgtm with comments below.

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:1782: if (it.done()) return;  // Do not trigger an event with
an empty stack.
Is there corresponding code in inspector that we can now remove?

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:1785: if (!debug_delegate_) return;
Would it make sense to move this to the very top of this function? The only
side-effect I see here is setting promise_debug_marker_symbol, but that's
supposed to denote whether an event was already triggered for that promise -
which doesn't happen in this case.

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:2343: Execution::Call(isolate_,
Handle<JSFunction>::cast(listener_), global,
Nit: the JSFunction cast is already done in the constructor.

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.h#newco...
src/debug/debug.h:509: Handle<Object> event_listener_data_;
Can we remove event_listener{,_data}_?

[Yang, 2017-02-09 10:57:17]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-02-09 10:57:17]

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/2682593003/#ps100001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-02-09 10:57:19]

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

[Yang, 2017-02-09 11:24:12]

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:1782: if (it.done()) return;  // Do not trigger an event with
an empty stack.
On 2017/02/09 10:33:28, jgruber wrote:
> Is there corresponding code in inspector that we can now remove?

As explained offline, this is not new. Alexey already cleaned up in the
inspector.

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:1785: if (!debug_delegate_) return;
On 2017/02/09 10:33:28, jgruber wrote:
> Would it make sense to move this to the very top of this function? The only
> side-effect I see here is setting promise_debug_marker_symbol, but that's
> supposed to denote whether an event was already triggered for that promise -
> which doesn't happen in this case.

Good idea. I did not move up above the side effect though to not change
semantics. Promise events are complicated as is :)

https://codereview.chromium.org/2682593003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:2343: Execution::Call(isolate_,
Handle<JSFunction>::cast(listener_), global,
On 2017/02/09 10:33:28, jgruber wrote:
> Nit: the JSFunction cast is already done in the constructor.

Done.

[commit-bot: I haz the power, 2017-02-09 11:31:52]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486637837100920,
"parent_rev": "1d3317ff05091937dc56bec83b2278184ee3abb3", "commit_rev":
"d0d4189dc094b9d6d7750ddb3ad250f3518c2225"}

[commit-bot: I haz the power, 2017-02-09 11:31:57]

Description was changed from

==========
[debugger] implement legacy debug event listeners via debug delegate.

R=jgruber@chromium.org
BUG=v8:5530
==========

to

==========
[debugger] implement legacy debug event listeners via debug delegate.

R=jgruber@chromium.org
BUG=v8:5530

Review-Url: https://codereview.chromium.org/2682593003
Cr-Commit-Position: refs/heads/master@{#43059}
Committed:
https://chromium.googlesource.com/v8/v8/+/d0d4189dc094b9d6d7750ddb3ad250f3518...
==========

[commit-bot: I haz the power, 2017-02-09 11:31:58]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/d0d4189dc094b9d6d7750ddb3ad250f3518...

[kozy, 2017-02-10 18:53:28]

Thanks!