Closed
Bug 1153305
Opened 10 years ago
Closed 10 years ago
update css-tokenizer and friends to use built-in CSS lexer
Categories
(DevTools :: Inspector, defect)
Tracking
(firefox40 affected, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: tromey, Assigned: tromey)
References
Details
Attachments
(1 file, 6 obsolete files)
72.85 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
Once bug 1152033 lands, we'll want to update css-tokenizer and its callers to use the new interface.
Assignee | ||
Comment 1•10 years ago
|
||
This changes everything to use nsICSSLexer.
Assignee | ||
Comment 2•10 years ago
|
||
Updated for the changes to the lexer.
Attachment #8591065 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Update for another revision to the webidl.
Attachment #8599946 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Final update, ready for review.
Attachment #8600008 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8601631 -
Flags: review?(pbrosset)
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.
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.
Attachment #8601631 -
Flags: review?(pbrosset) → review+
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?.
Assignee | ||
Comment 8•10 years ago
|
||
Updated for review comments.
Attachment #8601631 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2cb2f8a0f82
Assignee | ||
Updated•10 years ago
|
Attachment #8604090 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8601634 -
Attachment is obsolete: true
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.
Attachment #8604090 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba4c5f80ddf6
Assignee | ||
Updated•10 years ago
|
Attachment #8604256 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/95f5ef05f736
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/95f5ef05f736
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•