New CSS parser written in Dart to replace pyparser

New tree objects to match CSS BNF.

Fairly complete CSS syntax

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=2675

[terry, 2011-12-20 22:16:40]

John,

There's a lot of code here for Jim and Nathan to review later.  However, there
are a few changes to frog that I'd like you to review.

Thanks,
Terry.

[Jennifer Messerly, 2011-12-20 22:28:15]

frog changes look good

http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart
File frog/file_system_node.dart (right):

http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart#n...
frog/file_system_node.dart:46: } catch (var e) {
Is this just a style change, or did the compiler need this for correctness?

style wise I suspect we'll move to not having "var" -- although frog's for-in
loops are mostly not doing that yet.

[terry, 2011-12-20 22:32:14]

On 2011/12/20 22:28:15, John Messerly wrote:
> frog changes look good
> 
> http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart
> File frog/file_system_node.dart (right):
> 
>
http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart#n...
> frog/file_system_node.dart:46: } catch (var e) {
> Is this just a style change, or did the compiler need this for correctness?
> 
> style wise I suspect we'll move to not having "var" -- although frog's for-in
> loops are mostly not doing that yet.

[terry, 2011-12-20 22:33:52]

On 2011/12/20 22:32:14, terry wrote:
> On 2011/12/20 22:28:15, John Messerly wrote:
> > frog changes look good
> > 
> > http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart
> > File frog/file_system_node.dart (right):
> > 
> >
>
http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart#n...
> > frog/file_system_node.dart:46: } catch (var e) {
> > Is this just a style change, or did the compiler need this for correctness?
> > 
> > style wise I suspect we'll move to not having "var" -- although frog's
for-in
> > loops are mostly not doing that yet.

Did this for DartC because it requires the var (and I wrote CSS using the
DartEditor).  However, I can certainly remove this.

[Jennifer Messerly, 2011-12-20 22:41:31]

On 2011/12/20 22:33:52, terry wrote:
> On 2011/12/20 22:32:14, terry wrote:
> > On 2011/12/20 22:28:15, John Messerly wrote:
> > > frog changes look good
> > > 
> > >
http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart
> > > File frog/file_system_node.dart (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart#n...
> > > frog/file_system_node.dart:46: } catch (var e) {
> > > Is this just a style change, or did the compiler need this for
correctness?
> > > 
> > > style wise I suspect we'll move to not having "var" -- although frog's
> for-in
> > > loops are mostly not doing that yet.
> 
> Did this for DartC because it requires the var (and I wrote CSS using the
> DartEditor).  However, I can certainly remove this.

Ah, makes sense. Yeah, in that case I'd leave it the way it is in your change.
Would be worth filing an issue against the Editor if there isn't one already
(low priority though)

[terry, 2011-12-20 22:50:39]

TBR=jimhug,nweiz

http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart
File frog/file_system_node.dart (right):

http://codereview.chromium.org/8937017/diff/4061/frog/file_system_node.dart#n...
frog/file_system_node.dart:46: } catch (var e) {
I did this because DartC errors if no var and I wrote CSS using the DartEditor. 
But I can remove this.

On 2011/12/20 22:28:15, John Messerly wrote:
> Is this just a style change, or did the compiler need this for correctness?
> 
> style wise I suspect we'll move to not having "var" -- although frog's for-in
> loops are mostly not doing that yet.

[nweiz, 2012-01-04 19:05:40]

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart
File utils/css/generate.dart (right):

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:5: class Generate {
A class full of static methods seems like it wants to just be a bunch of
top-level methods instead.

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:8: static List<String> computeClassSelectors(RuleSet
ruleset, classes) {
Shouldn't this be a Set rather than a List? That would make this O(n) rather
than O(n^2).

Even if it's a list, you don't need to return it, since you're destructively
modifying it.

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:26: static dartClass(FileSystem files, String outPath,
Stylesheet stylesheet,
This should either list the return type or not return a value.

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:29: List<String> knownClasses = [];
Set?

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:32: '// File generated by Dart CSS from source file
${filename}\n' +
Style nit: $filename

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:35: // Emit class for any @stylet directive.
Shouldn't this comment be attached to the StyletDirective code in particular?

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:37: if (production is Directive) {
Looks like you can just check for StyletDirective and IncludeDirective directly,
without the additional nesting of the Directive check.

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:46: for (var selector in
ruleset.selectorGroup.selectors) {
I'm having trouble understanding this loop. Why are you ignoring simple selector
sequences with length > 1? If there's more than one with length == 1, won't you
end up with more than one "{"?

If you only expect there to be one simple selector in the ruleset, make that
explicit by using #first and asserting the length.

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:47: var selSeq = selector.simpleSelectorSquences;
s/Squences/Sequences/

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:49: buff.add('    \'${selSeq.toString()}\' : const
{\n');
$selSeq; toString will be called automatically.

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:55: '\'${decl.expression.toString()}\',\n');
${decl.expression}

http://codereview.chromium.org/8937017/diff/7046/utils/css/generate.dart#newc...
utils/css/generate.dart:74: StringBuffer classSelectors = new StringBuffer(
Why is this a separate StringBuffer? Why not use the existing buffer?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart
File utils/css/parser.dart (left):

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#oldcode3
utils/css/parser.dart:3: // BSD-style license that can be found in the LICENSE
file.
Typo?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart
File utils/css/parser.dart (right):

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcode30
utils/css/parser.dart:30: // TODO(terry): Need to handle charset, import, media
and page.
I think you'll eventually want to parse unknown directives in order to be
forwards-compatible with new directives that will inevitably be added to CSS, as
well as browser-specific directives. You don't want to get into the position
where you need to update this code every time any browser adds a new directive.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:130: SelectorGroup parseTemplate() {
Now that we're parsing things other than just selectors, maybe call this
parseSelectorTemplate?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:151: SelectorGroup group = new SelectorGroup(selectors,
_makeSpan(start));
Why not just new SelectorGroup([processSelector()], ...)?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:162: processMedia([bool oneRequired = false]) {
Why are all the names prefixed with "process"? Seems redundant, as well as
inconsistent with the style in frog/parser.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:162: processMedia([bool oneRequired = false]) {
This should be called processMediaQuery, since it only parses the query portion
and not the full @media directive.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:165: while (_peekIdentifier()) {
Add a TODO here about supporting the full media query syntax
(http://www.w3.org/TR/css3-mediaqueries/).

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:167: var medium = identifier();   // Medium ident.
What does "medium" mean here?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:190: //  keyframes:    '@-webkit-keyframes ...' (see
grammar below).
Also @-moz-keyframes, and probably also @keyframes for forwards-compatibility.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:196: if (_maybeEat(TokenKind.AT)) {
if (!_maybeEat(TokenKind.AT)) return;

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:204: if (func is UriTerm) {
This needs better error handling.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:211: // Any medias?
Grammar nit: "media" is already plural

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:226: ruleset = processRuleSet();
@media directives can contain multiple rulesets.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:240: if (_peekIdentifier()) {
What if there is no identifier after the colon?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:260: if (_peekIdentifier()) {
Needs error handling.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:298: if (_fs.fileExists('${_basePath}${filename}')) {
I really don't like resolving the @include in the parser. This seems like the
job for a different part of the compilation process. Parsing should never invoke
the filesystem.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:329: if (_peekIdentifier()) {
Needs error handling.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:565: if (_maybeEat(TokenKind.LBRACK)) {
if (!...) return;

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:568: int op = TokenKind.NO_MATCH;
This would be clearer if you set op to NO_MATCH in the default clause of the
switch statement.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:615: if (TokenKind.isIdentifier(_peekToken.kind)) {
if (!...) return null;

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:702: if (_peekKind(TokenKind.INTEGER)) {
This seems really nasty. Shouldn't you be parsing HASH as a token unto itself as
per the CSS grammar?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:733: value = '"${value}"';
This will break if the original string was single-quoted and included a double
quote. In order to preserve the original structure of the CSS, you should
probably keep track of whether the original string was single- or double-quoted.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:735: case TokenKind.LPAREN:
Is there somewhere the semantics of additions like GroupTerm and ItemTerm are
explained?

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:788: print('Warning: unknown property value
${error.name}');
I don't understand this warning. There are tons of identifiers other than color
names that are valid in properties. Syntactically, every identifier is valid.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:799: switch (unitType) {
Manually enumerating all the possible unit types, and the semantics of the
numbers that use them, seems like a huge amount of effort that buys you nothing.
The way Sass does it is to support any identifier (as well as "%") as a unit.
This seems much more robust and future-proof, while also requiring much less
code.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:850: processQuotedString([bool urlString = false]) {
Why are you parsing strings in the parser and not the tokenizer? This seems like
not only a violation of layering (strings are conceptually a single token) but
also very error-prone, as you rely on everything in the string being tokenizable
in order for the parse to work.

http://codereview.chromium.org/8937017/diff/7046/utils/css/parser.dart#newcod...
utils/css/parser.dart:898: case 'url':
It seems wrong that url() is being parsed in processFunction when it's not
semantically a function and it only superficially resembles one syntactically.
It seems much cleaner to me to tokenize each url().

http://codereview.chromium.org/8937017/diff/7046/utils/css/tree.dart
File utils/css/tree.dart (right):

http://codereview.chromium.org/8937017/diff/7046/utils/css/tree.dart#newcode380
utils/css/tree.dart:380: RuleSet _ruleset;
Shouldn't this be a list of RuleSets?

http://codereview.chromium.org/8937017/diff/7046/utils/css/tree.dart#newcode474
utils/css/tree.dart:474: List<Declaration> _declarations;
Shouldn't this be a DeclarationGroup?