1359144 - switch console to use client-side source map service

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Tom Tromey :tromey]

We want to switch the console to use the new client-side source map code.

Comment 1

[Tom Tromey :tromey]

A few notes on what I've learned so far:

The actual console changes are simple.
Most of the adaptation is in the Frame component.
However, the only place that we get a sourceMapURL is the source actor.
So, we need some kind of intermediary for this.

Comment 2

[Tom Tromey :tromey]

The Frame component is also used in the memory and performance tools, but they don't
pass in a source map service.  However they may still need updates if we change the
shape of location objects passed in to Frame.  (Not sure yet if this is the plan.)

Comment 3

[Tom Tromey :tromey]

There is no source actor if there is no source.
This can happen if the page calls console.log and then the source is GC'd.
So, for example, if you open the console after the page is loaded, we'll be unable to source map
some things.

One fix for this would be to fetch the original resource ourselves and parse it to get the
source map url.

Comment 4

[Tom Tromey :tromey]

Another idea is to change Console.webidl and Console.cpp to put the sourceMapURL into
the ConsoleStackEntry.  I'm going to investigate this a bit more because it seems promising.

Comment 5

[Tom Tromey :tromey]

The Console.webidl change seems doable, but somewhat complicated; and it affects memory use.
So I am going to defer it to a separate bug.

Comment 6

[Tom Tromey :tromey]

My patch does the mapping ok, but with webpack I see an original URL like "webpack:///./f2.js".
Clicking on this in the console doesn't do anything useful.

Comment 7

[Tom Tromey :tromey]

Probably what needs to happen is that we should have a special case when clicking on any source-mapped link.

Comment 8

[Tom Tromey :tromey]

devtools-source-map doesn't emit any sort of notification when applySourceMap is called.
Until this is done, the console will not notice when pretty-printing is done in the debugger.
I think I will handle this in a follow-up bug.

Comment 9

[Tom Tromey :tromey]

I filed it on github: https://github.com/devtools-html/devtools-core/issues/352

Comment 10

[Tom Tromey :tromey]

Attachment: Bug 1359144 - use client-side source map service in console;

Review commit: https://reviewboard.mozilla.org/r/135176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135176/

Comment 11

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ea285afa105d93675a8cdd2773ca2bd89e0207

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

https://reviewboard.mozilla.org/r/135176/#review138144

Overall, this looks reasonable to me!  I was curious about the naming issues, so I'd like to hear more about that before marking r+.

::: devtools/client/framework/source-map-url-service.js:80
(Diff revision 1)
> +  if (!urlInfo) {
> +    return null;
> +  }
> +  // Call getOriginalURLs to make sure the source map has been
> +  // fetched.  We don't actually need the result of this though.
> +  await this._sourceMapService.getOriginalURLs(urlInfo);

If something goes wrong (bad SM URL, invalid SM syntax, etc.), this will return `false`, perhaps that's useful to test for to catch the error case?

I was about to say you could add the first of many source map diagnostic logs here (to debug broken maps), but this is probably too high level?  At this spot, all you can probably say is "something is wrong!".  (Though, even that is probably better than silence!)

::: devtools/client/framework/toolbox.js:561
(Diff revision 1)
>  
> +  /**
> +   * Clients wishing to use source maps but that want the toolbox to
> +   * track the source actor mapping can use this source map service.
> +   */
> +  get sourceMapURLService() {

The overall design here seems reasonable (a service to track the URLs, and then call into other modules), but the naming does worry me a bit...  The difference between `sourceMapService` and `sourceMapURLService` doesn't seem very obvious at all.

But...  since we want to share `sourceMapService` with the debugger, we don't want to just drop it from the toolbox...

Any thoughts on better naming to reduce confusion here?

::: devtools/client/shared/components/test/mochitest/test_frame_01.html:290
(Diff revision 1)
>        line: "1",
>        shouldLink: true,
>        tooltip: "View source in Debugger → http://www.cnn.com/:1",
>      });
>  
> +    const resolvedLocation = {

Does the console have any other source map tests, or is the only one?  Not sure what the right level of test coverage is, since we eventually call down to mozilla/source-map which is fairly well tested, and the debugger also has some high level tests...  So maybe the console doesn't have to have an extensive suite of its own...

Comment 13

[Tom Tromey :tromey]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)

> I was about to say you could add the first of many source map diagnostic
> logs here (to debug broken maps), but this is probably too high level?

It's a good idea, but I'd rather defer the error-reporting issue to a separate bug,
I suppose bug 1345533.  Thinking about why I want to do this, I suppose partly
it's to make progress piecemeal, and also partly to attack the reporting problem
once I have a better summary view of the various users of the service.

> The overall design here seems reasonable (a service to track the URLs, and
> then call into other modules), but the naming does worry me a bit...  The
> difference between `sourceMapService` and `sourceMapURLService` doesn't seem
> very obvious at all.
> 
> But...  since we want to share `sourceMapService` with the debugger, we
> don't want to just drop it from the toolbox...
> 
> Any thoughts on better naming to reduce confusion here?

Yeah, this name is kind of terrible with the other service still around.  I don't
think my naming skills are all that great, but I did think about this a bit, and
couldn't find another obviously better name.  Then I talked myself into this name
because we're going to remove source-map-service in bug 1349354. 

> Does the console have any other source map tests, or is the only one?  Not
> sure what the right level of test coverage is, since we eventually call down
> to mozilla/source-map which is fairly well tested, and the debugger also has
> some high level tests...  So maybe the console doesn't have to have an
> extensive suite of its own...

This might be the only one.  I was just trying to make sure the new code was hit,
but that's pretty lame really.  I will see if I can come up with a bit more, and 
also it seems to me that I could unit test the url service.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tom Tromey :tromey from comment #13)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> > The overall design here seems reasonable (a service to track the URLs, and
> > then call into other modules), but the naming does worry me a bit...  The
> > difference between `sourceMapService` and `sourceMapURLService` doesn't seem
> > very obvious at all.
> > 
> > But...  since we want to share `sourceMapService` with the debugger, we
> > don't want to just drop it from the toolbox...
> > 
> > Any thoughts on better naming to reduce confusion here?
> 
> Yeah, this name is kind of terrible with the other service still around.  I
> don't
> think my naming skills are all that great, but I did think about this a bit,
> and
> couldn't find another obviously better name.  Then I talked myself into this
> name
> because we're going to remove source-map-service in bug 1349354. 

Okay, well, maybe we could at least add a few more comments around the toolbox getters for the two services to explain one is a lower level service used by new debugger vs. the higher level service used by the console?

Anyway, seems like you've thought it through, so I'll mark r+.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

https://reviewboard.mozilla.org/r/135176/#review138164

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135176/diff/1-2/

Comment 17

[Tom Tromey :tromey]

Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

Re-requesting review.  I added some comments and I adapted one of the
server-side tests to use the new service.  (The other server-side test
will have to wait for a later bug.)

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8863416 [details]
Bug 1359144 - use client-side source map service in console;

https://reviewboard.mozilla.org/r/135176/#review138648

Thanks, this looks good to me!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5a0c4a83b72
use client-side source map service in console; r=jryans

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5a0c4a83b72