Closed Bug 1179820 Opened 9 years ago Closed 7 years ago

change inspector to use client-side source maps

Categories

(DevTools :: Inspector, enhancement, P3)

41 Branch
enhancement

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.
Depends on: 1179823
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
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)
See Also: → 1045237, 1134798
Blocks: 1364232
Blocks: 1364992
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
Depends on: 1371852
Assignee: nobody → ttromey
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.
Blocks: 1388497
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.
Depends on: 1388855
No longer blocks: 1388497
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.
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.
Blocks: 1402382
Once the conversion is complete, OriginalSourceActor, it's spec, and a few style sheet actor methods
can be removed as well.
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
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 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 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 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+
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 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 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+
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.
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.
One problem is that I failed to take the href/nodeHref distinction into account.
The first patch doesn't introduce the leak.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=931cfc3ad42c66ec8dcd818b6c5c84d36a41b270

Trying the second now.
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
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.
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.
Rebased & can't reproduce the leak locally.
Now for try.
:Pike says that L10N failure can be fixed by rebasing.
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
Depends on: 1409606
See Also: → 1330383
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: