[inspector] added Debugger.moduleRequested notification

[inspector] added Debugger.moduleRequested notification

This notification is fired when module requests another nodule during instantiation.
This event is needed by DevTools frontend as minimal information exposed by protocol to implement: "Go to import", module dependency graph and to help users with understanding import errors.

One-pager: https://docs.google.com/document/d/1wfMHzSv6mr0-NdauFfhR65ebH821_HJCkERJKb1ouUc/edit#

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org

[kozy, 2017-01-31 01:57:57]

Description was changed from

==========
[inspector] added Debugger.moduleResolved notification

This notification is fired when one es6 module successfully requested another.

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

to

==========
[inspector] added Debugger.moduleResolved notification

It's PoC.
This notification is fired when one es6 module successfully requested another.

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

[kozy, 2017-01-31 01:58:24]

FYI: PoC how we can implement moduleResolved notification. I'll write in details
about other options in document.

[Yang, 2017-01-31 07:53:00]

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

[Yang, 2017-01-31 07:53:01]

https://codereview.chromium.org/2668613002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2668613002/diff/20001/src/objects.h#newcode6749
src/objects.h:6749: DECL_ACCESSORS(module, Object)
This field is so we can iterate the script list to find all modules, for
ReportExistingModules, right? This can also be accomplished by simply iterating
the heap to find module objects. This is what we used to do to find all scripts,
before we introduced the script list for other reasons. This is what I prefer,
if we only have to do this once, when debugging.

https://codereview.chromium.org/2668613002/diff/20001/src/objects.h#newcode7929
src/objects.h:7929: DECL_ACCESSORS(script, Object)
Can't we get the script through module->code()->script()?

[adamk, 2017-01-31 18:15:13]

adamk@chromium.org changed reviewers:
+ neis@chromium.org

[adamk, 2017-01-31 18:15:14]

+neis

[kozy, 2017-01-31 22:12:42]

Description was changed from

==========
[inspector] added Debugger.moduleResolved notification

It's PoC.
This notification is fired when one es6 module successfully requested another.

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

to

==========
[inspector] added Debugger.moduleResolved notification

This notification is fired when one es module successfully requested another.

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

[kozy, 2017-01-31 22:14:47]

addressed comments and ready for review.

https://codereview.chromium.org/2668613002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2668613002/diff/20001/src/objects.h#newcode6749
src/objects.h:6749: DECL_ACCESSORS(module, Object)
On 2017/01/31 07:53:00, Yang wrote:
> This field is so we can iterate the script list to find all modules, for
> ReportExistingModules, right? This can also be accomplished by simply
iterating
> the heap to find module objects. This is what we used to do to find all
scripts,
> before we introduced the script list for other reasons. This is what I prefer,
> if we only have to do this once, when debugging.

Done.

https://codereview.chromium.org/2668613002/diff/20001/src/objects.h#newcode7929
src/objects.h:7929: DECL_ACCESSORS(script, Object)
On 2017/01/31 07:53:00, Yang wrote:
> Can't we get the script through module->code()->script()?

We couldn't use it for reporting existing module, since this field can contain
ModuleInfo after evaluation instead of SFI or JSFunction.

[neis, 2017-02-01 13:38:56]

Hi, some comments below.

Could you say a few words on the motivation for this feature?

