Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622253004:
[inspector] introduced debug::SetBreakEventListener,SetExceptionEventListener (Closed)
Description[inspector] introduced debug::SetBreakEventListener,SetExceptionEventListener
Inspector is moved to per-event-type callbacks instead of general v8::debug::SetDebugEventListener. It allows to:
- remove any usage of v8::Debug::EventDetails in debug-interface,
- avoid redundant JS call on each event to get properties of event objects,
- introduce better pure C++ API for these events later.
BUG=v8:5510
R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org
Review-Url: https://codereview.chromium.org/2622253004
Cr-Commit-Position: refs/heads/master@{#42483}
Committed: https://chromium.googlesource.com/v8/v8/+/f3dcdf886213b9bb36fe40fbde85d4fca6587fb7
Patch Set 1 #Patch Set 2 : ready for review #
Total comments: 1
Patch Set 3 : addressed comments #Patch Set 4 : fixed compilation #
Total comments: 10
Patch Set 5 : addressed comments #
Total comments: 1
Depends on Patchset: Messages
Total messages: 45 (25 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [inspector] introduced debug::SetBreakEventListener,SetExceptionEventListener With new two methods we can get rid of general SetDebugEventListener in inspector codebase - it moves us closer to removing debugger context support. New callbacks allow futher migrating to pure C++ API. BUG= R+ ========== to ========== [inspector] introduced debug::SetBreakEventListener,SetExceptionEventListener Inspector is moved to per-event-type callbacks instead of general v8::debug::SetDebugEventListener. It allows to: - remove any usage of v8::Debug::EventDetails in debug-interface, - avoid redundant JS call on each event to get properties of event objects, - introduce better pure C++ API for these events later. BUG=v8:5510 R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org ==========
kozyatinskiy@chromium.org changed reviewers: + dgozman@chromium.org, jgruber@chromium.org, yangguo@chromium.org
Please take a look!
On 2017/01/16 22:53:40, kozyatinskiy wrote:
> Please take a look!
A general idea now that we have several such callbacks: what if we use a
listener class instead?
class DebugListener {
... OnBreakEvent(event-specific args) ...
... OnExceptionEvent(...) ...
...
}
Might be cleaner as users only need to register a single listener and can then
decide what events
to care about themselves. We could simplify the ( ... || break listener ||
exception listener || ... )
handling we currently have at a few spots. Also, we could omit the void* data
args that are currently
needed with function callbacks.
WDYT?
lgtm + comment above https://codereview.chromium.org/2622253004/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2622253004/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:97: v8::debug::SetBreakEventListener(m_isolate, nullptr, nullptr); Double copy-and-paste
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/17 08:21:09, jgruber wrote:
> On 2017/01/16 22:53:40, kozyatinskiy wrote:
> > Please take a look!
>
> A general idea now that we have several such callbacks: what if we use a
> listener class instead?
>
> class DebugListener {
> ... OnBreakEvent(event-specific args) ...
> ... OnExceptionEvent(...) ...
> ...
> }
>
> Might be cleaner as users only need to register a single listener and can then
> decide what events
> to care about themselves. We could simplify the ( ... || break listener ||
> exception listener || ... )
> handling we currently have at a few spots. Also, we could omit the void* data
> args that are currently
> needed with function callbacks.
>
> WDYT?
I like this idea, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
👍 nice! lgtm.
inspector lgtm. Keep it up! https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:153: virtual void OnAsyncTaskEvent(debug::PromiseDebugActionType type, int id) {} PromiseEventOccurred https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:154: virtual void OnCompileEvent(v8::Local<Script> script, ScriptCompiled https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:156: virtual void OnBreakEvent(v8::Local<v8::Context> paused_context, BreakProgramRequested https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:159: virtual void OnExceptionEvent(v8::Local<v8::Context> paused_context, ExceptionThrown https://codereview.chromium.org/2622253004/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger.h (right): https://codereview.chromium.org/2622253004/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger.h:98: void OnAsyncTaskEvent(v8::debug::PromiseDebugActionType type, // v8::debug::DebugEventListener implementation. You can also move them to private.
thanks! all done. https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:153: virtual void OnAsyncTaskEvent(debug::PromiseDebugActionType type, int id) {} On 2017/01/18 18:43:05, dgozman wrote: > PromiseEventOccurred Done. https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:154: virtual void OnCompileEvent(v8::Local<Script> script, On 2017/01/18 18:43:05, dgozman wrote: > ScriptCompiled Done. https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:156: virtual void OnBreakEvent(v8::Local<v8::Context> paused_context, On 2017/01/18 18:43:05, dgozman wrote: > BreakProgramRequested Done. https://codereview.chromium.org/2622253004/diff/100001/src/debug/debug-interf... src/debug/debug-interface.h:159: virtual void OnExceptionEvent(v8::Local<v8::Context> paused_context, On 2017/01/18 18:43:05, dgozman wrote: > ExceptionThrown Done. https://codereview.chromium.org/2622253004/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger.h (right): https://codereview.chromium.org/2622253004/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger.h:98: void OnAsyncTaskEvent(v8::debug::PromiseDebugActionType type, On 2017/01/18 18:43:05, dgozman wrote: > // v8::debug::DebugEventListener implementation. > > You can also move them to private. Done.
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kozyatinskiy@chromium.org
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2622253004/#ps120001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19553) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19555) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19557) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484781301459170,
"parent_rev": "ea4f834c7ef645e4f71e3636033a221d9bd1768a", "commit_rev":
"f3dcdf886213b9bb36fe40fbde85d4fca6587fb7"}
Message was sent while issue was closed.
Description was changed from ========== [inspector] introduced debug::SetBreakEventListener,SetExceptionEventListener Inspector is moved to per-event-type callbacks instead of general v8::debug::SetDebugEventListener. It allows to: - remove any usage of v8::Debug::EventDetails in debug-interface, - avoid redundant JS call on each event to get properties of event objects, - introduce better pure C++ API for these events later. BUG=v8:5510 R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org ========== to ========== [inspector] introduced debug::SetBreakEventListener,SetExceptionEventListener Inspector is moved to per-event-type callbacks instead of general v8::debug::SetDebugEventListener. It allows to: - remove any usage of v8::Debug::EventDetails in debug-interface, - avoid redundant JS call on each event to get properties of event objects, - introduce better pure C++ API for these events later. BUG=v8:5510 R=yangguo@chromium.org,jgruber@chromium.org,dgozman@chromium.org Review-Url: https://codereview.chromium.org/2622253004 Cr-Commit-Position: refs/heads/master@{#42483} Committed: https://chromium.googlesource.com/v8/v8/+/f3dcdf886213b9bb36fe40fbde85d4fca65... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/v8/v8/+/f3dcdf886213b9bb36fe40fbde85d4fca65...
Message was sent while issue was closed.
https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc#new... src/debug/debug.cc:1779: if (!non_inspector_listener_exists()) return; Let's implement the non-inspector listener through this new API?
Message was sent while issue was closed.
On 2017/01/19 08:55:49, Yang wrote: > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc > File src/debug/debug.cc (right): > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc#new... > src/debug/debug.cc:1779: if (!non_inspector_listener_exists()) return; > Let's implement the non-inspector listener through this new API? I'll implement it on next week.
Message was sent while issue was closed.
On 2017/01/20 06:03:02, kozyatinskiy wrote: > On 2017/01/19 08:55:49, Yang wrote: > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc > > File src/debug/debug.cc (right): > > > > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc#new... > > src/debug/debug.cc:1779: if (!non_inspector_listener_exists()) return; > > Let's implement the non-inspector listener through this new API? > > I'll implement it on next week. Very cool. Thanks.
Message was sent while issue was closed.
On 2017/01/20 06:21:13, Yang wrote: > On 2017/01/20 06:03:02, kozyatinskiy wrote: > > On 2017/01/19 08:55:49, Yang wrote: > > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc > > > File src/debug/debug.cc (right): > > > > > > > > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc#new... > > > src/debug/debug.cc:1779: if (!non_inspector_listener_exists()) return; > > > Let's implement the non-inspector listener through this new API? > > > > I'll implement it on next week. > > Very cool. Thanks. One question, would we like to still support v8::Debug::SetDebugEventListener in v8-debug.h for C++ native callbacks? Looks like currently neither inspector nor chromium nor Node.js use it.
Message was sent while issue was closed.
On 2017/01/22 00:31:45, kozyatinskiy wrote: > On 2017/01/20 06:21:13, Yang wrote: > > On 2017/01/20 06:03:02, kozyatinskiy wrote: > > > On 2017/01/19 08:55:49, Yang wrote: > > > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc > > > > File src/debug/debug.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2622253004/diff/120001/src/debug/debug.cc#new... > > > > src/debug/debug.cc:1779: if (!non_inspector_listener_exists()) return; > > > > Let's implement the non-inspector listener through this new API? > > > > > > I'll implement it on next week. > > > > Very cool. Thanks. > > One question, would we like to still support v8::Debug::SetDebugEventListener in > v8-debug.h for C++ native callbacks? Looks like currently neither inspector nor > chromium nor Node.js use it. our debug tests still do. Let's keep it until I adapt these tests |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
