Closed
Bug 1388855
Opened 7 years ago
Closed 7 years ago
have CSS parser find source map URLs and preserve them on style sheet
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: tromey, Assigned: tromey)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Currently to find the source map URL for a style sheet the devtools
re-fetch the style sheet's original source and search through it.
It would be convenient if we did not have to do that. Instead,
the CSS parser could extract the source map URL, the same way that
SpiderMonkey does.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8895513 [details]
Bug 1388855 - Simplify source map URL extraction in stylesheet actor;
https://reviewboard.mozilla.org/r/166716/#review171880
Attachment #8895513 -
Flags: review?(gl) → review+
Comment 4•7 years ago
|
||
Tom, can you point me to the spec for CSS source maps? I'm having trouble finding it.
Also, do you plan to implement this in Servo's CSS parser too? With Stylo enabled, will we fall back to devtools re-fetching the source to extract the source map URL, or will we think we have no source map?
Flags: needinfo?(ttromey)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review172070
r=me on the webidl changes. I didn't read the scanner parts carefully so far, because I assume Cameron is doing that, but please let me know if I should.
And I, too, would like to understand what the stylo plan is here.
::: layout/style/nsCSSParser.cpp:1650
(Diff revision 1)
> if (ParseRuleSet(AppendRuleToSheet, this)) {
> mSection = eCSSSection_General;
> }
> }
> +
> + nsString sourceMapURL(scanner.GetSourceMapURL());
This should really be declared as a `const nsAString&` to avoid the extra refcounting. And the scanner's GetSourceMapURL() should return `const nsAString&` as well.
Attachment #8895512 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #4)
> Tom, can you point me to the spec for CSS source maps? I'm having trouble
> finding it.
Yes, for some reason it is just a random google doc:
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#
This is for source maps in general; but there's one CSS example in the
"Linking generated code to source maps" section.
> Also, do you plan to implement this in Servo's CSS parser too? With Stylo
> enabled, will we fall back to devtools re-fetching the source to extract the
> source map URL, or will we think we have no source map?
I didn't think of this, but yes, I will do it.
Flags: needinfo?(ttromey)
Assignee | ||
Comment 7•7 years ago
|
||
Part 1 is here: https://github.com/servo/rust-cssparser/pull/178
I'll be rewriting it tomorrow per the review.
Part 2 will be a rust-cssparser release.
Part 3 will be servo bits.
Then I guess we can move on to the M-C stuff.
Assignee | ||
Comment 8•7 years ago
|
||
Part 3 is here: https://github.com/tromey/servo/tree/source-map-url
No PR yet because I'm having some trouble building this in M-C, and I wanted to
test it with the test case in this bug before submitting.
I was discussing it on #servo with :emilio, but had to go; will revisit next week.
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review173276
::: layout/style/nsCSSScanner.cpp:608
(Diff revision 1)
> + if (copying)
> + mSourceMapURL.Append('*');
Nit: would prefer braces around this, and below.
::: layout/style/test/browser_sourcemap_comment.js:2
(Diff revision 1)
> + // The first N (hard-coded below) should yield "here" and the
> + // remainder should be invalid. In ordinary circumstances this
> + // array would also contain the expected results, but the checking
> + // function is run in content and so can't access this.
It would be nice to avoid this hard coding. Can you make the test_cases entries look like this:
["/*# sourceMappingURL=href*/", "here"],
...
and test against that second value instead?
Attachment #8895512 -
Flags: review?(cam) → review+
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review173276
> Nit: would prefer braces around this, and below.
Oops, sorry about that. Normally I do this (I think it's clearly better), but was confused by the other spots not using braces in this file.
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review173276
> It would be nice to avoid this hard coding. Can you make the test_cases entries look like this:
>
> ["/*# sourceMappingURL=href*/", "here"],
> ...
>
> and test against that second value instead?
I tried this originally, but it did not work, because the actual testing function is serialized and run in a different context, and so has no access to the test array. https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/testing/mochitest/BrowserTestUtils/ContentTask.jsm#54-58
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review173276
> I tried this originally, but it did not work, because the actual testing function is serialized and run in a different context, and so has no access to the test array. https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/testing/mochitest/BrowserTestUtils/ContentTask.jsm#54-58
Hah, now that I re-read the ContentTask.spawn docs I see that I can send an argument along as well.
Assignee | ||
Comment 13•7 years ago
|
||
Servo PR is here: https://github.com/servo/servo/pull/18073
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
This version adds the stylo code, so I think it needs a re-review.
Attachment #8895512 -
Flags: review+ → review?(cam)
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review175610
r=me with that.
::: layout/style/ServoStyleSheet.cpp:217
(Diff revision 2)
> + nsString sourceMapURL;
> + Servo_StyleSheet_GetSourceMapURL(Inner()->mContents, &sourceMapURL);
> + if (!sourceMapURL.IsEmpty()) {
> + SetSourceMapURL(sourceMapURL);
> + }
Ah, I just noticed:
If the style sheet changes from having a source map URL to not having one, when re-parsing (e.g. being editing in devtools), then I think we want to ensure mSourceMapURL gets cleared. So let's just not check whether sourceMapURL is empty (here and in nsCSSParser) and just call SetSourceMapURL() unconditionally.
Attachment #8895512 -
Flags: review?(cam) → review+
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #17)
> If the style sheet changes from having a source map URL to not having one,
> when re-parsing (e.g. being editing in devtools), then I think we want to
> ensure mSourceMapURL gets cleared. So let's just not check whether
> sourceMapURL is empty (here and in nsCSSParser) and just call
> SetSourceMapURL() unconditionally.
I think this may result in the SourceMap HTTP header being overwritten
by the empty string. I'll give it a try; the test case is layout/style/test/browser_sourcemap.js
I tend to think it's better for devtools to handle this situation directly
somehow.
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #18)
> I think this may result in the SourceMap HTTP header being overwritten
> by the empty string. I'll give it a try; the test case is
> layout/style/test/browser_sourcemap.js
Yes, when I make the assignments unconditional, this test fails.
My inclination is to just ignore this problem here and deal with it in devtools.
However there may be other ways as well.
One other way that comes to mind is to have StyleSheet have separate members
for the response header and the value from the style sheet text, and then
to reimplement GetSourceMapURL to check both.
Maybe there's some other approach as well; let me know what you'd like.
Flags: needinfo?(cam)
Comment 20•7 years ago
|
||
(In reply to Tom Tromey :tromey from comment #19)
> One other way that comes to mind is to have StyleSheet have separate members
> for the response header and the value from the style sheet text, and then
> to reimplement GetSourceMapURL to check both.
That sounds OK, and is probably the simplest thing.
Flags: needinfo?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
This changed enough to require re-review.
Attachment #8895512 -
Flags: review+ → review?(cam)
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8895512 [details]
Bug 1388855 - Extract source map URL when parsing CSS,
https://reviewboard.mozilla.org/r/166714/#review180856
Looks good, thanks!
::: layout/style/StyleSheet.cpp:514
(Diff revision 3)
> + if (mInner->mSourceMapURL.IsEmpty())
> + aSourceMapURL = mInner->mSourceMapURLFromComment;
> + else
> - aSourceMapURL = mInner->mSourceMapURL;
> + aSourceMapURL = mInner->mSourceMapURL;
Nit: prefer braces around the if/else branches.
::: layout/style/StyleSheetInfo.h:68
(Diff revision 3)
> + // This stores any source map URL that might have been seen in a
> + // comment in the style sheet.
Can you briefly mention in the comment here why we need to store these two separately?
Attachment #8895512 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•7 years ago
|
||
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/742f0335e858
Extract source map URL when parsing CSS, r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/258bede4a379
Simplify source map URL extraction in stylesheet actor; r=gl
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/742f0335e858
https://hg.mozilla.org/mozilla-central/rev/258bede4a379
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•