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)

enhancement

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 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 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+
(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)
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.
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 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+
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.
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
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 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)
Blocks: 1388497
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+
(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.
(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 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)
Priority: -- → P1
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+
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
You need to log in before you can comment on or make changes to this bug.