Extract source map URL from directive comments

[tromey]

Change the parser to extract the source map URL from directive comments.
The relevant spec is here:

https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.lmz475t4mvbx

This is part of similar work being done in M-C in
https://bugzilla.mozilla.org/show_bug.cgi?id=1388855

---

This change is 

[SimonSapin]

I don’t think this belongs in the CSS tokenizer. Could it be built on top, with Parser::next_including_whitespace_and_comments?

[tromey]

The reason I put it into the token stream is because it seemed to me that there could be multiple parsers using a single token stream. At least, that's what I understood when I was looking into the servo side of this. In this situation, one must be careful to query whichever parser happened to see the magic comment.

Perhaps in practice this isn't a problem. In normal use the directive only appears at the end of the file.

[SimonSapin]

Sorry I was unclear. I meant using next_including_whitespace_and_comments outside of this crate in higher-level stylesheet parsing code, not moving this from Tokenizer to Parser.

[SimonSapin]

The Source Map "proposal" is a bit unclear. Does the magic comment need to be at the very start of the stylesheet, or can it be anywhere in the middle?

[tromey]

Does the magic comment need to be at the very start of the stylesheet, or can it be anywhere in the middle?

I think it can be anywhere. In practice tools put it at the end of the text.

To make sure I understand, you are saying I should just close this PR and do the work in servo? I can do that; though of course it means any other user of this crate will also have to reimplement this code.

[SimonSapin]

I was saying that I’d prefer such a solution, though I don’t know how that would work exactly. But if the magic comment can be anywhere that’s more annoying: this may need to be in the tokenizer after all.

Still, I’d prefer not to duplicate the inner loop of consume_comment. Could this leave that function unchanged and look at its return value instead? (Or something like only change it to call another function when it’s about to return.)

[tromey]

The text says:

The generated code may include a line at the end of the source, with the following form: [...]

It's hard to say if that is normative though. At least SpiderMonkey, and the current approach in DevTools, both allow the comment to appear anywhere. I don't know whether any tools put the comment anywhere besides the end.

[tromey]

I looked a little deeper at other approaches.

The end goal is to expose this value via StyleSheet, see https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/dom/webidl/StyleSheet.webidl#33

To do that I think I would need it to be available in StyleSheetContents: https://github.com/servo/servo/blob/master/components/style/stylesheets/stylesheet.rs#L48, in particular via the from_str constructor, since that seems to be what Stylo uses.

The contents are parsed using a rule list parser: https://github.com/servo/servo/blob/master/components/style/stylesheets/stylesheet.rs#L48

The rule list parser works primarily by callbacks. Servo provides a TopLevelRuleParser that handles the details. I read through this code a bit and it seems to call back into rust-cssparser to use DeclarationListParser (I've only looked at ordinary rules, not at-rules).

So maybe the work can be done here somehow.

[SimonSapin]

This may be tricky given the way RuleListParser works (consuming &mut Parser until the end of the input, and calling back for style rules and at-rules). So I’ve changed my mind, this may need to be in the tokenizer after all.

This PR looks good overall, except that I’d prefer it didn’t duplicate the inner loop of consume_comment. So please make the following changes:

• Revert changes to consume_comment
• In next_token, replace Comment(consume_comment(tokenizer)) with:

  let comment_content = consume_comment(tokenizer);
  check_for_source_map_url(tokenizer, comment_content);
  Comment(comment_content)

• Add a new check_for_source_map_url function (or whatever name you feel describes it) that sets tokenizer.source_map_url as appropriate

[SimonSapin]

Looks good. I assume you’ll want this change on crates.io to be able to use it in Servo or Gecko, so please also change the version number in Cargo.toml from 0.19.0 to 0.19.1. r=me with that.

[SimonSapin]

@bors-servo r+

[bors-servo]

📌 Commit 94330b1 has been approved by SimonSapin

[bors-servo]

⌛ Testing commit 94330b1 with merge ff1ba7d...

[bors-servo]

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing ff1ba7d to master...