1153305 - update css-tokenizer and friends to use built-in CSS lexer

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tom Tromey :tromey]

Once bug 1152033 lands, we'll want to update css-tokenizer and its callers
to use the new interface.

Comment 1

[Tom Tromey :tromey]

Attachment: change css-tokenizer.js to use nsICSSLexer

This changes everything to use nsICSSLexer.

Comment 2

[Tom Tromey :tromey]

Attachment: change css-tokenizer.js to use CSSLexer

Updated for the changes to the lexer.

Comment 3

[Tom Tromey :tromey]

Attachment: change css-tokenizer.js to use CSSLexer

Update for another revision to the webidl.

Comment 4

[Tom Tromey :tromey]

Attachment: change css-tokenizer.js to use CSSLexer

Final update, ready for review.

Comment 5

[Tom Tromey :tromey]

Attachment: css-tokenizer.js

The css-tokenizer.js part of the patch is rather messy,
so I thought I'd attach the complete file in case you
want to see it directly.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8601631 [details] [diff] [review]
change css-tokenizer.js to use CSSLexer

Review of attachment 8601631 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Tom, feels good to get rid of so much code in css-tokenizer.js
You're right, the diff is very messy, but I applied the patch locally and looked at the file in my editor anyway. The changes look great to me. I just have a few remarks about documenting the functions in this file better.
The changes in the 2 other files seem good to me too. They look enough like slight API output adjustments that I didn't spend toooo much time reviewing the details. A Try build will be much better at spotting problems with these types of changes (did you push already?).
All my local manual tests did work fine, so R+ with a green try push.

::: browser/devtools/sourceeditor/css-tokenizer.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {Cc, Ci} = require('chrome');

nit: double quotes here instead.

@@ +10,5 @@
> +loader.lazyGetter(this, "DOMUtils", () => {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
> +
> +function* cssTokenizer(string) {

Can you add a comment for this function?

@@ +34,5 @@
> + * line and column information attached.
> + *
> + * It's best not to add new uses of this function.  In general it is
> + * simpler and better to use the CSSToken offsets, rather than line
> + * and column.

I think this comment should also say that it's advised to use the cssTokenizer function instead, and that the CSSToken offsets isn't the only reason, but also because it's a generator function so the lexing can be stop at any time, whereas cssTokenizerWithLineColumn goes over the whole string every time.

@@ +39,5 @@
>   */
> +function cssTokenizerWithLineColumn(string) {
> +  let lexer = DOMUtils.getCSSLexer(string);
> +  let result = [];
> +  let prevLine = 1;

This variable isn't used.

@@ +40,5 @@
> +function cssTokenizerWithLineColumn(string) {
> +  let lexer = DOMUtils.getCSSLexer(string);
> +  let result = [];
> +  let prevLine = 1;
> +  let prevColumn = 0;

ditto.

Comment 7

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)

> A Try build will be much better at spotting problems with these
> types of changes (did you push already?).

Not yet.  On #devtools I was advised to generally get a review before
a try push, so that's what I've been doing.

I do try to run all the relevant tests locally that I can think of before
marking them r?.

Comment 8

[Tom Tromey :tromey]

Attachment: change css-tokenizer.js to use CSSLexer

Updated for review comments.

Comment 9

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2cb2f8a0f82

Comment 10

[Tom Tromey :tromey]

Attachment: change css-tokenizer.js to use CSSLexer

Naturally "try" pointed out that I forgot test_parseDeclarations.js.
This required a minor change to make parseDeclarations sanity-check
its input.  And, it required some test changes -- for the better, as
the parsing utilities are generally better about preserving things
as-authored at a low level now.

Carrying over r+ on account of the nature of the changes.

Comment 11

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba4c5f80ddf6

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/95f5ef05f736

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95f5ef05f736