THanks.

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug-interfa...
src/debug/debug-interface.h:149: void ReportExistingModules(Isolate* isolate);
Please add a comment describing what this does, as it is not what the function
name suggests.  It seems to be reporting the module requests of modules that
have already been evaluated. (I'm not sure why this is the desired semantics.)

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug.cc#newc...
src/debug/debug.cc:1901: return;
Should this ever happen? See comment in objects.h.

https://codereview.chromium.org/2668613002/diff/40001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2668613002/diff/40001/src/inspector/js_protoc...
src/inspector/js_protocol.json:726: "description": "Fired when one module
successfully import another."
s/import/imports/

I find this description a little unclear. "Successfully" imported to me would
imply e.g. that all the variables that the importing module requests from the
imported module can be resolved, but you're calling the handler before that is
determined (in fact even before the imported module itself has been
instantiated).

https://codereview.chromium.org/2668613002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2668613002/diff/40001/src/objects.h#newcode7919
src/objects.h:7919: 
Shouldn't this always be a Script?

[kozy, 2017-02-01 16:37:28]

Description was changed from

==========
[inspector] added Debugger.moduleResolved notification

This notification is fired when one es module successfully requested another.

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

to

==========
[inspector] added Debugger.moduleResolved notification

This notification is fired when one es module successfully requested another.

One-pager:
https://docs.google.com/document/d/1wfMHzSv6mr0-NdauFfhR65ebH821_HJCkERJKb1ou...

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

[kozy, 2017-02-01 16:38:56]

Description was changed from

==========
[inspector] added Debugger.moduleResolved notification

This notification is fired when one es module successfully requested another.

One-pager:
https://docs.google.com/document/d/1wfMHzSv6mr0-NdauFfhR65ebH821_HJCkERJKb1ou...

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

to

==========
[inspector] added Debugger.moduleRequested notification

This notification is fired when module requests another nodule during
instantiation.
This event is needed by DevTools frontend as minimal information exposed by
protocol to implement: "Go to import", module dependency graph and to help users
with understanding import errors.

One-pager:
https://docs.google.com/document/d/1wfMHzSv6mr0-NdauFfhR65ebH821_HJCkERJKb1ou...

BUG=v8:1569
R=dgozman@chromium.org,adamk@chromium.org
==========

[kozy, 2017-02-01 16:39:25]

Addressed all comments, added better description.
Please take another look.

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug-interfa...
src/debug/debug-interface.h:149: void ReportExistingModules(Isolate* isolate);
On 2017/02/01 13:38:55, neis wrote:
> Please add a comment describing what this does, as it is not what the function
> name suggests.  It seems to be reporting the module requests of modules that
> have already been evaluated. (I'm not sure why this is the desired semantics.)

Done.

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2668613002/diff/40001/src/debug/debug.cc#newc...
src/debug/debug.cc:1901: return;
On 2017/02/01 13:38:55, neis wrote:
> Should this ever happen? See comment in objects.h.

Done.

https://codereview.chromium.org/2668613002/diff/40001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2668613002/diff/40001/src/inspector/js_protoc...
src/inspector/js_protocol.json:726: "description": "Fired when one module
successfully import another."
On 2017/02/01 13:38:55, neis wrote:
> s/import/imports/
> 
> I find this description a little unclear. "Successfully" imported to me would
> imply e.g. that all the variables that the importing module requests from the
> imported module can be resolved, but you're calling the handler before that is
> determined (in fact even before the imported module itself has been
> instantiated).

I renamed this notification to moduleRequested. We mostly need it to implement
go to import, dependency graph and to help users with understanding of import
errors.
Could you suggest better instrumentation point to get enough information as
earlier as possible?

https://codereview.chromium.org/2668613002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2668613002/diff/40001/src/objects.h#newcode7919
src/objects.h:7919: 
On 2017/02/01 13:38:55, neis wrote:
> Shouldn't this always be a Script?

Done.

[neis, 2017-02-06 09:20:33]

Thanks, lgtm.

https://codereview.chromium.org/2668613002/diff/40001/src/inspector/js_protoc...
File src/inspector/js_protocol.json (right):

https://codereview.chromium.org/2668613002/diff/40001/src/inspector/js_protoc...
src/inspector/js_protocol.json:726: "description": "Fired when one module
successfully import another."
On 2017/02/01 16:39:25, kozy wrote:
> On 2017/02/01 13:38:55, neis wrote:
> > s/import/imports/
> > 
> > I find this description a little unclear. "Successfully" imported to me
would
> > imply e.g. that all the variables that the importing module requests from
the
> > imported module can be resolved, but you're calling the handler before that
is
> > determined (in fact even before the imported module itself has been
> > instantiated).
> 
> I renamed this notification to moduleRequested. We mostly need it to implement
> go to import, dependency graph and to help users with understanding of import
> errors.
> Could you suggest better instrumentation point to get enough information as
> earlier as possible?

If you want to get information as early as possible, this seems like a
reasonable place.

[dgozman, 2017-07-15 05:44:36]

dgozman@chromium.org changed reviewers:
- dgozman@chromium.org