Closed Bug 1384943 Opened 7 years ago Closed 7 years ago

devtools-source-map: update to 0.10.0

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(2 files)

Bug 1384943 - import bundles from devtools-source-map 0.10.0;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1384943 - add initial implementation of SourceMapServiceWrapper;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
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.
0.7.0 and 0.8.0 were already released, so morphing this to bump to 0.9.0.
Summary: devtools-source-map: update to 0.7.0 → devtools-source-map: update to 0.9.0
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
Attachment #8891428 - Flags: review?(bgrinstead) → review+
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.
Additionally I think another devtools-source-map bump will be needed to fix up the error
handling in _getSourceMap.
Summary: devtools-source-map: update to 0.9.0 → devtools-source-map: update to 0.10.0
This time, pushing to try before bumping the version upstream.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c3124e39577929e4160fa07f67b87a943a689c8
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).
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 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];
  }
});

```
Attachment #8892918 - Flags: review?(bgrinstead)
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 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`
Attachment #8892918 - Flags: review?(bgrinstead) → review+
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.
Pushed by ttromey@mozilla.com:
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: