1384943 - devtools-source-map: update to 0.10.0

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tom Tromey :tromey]

I plan to bump devtools-source-map to 0.7.0 soon, and once that is done,
land the new bundle in M-C via this bug.
The new bundle brings a few bug fixes that are needed for bug 1345533.

Comment 1

[Tom Tromey :tromey]

0.7.0 and 0.8.0 were already released, so morphing this to bump to 0.9.0.

Comment 2

[Tom Tromey :tromey]

Attachment: Bug 1384943 - import bundles from devtools-source-map 0.10.0;

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

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8891428 [details]
Bug 1384943 - import bundles from devtools-source-map 0.10.0;

https://reviewboard.mozilla.org/r/162586/#review167924

I assume a try push will pick up any obvious breakage here

Comment 4

[Tom Tromey :tromey]

Looks like I'll have to land part of the error-handling wrapper here,
because there's already a debugger test for an invalid source map.

Comment 5

[Tom Tromey :tromey]

Additionally I think another devtools-source-map bump will be needed to fix up the error
handling in _getSourceMap.

Comment 6

[Tom Tromey :tromey]

Step 1 is https://github.com/devtools-html/devtools-core/pull/537

Comment 7

[Tom Tromey :tromey]

This time, pushing to try before bumping the version upstream.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3124e39577929e4160fa07f67b87a943a689c8

Comment 8

[Tom Tromey :tromey]

This patch seems to make devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-05.js
fail more often (it's already intermittent).  I haven't managed to reproduce that locally yet,
but I'm going to investigate before putting the new patches up for review (or before sending
the PR to bump to 0.10.0).

Comment 9

[Tom Tromey :tromey]

I rebased and now I can't reproduce the intermittent:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb41602328840efce626da0c2c72f434f818d975

Comment 10

[Tom Tromey :tromey]

version bump PR is here: https://github.com/devtools-html/devtools-core/pull/539

Comment 11

[Tom Tromey :tromey]

Ok, I'm going to try this again, this time with a simple error-reporting wrapper.
I'm not sure I wrote the wrapper in the most idiomatic way; suggestions welcome.
For the time being the wrapper just uses console.error -- keeping reporting at the status quo.
This will be changed in bug 1345533.

Comment 12

[Tom Tromey :tromey]

Attachment: Bug 1384943 - add initial implementation of SourceMapServiceWrapper;

This adds a simple wrapper for the source map service.  This wrapper
logs errors using console.error, preserving the status quo from before
this series.

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

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8891428 [details]
Bug 1384943 - import bundles from devtools-source-map 0.10.0;

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

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8892918 [details]
Bug 1384943 - add initial implementation of SourceMapServiceWrapper;

https://reviewboard.mozilla.org/r/163940/#review169412

::: devtools/client/framework/toolbox.js:89
(Diff revision 1)
> + * @param {Toolbox} toolbox
> + *        The toolbox where errors should be reported.
> + */
> +function SourceMapServiceWrapper(service, toolbox) {
> +  this._service = service;
> +  this._toolbox = toolbox;

`this._toolbox` appears to be unused

::: devtools/client/framework/toolbox.js:92
(Diff revision 1)
> +function SourceMapServiceWrapper(service, toolbox) {
> +  this._service = service;
> +  this._toolbox = toolbox;
> +
> +  // Forward these methods to the service.
> +  for (let method of ["originalToGeneratedId", "generatedToOriginalId", "isGeneratedId",

This is fine and if you prefer it I'm OK to land it this way, but I think I'd do something like this instead:

```
// A wrapper for the source map service that reports errors in a nicer way.
let service = this.browserRequire("devtools/client/shared/source-map/index");
this._sourceMapService = new Proxy(service, {
  get: (target, name) => {
    if (name === 'getOriginalURLs') {
      return function(urlInfo) {
        return target.getOriginalURLs(urlInfo).catch(console.error);
      }
    }
    return target[name];
  }
});

```

Comment 15

[Tom Tromey :tromey]

Comment on attachment 8892918 [details]
Bug 1384943 - add initial implementation of SourceMapServiceWrapper;

https://reviewboard.mozilla.org/r/163940/#review169790

::: devtools/client/framework/toolbox.js:89
(Diff revision 1)
> + * @param {Toolbox} toolbox
> + *        The toolbox where errors should be reported.
> + */
> +function SourceMapServiceWrapper(service, toolbox) {
> +  this._service = service;
> +  this._toolbox = toolbox;

Yeah, it's used in a future patch, which I should have explained.  But it probably won't matter if I change the implementation approach.

::: devtools/client/framework/toolbox.js:92
(Diff revision 1)
> +function SourceMapServiceWrapper(service, toolbox) {
> +  this._service = service;
> +  this._toolbox = toolbox;
> +
> +  // Forward these methods to the service.
> +  for (let method of ["originalToGeneratedId", "generatedToOriginalId", "isGeneratedId",

I was thinking either that or using getOwnProperties.  For some reason I thought we tried to avoid Proxy but I don't know why - looks reasonable to me.

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8892918 [details]
Bug 1384943 - add initial implementation of SourceMapServiceWrapper;

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

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8892918 [details]
Bug 1384943 - add initial implementation of SourceMapServiceWrapper;

https://reviewboard.mozilla.org/r/163940/#review169796

I assume there are existing tests for `this._sourceMapService`

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8892918 [details]
Bug 1384943 - add initial implementation of SourceMapServiceWrapper;

https://reviewboard.mozilla.org/r/163940/#review169796

Yes, there are.  And more to come in the next patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbe9ba9aafde
import bundles from devtools-source-map 0.10.0; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/8ecab1d291bf
add initial implementation of SourceMapServiceWrapper; r=bgrins

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dbe9ba9aafde
https://hg.mozilla.org/mozilla-central/rev/8ecab1d291bf