Closed
Bug 1388497
Opened 7 years ago
Closed 7 years ago
source maps not applied to css warnings
Categories
(DevTools :: Framework, defect, P3)
DevTools
Framework
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → ttromey
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
I realized I forgot to unregister an event handler.
New patch momentarily.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
Did a talos push since it looks like this is going to eagerly initialize the StyleSheetActor: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=fe6098484af1ab47f120a9fab47d4b25c7ff455e&newProject=try&newRevision=e82ac7e4c7b3248ab4671a7817db9f82ca90403f&framework=1&showOnlyImportant=0&showOnlyConfident=1
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
Note to self, this needs an update now due to bug 1397967.
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 12•7 years ago
|
||
Maybe nsIScriptError.category could be used but that would mean getting that data
to the Frame component somehow.
Assignee | ||
Comment 13•7 years ago
|
||
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.
Assignee | ||
Comment 14•7 years ago
|
||
I think on the whole I prefer isolating the weirdness to the service, so I'm going
to try the lazy initialization idea.
Assignee | ||
Comment 15•7 years ago
|
||
Since most code is using subscribe, I think lazy initialization will be just fine here.
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Forgot to ignore protocol errors here.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
Handle the case where the style sheet actor is not available on the target.
Comment 23•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25116b9d0541
apply source maps to CSS warnings in the console; r=bgrins
Comment 24•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•