Closed
Bug 1349360
Opened 8 years ago
Closed 8 years ago
Expose client-side source map service in toolbox
Categories
(DevTools :: General, enhancement, P1)
DevTools
General
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jryans, Assigned: jryans)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
We want to expose the `devtools-source-map` package (using client side mapping) for use by various panels.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8849772 -
Flags: review?(jdescottes) → review?(poirot.alex)
Attachment #8849773 -
Flags: review?(jdescottes) → review?(poirot.alex)
Attachment #8849774 -
Flags: review?(jdescottes) → review?(poirot.alex)
Assignee | ||
Comment 6•8 years ago
|
||
(:jdescottes is on PTO, let's try :ochameau!)
Comment 7•8 years ago
|
||
mozreview-review |
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 8•8 years ago
|
||
mozreview-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 9•8 years ago
|
||
mozreview-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 10•8 years ago
|
||
mozreview-review |
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 11•8 years ago
|
||
mozreview-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 12•8 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
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!
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 15•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 16•8 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05bc9875a17a
https://hg.mozilla.org/mozilla-central/rev/4d99e3b1fc8b
https://hg.mozilla.org/mozilla-central/rev/1c65b4206a84
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•