Closed Bug 1388497 Opened 7 years ago Closed 7 years ago

source maps not applied to css warnings

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

A warning coming from a source-mapped css file doesn't link to the correct
source in the console.
See https://tromey.github.io/source-map-examples/css-warning/index.html for a test.

This probably depends on the inspector change.
Looking into this.  I think we'll need a way to get the style sheet actors so we can do the source mapping.
And, we'll want to keep track as the sheets come and go.
Depends on: 1224558
Assignee: nobody → ttromey
Depends on: 1388855
No longer depends on: 1179820
I realized I forgot to unregister an event handler.
New patch momentarily.
Comment on attachment 8898921 [details]
Bug 1388497 - apply source maps to CSS warnings in the console;

https://reviewboard.mozilla.org/r/170284/#review175550

When I apply this locally I don't see the scss url either on the source-map-examples page or when running the mochitest. Do we need to wait for patches from the blockers to land before this is expected to work?

::: devtools/client/framework/toolbox.js:2675
(Diff revision 2)
> +  initStyleSheetsFront: function () {
> +    if (!this._styleSheets) {
> +      if (this.target.form.styleSheetsActor) {
> +        this._styleSheets = StyleSheetsFront(this.target.client, this.target.form);
> +      } else {
> +        /* We're talking to a pre-Firefox 29 server-side */

Doesn't have to happen in this bug, but we should discuss removing any remaining pre-29 StyleEditor code from the tree
Attachment #8898921 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8898921 [details]
> Bug 1388497 - apply source maps to CSS warnings in the console;
> 
> https://reviewboard.mozilla.org/r/170284/#review175550
> 
> When I apply this locally I don't see the scss url either on the
> source-map-examples page or when running the mochitest. Do we need to wait
> for patches from the blockers to land before this is expected to work?

Yes.
Looks like there's a pretty big regression with stylo disabled on osx but nothing with high confidence with stylo enabled (although it is showing a low confidence 6% regression on osx). I guess it makes sense that stylo would be a factor here since we are starting up the StyleSheetsActor.

If this really doesn't cause a regression stylo enabled it may not be worth doing these optimizations, but if you think any of them make sense and would be simple I'd be interested to do another talos push:

1) It seems that it would be ideal to wait to create the stylesheetsFront until we get a call to originalPositionFor that we knew was a CSS file, but from the signature it looks like we just have a URL so I don't think we can know the resource type from that
2) Another option would be to wait to kick off all of the loadingPromise stuff until after a call to originalPositionFor happens (making the first call slow but limiting the work we do when the service is constructed)
3) Another would be to kick off the loadingPromise stuff into an idle callback in the SourceMapURLService constructor.
Note to self, this needs an update now due to bug 1397967.
Comment on attachment 8898921 [details]
Bug 1388497 - apply source maps to CSS warnings in the console;

https://reviewboard.mozilla.org/r/170284/#review175550

> Doesn't have to happen in this bug, but we should discuss removing any remaining pre-29 StyleEditor code from the tree

This happened in bug 1397967 and I've updated my patch (locally) to cope.
Maybe nsIScriptError.category could be used but that would mean getting that data
to the Frame component somehow.
One idea is to have the console notify the sourceMapURLService that it's seen a
CSS warning and have it start the server side then.
This wouldn't require messing with the Frame component.
I think on the whole I prefer isolating the weirdness to the service, so I'm going
to try the lazy initialization idea.
Since most code is using subscribe, I think lazy initialization will be just fine here.
Comment on attachment 8898921 [details]
Bug 1388497 - apply source maps to CSS warnings in the console;

https://reviewboard.mozilla.org/r/170284/#review183380

Great, thanks!
Attachment #8898921 - Flags: review?(bgrinstead) → review+
Forgot to ignore protocol errors here.
Handle the case where the style sheet actor is not available on the target.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25116b9d0541
apply source maps to CSS warnings in the console; r=bgrins
https://hg.mozilla.org/mozilla-central/rev/25116b9d0541
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.