have CSS parser find source map URLs and preserve them on style sheet

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

2 years ago
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 3

2 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+
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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 16

2 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)
Assignee

Updated

2 years ago
Blocks: 1388497

Comment 17

2 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

2 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

2 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)
(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

2 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

2 years ago
Priority: -- → P1

Comment 24

2 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

2 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
https://hg.mozilla.org/mozilla-central/rev/742f0335e858
https://hg.mozilla.org/mozilla-central/rev/258bede4a379
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.