preserve sourceURL when parsing style sheets

RESOLVED FIXED in Firefox 57

Status

()

defect
P3
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

(1 attachment)

Much like sourceMappingURL, "sourceURL" is another special comment that
is useful for devtools.  The CSS parser should preserve this as well.
Comment on attachment 8908825 [details]
Bug 1399911 - preserve sourceURL comment directive on style sheets;

https://reviewboard.mozilla.org/r/180442/#review185680

::: layout/style/nsCSSScanner.cpp:551
(Diff revision 1)
> + * If the given text appears at the current offset in the buffer,
> + * advance over the text and return true.  Otherwise, return false.
> + * mLength is the number of characters in mDirective.
> + */
> +bool
> +nsCSSScanner::CheckCommentDirective(const char16_t mDirective[], size_t mLength)

(These should be aDirective and aLength.)

::: layout/style/nsCSSScanner.cpp:569
(Diff revision 1)
> -  static const char sourceMappingURLDirective[] = "# sourceMappingURL=";
> +  // Note that these do not start with "#" or "@" -- that is handled
> +  // separately, below.
> +  static const char16_t sourceMappingURLDirective[] = u" sourceMappingURL=";

I think I would prefer to use Gecko string classes rather than arrays of chars and lengths and memcmp.  How about we declare these two like this:

static NS_NAMED_LITERAL_STRING(kSourceMappingURLDirective, " sourceMappingURL=");

or

static const nsLiteralString kSourceMappingURLDirective(u" sourceMappingURL=");

and pass that to CheckCommentDirective, and make CheckCommentDirective use an nsDependentSubstring to wrap the contents of mBuffer from mOffset to the end, and then we can call StringBeginsWith.

::: layout/style/nsCSSScanner.cpp:572
(Diff revision 1)
>  nsCSSScanner::SkipComment()
>  {
> -  static const char sourceMappingURLDirective[] = "# sourceMappingURL=";
> +  // Note that these do not start with "#" or "@" -- that is handled
> +  // separately, below.
> +  static const char16_t sourceMappingURLDirective[] = u" sourceMappingURL=";
> +  static const char16_t sourceURLDirective[] = u" sourceURL=";

The Firebug blog post you link to has a regular expression (which seems to be missing some backslashes but I can see where they should go), which doesn't match what we're parsing here.  That page says any white space character can appear after the @, and that any amount of white space can appear around the "=".

It also effectively says that it's not a valid source URL directive if we don't find the end of the line immediately after the optional white space after the URL.

Are these differences important?

::: layout/style/test/browser_sourceurl_comment.js:8
(Diff revision 1)
> +  let test_cases = [
> +    ["/*# sourceURL=here*/", "here"],
> +    ["/*# sourceURL=here  */", "here"],
> +    ["/*@ sourceURL=here*/", "here"],
> +    ["/*@ sourceURL=there*/ /*# sourceURL=here*/", "here"],
> +    ["/*# sourceURL=here there  */", "here"],

For example, this one should be "" if we are parsing to the Firebug blog post's regular expression...
Comment on attachment 8908825 [details]
Bug 1399911 - preserve sourceURL comment directive on style sheets;

https://reviewboard.mozilla.org/r/180442/#review185682

r=me on the webidl bits.
Attachment #8908825 - Flags: review?(bzbarsky) → review+
Comment on attachment 8908825 [details]
Bug 1399911 - preserve sourceURL comment directive on style sheets;

https://reviewboard.mozilla.org/r/180442/#review185680

> The Firebug blog post you link to has a regular expression (which seems to be missing some backslashes but I can see where they should go), which doesn't match what we're parsing here.  That page says any white space character can appear after the @, and that any amount of white space can appear around the "=".
> 
> It also effectively says that it's not a valid source URL directive if we don't find the end of the line immediately after the optional white space after the URL.
> 
> Are these differences important?

Thank you for catching this.  I missed it, I suspect because I was looking at the source map URL "spec", which works a different way and which implies that sourceURL does too.

The sourceURL regexp allows any white space after the "@"/"#" -- but SpiderMonkey only allows a space.  https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/js/src/frontend/TokenStream.cpp#1135.  This has worked this way since the code was introduced in 2013.

I've been going back and forth on what to do, primarily because I don't believe there are cases in the wild of anything other than "sourceURL=something".  However, my current thinking is to do what SpiderMonkey does.  It is closer to the spec and so less likely to bite in the future.

I think the newline thing should just be ignored.  Either that "$" refers to the end of the comment text, or it is an artifact of when these directives were only applied to JS.  (My understanding is that they were extended to CSS later on.)
Comment on attachment 8908825 [details]
Bug 1399911 - preserve sourceURL comment directive on style sheets;

https://reviewboard.mozilla.org/r/180442/#review185680

> Thank you for catching this.  I missed it, I suspect because I was looking at the source map URL "spec", which works a different way and which implies that sourceURL does too.
> 
> The sourceURL regexp allows any white space after the "@"/"#" -- but SpiderMonkey only allows a space.  https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/js/src/frontend/TokenStream.cpp#1135.  This has worked this way since the code was introduced in 2013.
> 
> I've been going back and forth on what to do, primarily because I don't believe there are cases in the wild of anything other than "sourceURL=something".  However, my current thinking is to do what SpiderMonkey does.  It is closer to the spec and so less likely to bite in the future.
> 
> I think the newline thing should just be ignored.  Either that "$" refers to the end of the comment text, or it is an artifact of when these directives were only applied to JS.  (My understanding is that they were extended to CSS later on.)

Well, I actually read the SpiderMonkey code implementing the URL extraction and it just stops at the first whitespace.  So, I think we can just follow that and leave the code as it is today.  If it's a problem, we should fix it in all places.
Comment on attachment 8908825 [details]
Bug 1399911 - preserve sourceURL comment directive on style sheets;

https://reviewboard.mozilla.org/r/180442/#review186796

Thanks, happy to defer to you on the specific parsing details of the comment.

::: layout/style/nsCSSScanner.cpp:571
(Diff revisions 1 - 2)
>  nsCSSScanner::SkipComment()
>  {
>    // Note that these do not start with "#" or "@" -- that is handled
>    // separately, below.
> -  static const char16_t sourceMappingURLDirective[] = u" sourceMappingURL=";
> -  static const char16_t sourceURLDirective[] = u" sourceURL=";
> +  static NS_NAMED_LITERAL_STRING(kSourceMappingURLDirective, " sourceMappingURL=");
> +  static NS_NAMED_LITERAL_STRING(kSourceURLDirective, u" sourceURL=");

Nit: can drop the "u" here.

(Although it doesn't make a difference, since the macro just pastes an empty u"" string before the string you provide.)
Attachment #8908825 - Flags: review?(cam) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d5c378f5610
preserve sourceURL comment directive on style sheets; r=bz,heycam
https://hg.mozilla.org/mozilla-central/rev/2d5c378f5610
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.