Closed Bug 1349360 Opened 7 years ago Closed 7 years ago

Expose client-side source map service in toolbox

Categories

(DevTools :: General, enhancement, P1)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1349360 - Add source-map to browser-based list.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1349360 - Mark experimental SourceMapService as deprecated.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1349360 - Expose client-side source map service on toolbox.
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
We want to expose the `devtools-source-map` package (using client side mapping) for use by various panels.
At the moment, actually trying to get the service will fail with:

SyntaxError: Failed to load worker script at "undefined"

I need a separate change in `devtools-source-map` to fix this.
Attachment #8849772 - Flags: review?(jdescottes) → review?(poirot.alex)
Attachment #8849773 - Flags: review?(jdescottes) → review?(poirot.alex)
Attachment #8849774 - Flags: review?(jdescottes) → review?(poirot.alex)
(:jdescottes is on PTO, let's try :ochameau!)
Comment on attachment 8849773 [details]
Bug 1349360 - Mark experimental SourceMapService as deprecated.

https://reviewboard.mozilla.org/r/122532/#review124926

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:88
(Diff revision 1)
>          openNetworkPanel: (requestId) => {
>            return this.toolbox.selectTool("netmonitor").then(panel => {
>              return panel.panelWin.NetMonitorController.inspectRequest(requestId);
>            });
>          },
> -        sourceMapService: this.toolbox ? this.toolbox._sourceMapService : null,
> +        sourceMapService: this.toolbox ? this.toolbox._deprecatedServerSourceMapService : null,

Do you plan to pass a non-deprecated one to the console codebase?
I'm wondering if we should also rename this attribute everywhere in console?
Attachment #8849773 - Flags: review?(poirot.alex) → review+
Comment on attachment 8849772 [details]
Bug 1349360 - Add source-map to browser-based list.

https://reviewboard.mozilla.org/r/122530/#review124928
Attachment #8849772 - Flags: review?(poirot.alex) → review+
Comment on attachment 8849773 [details]
Bug 1349360 - Mark experimental SourceMapService as deprecated.

https://reviewboard.mozilla.org/r/122532/#review124930

I see some occurences of sourceMapService in debugger but no places where we would actually set this property??
Is it used at all? Would it be set from another github repo somehow?
Comment on attachment 8849774 [details]
Bug 1349360 - Expose client-side source map service on toolbox.

https://reviewboard.mozilla.org/r/122534/#review124996

Looks good given your description of current source-map prefs you do in:
https://docs.google.com/document/d/19TKnMJD3CMBzwByNE4aBBVWnl-AEan8Sf4hxi6J-eps/edit#heading=h.6u2ngpszd0gd
Attachment #8849774 - Flags: review?(poirot.alex) → review+
Comment on attachment 8849773 [details]
Bug 1349360 - Mark experimental SourceMapService as deprecated.

https://reviewboard.mozilla.org/r/122532/#review125000

::: devtools/client/framework/toolbox.js:101
(Diff revision 1)
> +
> +  // TODO: This approach to source maps uses server-side source maps, which we are
> +  // replacing with client-side source maps.  Do not use this in new code paths.
> +  // To be removed in bug 1349354.
>    if (Services.prefs.getBoolPref("devtools.source-map.locations.enabled")) {
> -    this._sourceMapService = new SourceMapService(this._target);
> +    this._deprecatedServerSourceMapService = new SourceMapService(this._target);

You may also comment about that in: devtools/client/preferences/devtools.js for this pref.
Comment on attachment 8849774 [details]
Bug 1349360 - Expose client-side source map service on toolbox.

https://reviewboard.mozilla.org/r/122534/#review125002

::: devtools/client/preferences/devtools.js:345
(Diff revision 2)
>  
>  // Enable the HTML responsive design mode for all channels.
>  pref("devtools.responsive.html.enabled", true);
> +
> +// Enable client-side mapping service for source maps
> +pref("devtools.source-map.client-service.enabled", true);

You may move that next to the existing pref to help understanding the two options
http://searchfox.org/mozilla-central/source/devtools/client/preferences/devtools.js#304
Comment on attachment 8849774 [details]
Bug 1349360 - Expose client-side source map service on toolbox.

https://reviewboard.mozilla.org/r/122534/#review125002

> You may move that next to the existing pref to help understanding the two options
> http://searchfox.org/mozilla-central/source/devtools/client/preferences/devtools.js#304

Thanks, moved!
Comment on attachment 8849773 [details]
Bug 1349360 - Mark experimental SourceMapService as deprecated.

https://reviewboard.mozilla.org/r/122532/#review124926

> Do you plan to pass a non-deprecated one to the console codebase?
> I'm wondering if we should also rename this attribute everywhere in console?

In a future step, yes, I'll pass the new client-side service to the console and other tools.  That step is more complex though, since the API is different between the old and the new.

I was thinking I'll swap them out when I do the API conversion in the console.  I think this name can stay as-is because I don't intend to have both services passed to the console simultaneously.
Comment on attachment 8849773 [details]
Bug 1349360 - Mark experimental SourceMapService as deprecated.

https://reviewboard.mozilla.org/r/122532/#review124930

It looks like this is the `frame` component in `devtools-sham-modules`.  It seems like dead code for the debugger.  There's probably a lot to prune there.
Comment on attachment 8849773 [details]
Bug 1349360 - Mark experimental SourceMapService as deprecated.

https://reviewboard.mozilla.org/r/122532/#review125000

> You may also comment about that in: devtools/client/preferences/devtools.js for this pref.

Good idea, added a comment there.  I also added a link to my doc about source maps.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05bc9875a17a
Add source-map to browser-based list. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/4d99e3b1fc8b
Mark experimental SourceMapService as deprecated. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/1c65b4206a84
Expose client-side source map service on toolbox. r=ochameau
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: