Closed
Bug 1179820
Opened 9 years ago
Closed 7 years ago
change inspector to use client-side source maps
Categories
(DevTools :: Inspector, enhancement, P3)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: jsantell, Assigned: tromey)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
This may kind of work now? When I see it, it seems to flash the original location as it reverts to the generated location.
Comment 1•8 years ago
|
||
Triaging as enhancement for now. Filter on CLIMBING SHOES. Jordan: The summary is a bit too vague, what is the expected outcome of this bug?
Severity: normal → enhancement
Flags: needinfo?(jsantell)
Priority: -- → P3
Reporter | ||
Comment 2•8 years ago
|
||
This is for showing source map style files in the rules view when using CSS preprocessors with source maps. I believe it works somehow, but not wired up at all to the SourceLocationController (which we may or may not want if its already working, very unfamiliar with the inspector)
Flags: needinfo?(jsantell)
Updated•7 years ago
|
Blocks: source-maps
Assignee | ||
Comment 3•7 years ago
|
||
The inspector does use source maps, but I'm repurposing this bug to switch it to the new client-side service.
Summary: Display source mapped locations in Inspector → change inspector to use client-side source maps
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ttromey
Assignee | ||
Comment 4•7 years ago
|
||
I think this will probably require also changing the style editor, because otherwise clicking on a source-mapped URL in the inspector will not open the correct file in the style editor.
Assignee | ||
Comment 5•7 years ago
|
||
Currently the stylesheet actor fetches the style sheet source to find the sourceMappingURL. I'm wondering, though, if this could be handled the way that SpiderMonkey handles it - let the parser extract this and make it available. This way we could have the source map URL service fetch all the style sheet actors, but not have to fetch the original sources just to get source map information.
Assignee | ||
Comment 6•7 years ago
|
||
I'm partway through my second attempt at this. I abandoned the first after redoing many things in pursuit of bug 1388497. The second approach is working ok so far, but there's an oddity that isn't handled (and is maybe a bug in the current approach). Any sort of edit in the rule view will rewrite the style sheet text. This edit preserves the sourceMappingURL comment; but that is wrong because the source map will no longer apply. Ideally I suppose we'd just discard the parts of the source map that have been invalidated. However, that might be difficult (not sure), so another idea is to just discard the source map entirely on edits.
Assignee | ||
Comment 7•7 years ago
|
||
It seems to me that I was overthinking this. We're moving the service from the server to the client. This by itself shouldn't really affect much -- whatever bugs there are are probably pre-existing. The one exception I know of is if a style sheet change would now cause a re-fetch of the original source. Currently the server side is lax about this.
Assignee | ||
Comment 8•7 years ago
|
||
Once the conversion is complete, OriginalSourceActor, it's spec, and a few style sheet actor methods can be removed as well.
Assignee | ||
Comment 9•7 years ago
|
||
I have a patch series for this and I'm working my way through fixing up the tests. I found out that we'll at least need the upstream SourceMapConsumer constructor change; so adding a dependency.
Depends on: 1401563
Assignee | ||
Comment 10•7 years ago
|
||
These pass most testing locally; the remainder, I think, being bug 1401563. So, while these aren't ready to land, they are probably ready for review.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8913794 [details] Bug 1179820 - Convert the computed view to client-side source maps; https://reviewboard.mozilla.org/r/185166/#review190562 ::: devtools/client/inspector/computed/computed.js:1236 (Diff revision 1) > > - this.ready = this.updateSourceLink(); > + const rule = this.selectorInfo.rule; > + if (!rule || !rule.parentStyleSheet || rule.type == ELEMENT_STYLE) { > + this.source = CssLogic.l10n("rule.sourceElement"); > + } else { > + this._updateLocation = this._updateLocation.bind(this); Let's move this function binding to below this.openStyleEditor.bind(this); ::: devtools/client/inspector/computed/computed.js:1343 (Diff revision 1) > > /** > * Update the text of the source link to reflect whether we're showing > * original sources or not. > */ > - updateSourceLink: function () { > + _updateLocation: function (enabled, url, line, column) { I think we need some @params to explain what these variables represent. I found myself question what enabled meant until I looked up sourceMapURLService.subscribe ::: devtools/client/inspector/computed/computed.js:1351 (Diff revision 1) > - }, > > - /** > - * Update the 'source' store based on our original sources preference. > - */ > - updateSource: function () { > + if (enabled) { > + this.currentLocation = { href: url, line, column }; > + } else { > + this.currentLocation = this.generatedLocation; I am wondering if we need this assignment since we did "this.generatedLocation = this.currentLocation;" prior in line 1247. ::: devtools/client/inspector/computed/computed.js:1389 (Diff revision 1) > let toolbox = gDevTools.getToolbox(inspector.target); > toolbox.viewSource(rule.href, rule.line); > return; > } > > - let location = promise.resolve(rule.location); > + let location = this.currentLocation; We can probably simplify this by doing let { href, line, column } = this.currentLocation
Attachment #8913794 -
Flags: review?(gl) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8913796 [details] Bug 1179820 - Log some exceptions caught in the style editor; https://reviewboard.mozilla.org/r/185170/#review190596
Attachment #8913796 -
Flags: review?(gl) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8913798 [details] Bug 1179820 - Remove source map handling from stylesheets actor; https://reviewboard.mozilla.org/r/185174/#review190598
Attachment #8913798 -
Flags: review?(gl) → review+
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8913794 [details] Bug 1179820 - Convert the computed view to client-side source maps; https://reviewboard.mozilla.org/r/185166/#review190562 > I am wondering if we need this assignment since we did "this.generatedLocation = this.currentLocation;" prior in line 1247. `currentLocation` is used to hold whichever location is currently being displayed, so that clicking the link can do the right thing without another lookup. I added a comment to explain this.
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8913795 [details] Bug 1179820 - Convert the rule view to client-side source maps; https://reviewboard.mozilla.org/r/185168/#review191450 ::: devtools/client/inspector/rules/views/rule-editor.js:69 (Diff revision 1) > this._onSelectorDone = this._onSelectorDone.bind(this); > this._locationChanged = this._locationChanged.bind(this); > this.updateSourceLink = this.updateSourceLink.bind(this); > + this._onToolChanged = this._onToolChanged.bind(this); > + this._updateLocation = this._updateLocation.bind(this); > + this._onClick = this._onClick.bind(this); s/onClick/onSourceClick for better clarity ::: devtools/client/inspector/rules/views/rule-editor.js:235 (Diff revision 1) > > /** > + * Called when a tool is registered or unregistered. > + */ > + _onToolChanged: function () { > + if (this.source.getAttribute("unselectable") === "permanent") { Add some comments for what is happening with the logic in these if statements/conditions. ::: devtools/client/inspector/rules/views/rule-editor.js:266 (Diff revision 1) > + toolbox.getCurrentPanel().selectStyleSheet(url, line, column); > + }); > + } > + }, > > - if (this.toolbox.isToolRegistered("styleeditor")) { > + _updateLocation: function (enabled, url, line, column) { Add a JSDoc and @param for this function. ::: devtools/client/inspector/rules/views/rule-editor.js:267 (Diff revision 1) > + }); > + } > + }, > > - if (this.toolbox.isToolRegistered("styleeditor")) { > - this.source.removeAttribute("unselectable"); > + _updateLocation: function (enabled, url, line, column) { > + if (enabled) { Let's avoid this empty if by simply doing if (!enabled) { }
Attachment #8913795 -
Flags: review?(gl) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8913797 [details] Bug 1179820 - Convert style editor to client-side source maps; https://reviewboard.mozilla.org/r/185172/#review191462 ::: devtools/client/styleeditor/StyleEditorUI.jsm:343 (Diff revision 1) > + let sourceMapService = toolbox.sourceMapService; > + if (!sourceMapService) { > return editor; > } > > - let sources = await styleSheet.getOriginalSources(); > + let {href: url, actor: id, sourceMapURL} = styleSheet._form; It seems like we didn't add getters for the href, actor, sourceMapURL in the StyleSheetFront if we need to access it by _form. I would prefer if we fix that, and avoid accessing it from _form. ::: devtools/client/styleeditor/original-source.js:67 (Diff revision 1) > + /** > + * Given a source-mapped, generated style sheet, a line, and a > + * column, return the corresponding original location in this style > + * sheet. > + * > + * @param {StyleSheetActor} relatedSheet I think this should be StyleSheetFront ::: devtools/client/styleeditor/original-source.js:79 (Diff revision 1) > + * The original location, an object with at least > + * `sourceUrl`, `source`, `styleSheet`, `line`, and `column` > + * properties. > + */ > + getOriginalLocation: function (relatedSheet, line, column) { > + let {href: sourceUrl, actor: sourceId} = relatedSheet._form; Same concerns as previous about _form
Attachment #8913797 -
Flags: review?(gl) → review+
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8913795 [details] Bug 1179820 - Convert the rule view to client-side source maps; https://reviewboard.mozilla.org/r/185168/#review191450 > Add a JSDoc and @param for this function. I reused the corresponding comment from the computed view.
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8913797 [details] Bug 1179820 - Convert style editor to client-side source maps; https://reviewboard.mozilla.org/r/185172/#review191910 ::: devtools/client/styleeditor/StyleEditorUI.jsm:343 (Diff revision 1) > + let sourceMapService = toolbox.sourceMapService; > + if (!sourceMapService) { > return editor; > } > > - let sources = await styleSheet.getOriginalSources(); > + let {href: url, actor: id, sourceMapURL} = styleSheet._form; I went ahead and fixed this in source-map-url-service as well.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
One problem is that I failed to take the href/nodeHref distinction into account.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•7 years ago
|
||
The first patch doesn't introduce the leak. https://treeherder.mozilla.org/#/jobs?repo=try&revision=931cfc3ad42c66ec8dcd818b6c5c84d36a41b270 Trying the second now.
Assignee | ||
Comment 36•7 years ago
|
||
The rule view patch is the leaker: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f69a8217455a74754f696b4701581c7b74970e28
Assignee | ||
Comment 37•7 years ago
|
||
Today I learned I can reproduce the leak with ./mach mochitest devtools/client/inspector/rules/test/browser_rules_copy_styles.js devtools/client/inspector/rules/test/browser_rules_search-filter_context-menu.js devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js
Assignee | ||
Comment 38•7 years ago
|
||
Just ./mach mochitest devtools/client/inspector/rules/test/browser_rules_select-and-copy-styles.js seems to be enough... but I feel certain I tried this before and it was ok.
Assignee | ||
Comment 39•7 years ago
|
||
It's a destruction race, where the toolbox destroys the source-map-url-service, and then the rule view recreates it (to unsubscribe) while trying to shut down.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 45•7 years ago
|
||
Rebased & can't reproduce the leak locally. Now for try.
Assignee | ||
Comment 46•7 years ago
|
||
:Pike says that L10N failure can be fixed by rebasing.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 52•7 years ago
|
||
Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/61b4f3f0f39d Convert the computed view to client-side source maps; r=gl https://hg.mozilla.org/integration/autoland/rev/730b1b67fb51 Convert the rule view to client-side source maps; r=gl https://hg.mozilla.org/integration/autoland/rev/9bb908ed8bcd Log some exceptions caught in the style editor; r=gl https://hg.mozilla.org/integration/autoland/rev/1a2618da581f Convert style editor to client-side source maps; r=gl https://hg.mozilla.org/integration/autoland/rev/3d7a9563f966 Remove source map handling from stylesheets actor; r=gl
Comment 53•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/61b4f3f0f39d https://hg.mozilla.org/mozilla-central/rev/730b1b67fb51 https://hg.mozilla.org/mozilla-central/rev/9bb908ed8bcd https://hg.mozilla.org/mozilla-central/rev/1a2618da581f https://hg.mozilla.org/mozilla-central/rev/3d7a9563f966
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•