Tom Tromey :tromey Assignee
	Description • 10 years ago

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

	Tom Tromey :tromey Assignee
	Comment 1 • 10 years ago

This changes everything to use nsICSSLexer.

	Tom Tromey :tromey Assignee
	Comment 2 • 10 years ago

Updated for the changes to the lexer.

	Tom Tromey :tromey Assignee
	Comment 3 • 10 years ago

Update for another revision to the webidl.

	Tom Tromey :tromey Assignee
	Comment 4 • 10 years ago

Final update, ready for review.

	Tom Tromey :tromey Assignee
	Comment 5 • 10 years ago

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.

	Patrick Brosset <:pbro>
	Comment 6 • 10 years ago

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.

	Tom Tromey :tromey Assignee
	Comment 7 • 10 years ago

(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?.

	Tom Tromey :tromey Assignee
	Comment 8 • 10 years ago

Updated for review comments.

	Tom Tromey :tromey Assignee
	Comment 9 • 10 years ago

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

	Tom Tromey :tromey Assignee
	Comment 10 • 10 years ago

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.

	Tom Tromey :tromey Assignee
	Comment 11 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 10 years ago

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