Closed Bug 1153305 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox40 affected, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(1 file, 6 obsolete files)

Once bug 1152033 lands, we'll want to update css-tokenizer and its callers
to use the new interface.
This changes everything to use nsICSSLexer.
Blocks: 984880
Blocks: 1154809
Updated for the changes to the lexer.
Attachment #8591065 - Attachment is obsolete: true
Update for another revision to the webidl.
Attachment #8599946 - Attachment is obsolete: true
Final update, ready for review.
Attachment #8600008 - Attachment is obsolete: true
Attachment #8601631 - Flags: review?(pbrosset)
Attached file css-tokenizer.js (obsolete) —
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 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.
Attachment #8601631 - Flags: review?(pbrosset) → review+
(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?.
Updated for review comments.
Attachment #8601631 - Attachment is obsolete: true
Attachment #8604090 - Flags: review+
Attachment #8601634 - Attachment is obsolete: true
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.
Attachment #8604090 - Attachment is obsolete: true
Attachment #8604256 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/95f5ef05f736
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.