Beginning of CSS parser using frog parsering infrastructure.

Beginning of CSS parser using frog parsering infrastructure.
Change for in memory buffers.


BUG=
TEST=

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

[terry, 2011-11-08 18:56:28]

Jim here's the beginning of the CSS parser using the frog infrastructure.  I'm
working on one more change integrating world fatal handling for throwing
exceptions when a parsing or validating error happens (instead of my home baked
throwing code).

More interested in how I've reused frog tokenizer/parser.

[kasperl, 2011-11-09 07:51:18]

I really don't think this belongs in the frog/ directory. I understand that you
want to use the frog parser infrastructure, but I don't see how the CSS parser
is a frog component.

[jimhug, 2011-11-09 16:04:03]

I'm excited to see the beginnings of our very own CSS parser in dart.  Having
this tool will dramatically increase our ability to do good things for
client-side code while still honoring the existing css.  I also love the ratio
of tests to non-test code in this CL!

We discussed some of this yesterday, but here's what I'd like to see before this
goes in:
1. Move the code to util/css.  Kasper is right that this doesn't belong in frog.
 Last week when frog was in experimental, this made sense as a way to handle the
dependencies, but this week there is no problem with having code in util depend
on code in frog.
2. You need to make a hard choice for each piece of frog about whether you want
to use it as a library or just take your own copy as an initial sample to help
build the css code.  Based on our discussions, I think the right choice for now
is to make css.dart depend on frog/lang.dart either with or without a prefix
because of the value we expect this to provide in the future for embedding dart
code in css.  However, as frog evolves we may change this decision and decide
that it is easier just to take a copy of the pieces you'd like to reuse.
3. Please ask Nathan to review the actual css parsing code as it would be a
shame not to take advantage of his expertise here.

Thanks for taking this first step towards a happier dart+css world in the
future! - Jim

http://codereview.chromium.org/8498020/diff/1/frog/css/cssparserexception.dart
File frog/css/cssparserexception.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/cssparserexception.dar...
frog/css/cssparserexception.dart:6: /** Can be thrown on any Css parser problem
includes source location. */
Any reason not to use CompilerException - possibly subclassing if you think the
Css part is important?

http://codereview.chromium.org/8498020/diff/1/frog/css/cssselectorexception.dart
File frog/css/cssselectorexception.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/cssselectorexception.d...
frog/css/cssselectorexception.dart:6: /** Can be thrown on any Css runtime
problem includes source location. */
Not sure why a separate exception type is needed here?

http://codereview.chromium.org/8498020/diff/1/frog/css/csstemplate.dart
File frog/css/csstemplate.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/csstemplate.dart#newcode9
frog/css/csstemplate.dart:9: static void xlate(String cssExpression, CssWorld
world) {
Speaking for Bob <smile>.  Any reason not to just make these top-level
functions?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart
File frog/css/parser_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode6
frog/css/parser_css.dart:6: * A simple recursive descent parser for the dart
language.
Old comment.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:374: _error('expected element name or universal, but
found $_peekToken', _peekToken.span);
Nit: too long line.

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart
File frog/css/tokenizer_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:5: class Tokenizer extends TokenizerBase {
It's weird to me that the file is tokenizer_css but the type is Tokenizer.  I'd
probably rename the type to CssTokenizer, but you could also rename the file to
tokenizer.dart

http://codereview.chromium.org/8498020/diff/1/frog/source.dart
File frog/source.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/source.dart#newcode11
frog/source.dart:11: //              filename is used for more than
informational.
I like making this explicit!

[terry, 2011-11-09 21:56:06]

I've moved css to $DART/utils/css added world and lang.dart (sync'ing to latest
with checking found a few errors - very nice compiler work guys) so world.fatal
works nicely and tests to the utils/tests/css.

http://codereview.chromium.org/8498020/diff/1/frog/css/cssparserexception.dart
File frog/css/cssparserexception.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/cssparserexception.dar...
frog/css/cssparserexception.dart:6: /** Can be thrown on any Css parser problem
includes source location. */
On 2011/11/09 16:04:03, jimhug wrote:
> Any reason not to use CompilerException - possibly subclassing if you think
the
> Css part is important?

Once I add lang.dart and world access then I'll set the options throwing flag to
use CompilerException.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/cssselectorexception.dart
File frog/css/cssselectorexception.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/cssselectorexception.d...
frog/css/cssselectorexception.dart:6: /** Can be thrown on any Css runtime
problem includes source location. */
This is for runtime validate exceptions after parsing.  Maybe I should rename to
CssValidateException.

On 2011/11/09 16:04:03, jimhug wrote:
> Not sure why a separate exception type is needed here?

http://codereview.chromium.org/8498020/diff/1/frog/css/csstemplate.dart
File frog/css/csstemplate.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/csstemplate.dart#newcode9
frog/css/csstemplate.dart:9: static void xlate(String cssExpression, CssWorld
world) {
On 2011/11/09 16:04:03, jimhug wrote:
> Speaking for Bob <smile>.  Any reason not to just make these top-level
> functions?

Good point Bob (Jim?) I'll do that.
Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart
File frog/css/parser_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode6
frog/css/parser_css.dart:6: * A simple recursive descent parser for the dart
language.
On 2011/11/09 16:04:03, jimhug wrote:
> Old comment.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:374: _error('expected element name or universal, but
found $_peekToken', _peekToken.span);
On 2011/11/09 16:04:03, jimhug wrote:
> Nit: too long line.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart
File frog/css/tokenizer_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:5: class Tokenizer extends TokenizerBase {
On 2011/11/09 16:04:03, jimhug wrote:
> It's weird to me that the file is tokenizer_css but the type is Tokenizer. 
I'd
> probably rename the type to CssTokenizer, but you could also rename the file
to
> tokenizer.dart

Done.

[nweiz, 2011-11-10 00:04:39]

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart
File frog/css/parser_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode16
frog/css/parser_css.dart:16: bool _inInitializers;
Unused variable.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode66
frog/css/parser_css.dart:66: return TokenKind.isIdentifier(_peekToken.kind);
Why can't you use _peekKind(TokenKind.IDENTIFIER)?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode85
frog/css/parser_css.dart:85: void _eatSemicolon() {
Unused method.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:130: List<SelectorGroup> expression([bool throwErrors =
false]) {
I think your terminology is confusing here. "expression" in particular is
ambiguous with expressions in property values, but "SelectorGroup" and
"selectorExpression" are also non-standard. The CSS spec divides selectors up as
follows:

foo.bar baz, beep
\A/
\--B--/
\----C----/
\-------D-------/

A is referred to as a "simple selector"; B as a "sequence of simple selectors";
C as a "selector"; and D as a "group of selectors".

In keeping with the CSS terminology, this method should probably be called
"selectorGroup" and what's not "SelectorGroup" should just be "Selector".

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:149: if (!_maybeEat(TokenKind.END_OF_FILE)) {
Can you just call isPrematureEndOfFile?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:164: if (_peek() == TokenKind.AT) {
Is there any reason to do all these if checks, rather than just using _eat and
using the built-in error handling? These deeply nested if statements are pretty
ugly.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:190: 'Can not mix Id selector with class selector(s).
Id ' +
This isn't actually true. "#foo.bar" is a valid selector, and can even be useful
for matching different elements across multiple pages.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:195: return ++matches;
Style nit: matches + 1, since you aren't actually mutating the variable.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:212: return --matches;
Style nit: matches - 1

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:223: validateTemplate(List<Node> selectors, CssWorld
cssWorld) {
Does this really belong in the parser? It's not actually parsing anything, nor
is it used in the parser.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:227: bool matches = 0;                   // < 0
IdSelectors, > 0 ClassSelector
You're declaring an int as a bool here. Also, it seems very kludgey to use a
variable with such odd semantics. It might be a little more verbose to use
separate id and class counters, but it would be much more comprehensible.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:234: for (className in cssWorld.classes) {
Perhaps cssWorld.classes should be a hash table so you don't have to iterate
over all the classes in the world for each class in each template?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:238: break;
If you break after you find a single valid selector, don't you fail to validate
the remainder of the template?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:269: 'Invalid selector $badSelector', selector.span);
This is a confusing error message; it implies that any non-class/id selector is
invalid, when in fact they're only disallowed in templates.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:273: errorSelector = selector;             // Flag the
problem selector.
If this is the only place you set errorSelector, why not just throw the error
here?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:291: selectorExpression() {
This method doesn't seem to do anything.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:295: simpleSelectorSequence() {
selector()

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:309: combinator() {
simpleSelectorSequence()

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:324: }
You never seem to be parsing a sequence of simple selectors; you just parse a
full selector (that is, multiple sequences of simple selectors separated by
combinators).

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:346: namespaceElementUniversal(Token combinator) {
simpleSelector()

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:359: if (first != null) {
Would be cleaner to return immediately if first == null, rather than indent
everything and have a dangling else.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:364: switch (_peek()) {
I don't like this duplicated logic. Could you check for namespace first, then
only check for universal/element once?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:388: selectorNameType(Token combinator) {
simpleSelectorTail()?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:392: int startHash = _peekToken.start;
Unused variable.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:403: bool pseudoClass = _peek() != TokenKind.PSEUDO;
Seems like parsing : vs :: is a job for the tokenizer.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:409: new PseudoElementSelector(name, combinator);
TODO: Some pseudo-class and -element selectors have parameters.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:417: if (!TokenKind.isIdentifier(tok.kind)) {
Why aren't you just using _eat here?

http://codereview.chromium.org/8498020/diff/1/frog/css/test.dart
File frog/css/test.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/test.dart#newcode37
frog/css/test.dart:37: if (parseOnly) {
} else if (parseOnly) {

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart
File frog/css/tokenizer_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:17: if (_interpStack != null && _interpStack.depth
== 0) {
Unused code

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:83: int getIdentifierKind() {
Not useful, remove

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:96: return ((c >= 97/*a*/ && c <= 122/*z*/) || (c >=
65/*A*/ && c <= 90/*Z*/) || c == 95/*_*/);
Line length (also below)

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart
File frog/css/tree_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode31
frog/css/tree_css.dart:31: List<Node> selectors;
Why isn't this List<SimpleSelector>?

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode46
frog/css/tree_css.dart:46: 
As mentioned elsewhere, there should be a representation of sequences of simple
selectors as distinct from selectors and groups of selectors.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode51
frog/css/tree_css.dart:51: Token _combinator;     // +, >, ~ or NONE (space)
Related to the sequence of simple selector issue, it doesn't make sense to map
simple selectors one-to-one with combinators. ".foo.bar" is different than ".foo
.bar".

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode54
frog/css/tree_css.dart:54: // PseudoClassSelector, PseudoElementSelector,
NotSelector, or Attribute
Is this really true? Aren't all these subclasses of SimpleSelector? Why does it
have a field that's a subclass of itself?

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode61
frog/css/tree_css.dart:61: String get name() => _name is Identifier ? _name.name
: _name;
Ah, so _name is an Identifier or a String. Why does it have two different types?
Is the Identifier form ever used? Don't some simple selectors not really even
have names?

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode78
frog/css/tree_css.dart:78: bool isWildcard() => name == '*';
A universal selector is not really the same thing as an element selector.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode90
frog/css/tree_css.dart:90: [Token combinator = TokenKind.COMBINATOR_NONE]) :
super(name, combinator);
I really don't like the overloading of _name that's going on here. The element
contained by a namespace selector is not its name.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode92
frog/css/tree_css.dart:92: String get namespace() => _namespace is Identifier ?
Same comment as SimpleSelector._name above.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode95
frog/css/tree_css.dart:95: bool isWildcardElement() => _name.isWildcard();
Why is this something that goes through NamespaceSelector?

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart
File utils/css/tokenkind.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart#new...
utils/css/tokenkind.dart:21: static final int COMBINATOR_TILDE = 12;
I don't like how some of these token names encode the semantics of how the token
is used in CSS (NAMESPACE, PSEUDO, PRIVATE_NAME), others refer only to the
character that makes up the token (AT, HASH, ASTERISK, COMMA), and still others
(COMBINATOR_*) include both. I'd say choose one and make it consistent.

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart#new...
utils/css/tokenkind.dart:45: static final int INCOMPLETE_MULTILINE_STRING_DQ =
509;
Unused constants

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart#new...
utils/css/tokenkind.dart:97: TokenKind() {
Why is this a class that gets instantiated? Wouldn't it be just as easy, and
much more efficient and less verbose, to manually assign the relevant character
codes to constants?

[jimhug, 2011-11-10 17:58:56]

This is starting to look really nice!

Take your time responding to Nathan's and my last round of comments - but I
think this is just about ready to go in.

http://codereview.chromium.org/8498020/diff/5019/frog/file_system_memory.dart
File frog/file_system_memory.dart (right):

http://codereview.chromium.org/8498020/diff/5019/frog/file_system_memory.dart...
frog/file_system_memory.dart:11: * [FileSystem] implementation a memory buffer.
This makes a lot of sense to me.  I have some niggling worries about getting too
many [FileSystem]s, but this one seems to fulfill a pretty obvious need.

http://codereview.chromium.org/8498020/diff/5019/frog/presubmit.py
File frog/presubmit.py (right):

http://codereview.chromium.org/8498020/diff/5019/frog/presubmit.py#newcode96
frog/presubmit.py:96: 'peg', 'leg_only', 'frog', 'css'])
Nice tests!

http://codereview.chromium.org/8498020/diff/5019/utils/css/css.dart
File utils/css/css.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/css.dart#newcode25
utils/css/css.dart:25: // Set these here so that we can compile the corelib
without its errors
I'd make this a TODO if you really need it - this seems like something that
should be fixed either in world or here - The REPL needs this for different
reasons.

http://codereview.chromium.org/8498020/diff/5019/utils/css/css.dart#newcode42
utils/css/css.dart:42: String cssParseAndValidateDebug(String cssExpression,
CssWorld world) {
Nice helper.

http://codereview.chromium.org/8498020/diff/5019/utils/css/cssselectorexcepti...
File utils/css/cssselectorexception.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/cssselectorexcepti...
utils/css/cssselectorexception.dart:7: class CssSelectorException implements
Exception {
Hmm...  I think that we should rename compiler exception to SourceException (or
something similar that is less awkward).  The main point of that exception type
to to contain both a message and a location both for printing nicely and uniform
tool support.  I'd add a TODO: Consider renaming CompilerException such that
this would be an appropriate subclass.  I'd also definitely not make this change
itself in this CL to minimize potential conflicts.

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

http://codereview.chromium.org/8498020/diff/5019/utils/css/parser.dart#newcode13
utils/css/parser.dart:13: // TODO(jimhug): 1. Try to kill initializers, if
fails, clean this up.
Field and comment shouldn't be needed for css.

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart
File utils/css/test.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart#newcode43
utils/css/test.dart:43: Parser parser = new Parser(new
lang.SourceFile(lang.SourceFile.IN_MEMORY_FILE,
Nit: line length.

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart#newcode68
utils/css/test.dart:68: var bgcolor;
Style suggestion.  I believe it is best to either type or initialize locals - as
a form of documentation.  You could change the types of these to String, or try
this form:

final bgcolor = templateValid ? "white" : "red";
...

Or, if you don't like ?:, try this one.

var bgcolor = "white";
...
if (!templateValid) {
  bgcolor = "red";
  ...
}

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart#newcode113
utils/css/test.dart:113: // TODO(terry): Needed so runCss isn't shakened out.
Hmm...  This is unfortunate - but matches the same hack we have in tip.  I'll
keep thinking about the right longer-term fix for this.

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenizer.dart
File utils/css/tokenizer.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenizer.dart#new...
utils/css/tokenizer.dart:20: /*
Is this relevant at all for css?

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenizer.dart#new...
utils/css/tokenizer.dart:109: class TokenizerHelpers {
Any reason not to use lang.TokenizerHelpers here?  It looks like you'd just need
to implement
  isCssIdentifierPart(int c) => TokenizerHelpers.isIdentifierPart(c) || c ==
45/*-*/;
But, maybe I'm missing something...

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

http://codereview.chromium.org/8498020/diff/5019/utils/css/tree.dart#newcode11
utils/css/tree.dart:11: class Identifier extends lang.Node {
I think this is really cool - sharing the same basic tree Node.  I hope this
works out well going forward.

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/css.status
File utils/tests/css/css.status (right):

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/css.status#n...
utils/tests/css/css.status:5: prefix css
I don't understand why you need to specify so much stuff in this file.  Wouldn't
just the skips be enough?

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/src/Expressi...
File utils/tests/css/src/ExpressionTest.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/src/Expressi...
utils/tests/css/src/ExpressionTest.dart:26: Parser parser0 = new Parser(new
lang.SourceFile(
Am I missing something, or could you split this into two methods and avoid the
need for parser0/parser1 and exprTree0/exprTree1?

[terry, 2011-11-16 14:00:22]

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart
File frog/css/parser_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode16
frog/css/parser_css.dart:16: bool _inInitializers;
On 2011/11/10 00:04:39, nweiz wrote:
> Unused variable.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode66
frog/css/parser_css.dart:66: return TokenKind.isIdentifier(_peekToken.kind);
This is mimicked after frog's parser.

On 2011/11/10 00:04:39, nweiz wrote:
> Why can't you use _peekKind(TokenKind.IDENTIFIER)?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode85
frog/css/parser_css.dart:85: void _eatSemicolon() {
Unused now will use later.

On 2011/11/10 00:04:39, nweiz wrote:
> Unused method.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:130: List<SelectorGroup> expression([bool throwErrors =
false]) {
I see your point, expression was probably a bad name it's currently used as a
parser test of the real CSS versus template which is the CSS literals @{...}.  A
better name would be preprocess since this will be the entry point for
pre-processing the CSS file.

On 2011/11/10 00:04:39, nweiz wrote:
> I think your terminology is confusing here. "expression" in particular is
> ambiguous with expressions in property values, but "SelectorGroup" and
> "selectorExpression" are also non-standard. The CSS spec divides selectors up
as
> follows:
> 
> foo.bar baz, beep
> \A/
> \--B--/
> \----C----/
> \-------D-------/
> 
> A is referred to as a "simple selector"; B as a "sequence of simple
selectors";
> C as a "selector"; and D as a "group of selectors".
> 
> In keeping with the CSS terminology, this method should probably be called
> "selectorGroup" and what's not "SelectorGroup" should just be "Selector".

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:164: if (_peek() == TokenKind.AT) {

On 2011/11/10 00:04:39, nweiz wrote:
> Is there any reason to do all these if checks, rather than just using _eat and
> using the built-in error handling? These deeply nested if statements are
pretty
> ugly.

Main reason was more exact error.  However, I think simplicity is better for now
I'll worry about better error messages/recovery later.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:190: 'Can not mix Id selector with class selector(s).
Id ' +
Of course, however this is used by the validator for css literals @{...} in that
context this is not a valid selector.

On 2011/11/10 00:04:39, nweiz wrote:
> This isn't actually true. "#foo.bar" is a valid selector, and can even be
useful
> for matching different elements across multiple pages.

http://codereview.chromium.org/8498020/diff/5019/utils/css/css.dart
File utils/css/css.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/css.dart#newcode25
utils/css/css.dart:25: // Set these here so that we can compile the corelib
without its errors
On 2011/11/10 17:58:56, jimhug wrote:
> I'd make this a TODO if you really need it - this seems like something that
> should be fixed either in world or here - The REPL needs this for different
> reasons.

This comment was wrong.  I've updated the comment with a TODO that throwOnXXX
should be set with arguments to support running as a tool vs/ programmatically.
Done.

http://codereview.chromium.org/8498020/diff/5019/utils/css/cssselectorexcepti...
File utils/css/cssselectorexception.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/cssselectorexcepti...
utils/css/cssselectorexception.dart:7: class CssSelectorException implements
Exception {
On 2011/11/10 17:58:56, jimhug wrote:
> Hmm...  I think that we should rename compiler exception to SourceException
(or
> something similar that is less awkward).  The main point of that exception
type
> to to contain both a message and a location both for printing nicely and
uniform
> tool support.  I'd add a TODO: Consider renaming CompilerException such that
> this would be an appropriate subclass.  I'd also definitely not make this
change
> itself in this CL to minimize potential conflicts.

Done.

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

http://codereview.chromium.org/8498020/diff/5019/utils/css/parser.dart#newcode13
utils/css/parser.dart:13: // TODO(jimhug): 1. Try to kill initializers, if
fails, clean this up.
On 2011/11/10 17:58:56, jimhug wrote:
> Field and comment shouldn't be needed for css.

Done.

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart
File utils/css/test.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart#newcode43
utils/css/test.dart:43: Parser parser = new Parser(new
lang.SourceFile(lang.SourceFile.IN_MEMORY_FILE,
On 2011/11/10 17:58:56, jimhug wrote:
> Nit: line length.

Done.

http://codereview.chromium.org/8498020/diff/5019/utils/css/test.dart#newcode68
utils/css/test.dart:68: var bgcolor;
On 2011/11/10 17:58:56, jimhug wrote:
> Style suggestion.  I believe it is best to either type or initialize locals -
as
> a form of documentation.  You could change the types of these to String, or
try
> this form:
> 
> final bgcolor = templateValid ? "white" : "red";
> ...
> 
> Or, if you don't like ?:, try this one.
> 
> var bgcolor = "white";
> ...
> if (!templateValid) {
>   bgcolor = "red";
>   ...
> }

Done.

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenizer.dart
File utils/css/tokenizer.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenizer.dart#new...
utils/css/tokenizer.dart:20: /*
I think we might want this for string interpolation in CSS however, it's not
used yet.

$type = 'gif'
url('image.${type}')  or   url('image.#{$type}')

or

div::before {
  content: "Chapter ${counter(chapter)} : -- "
}

for long strings but this might be rare.  My goal is to have a consistent
interpolation in CSS, Dart, HTML, etc. 

We could allow intrinsic CSS function calls inside of the curlys too with
parameters passed.

However, the commented out code will be removed for now.
On 2011/11/10 17:58:56, jimhug wrote:
> Is this relevant at all for css?

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenizer.dart#new...
utils/css/tokenizer.dart:109: class TokenizerHelpers {
On 2011/11/10 17:58:56, jimhug wrote:
> Any reason not to use lang.TokenizerHelpers here?  It looks like you'd just
need
> to implement
>   isCssIdentifierPart(int c) => TokenizerHelpers.isIdentifierPart(c) || c ==
> 45/*-*/;
> But, maybe I'm missing something...

Good point.
Done.

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/css.status
File utils/tests/css/css.status (right):

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/css.status#n...
utils/tests/css/css.status:5: prefix css
On 2011/11/10 17:58:56, jimhug wrote:
> I don't understand why you need to specify so much stuff in this file. 
Wouldn't
> just the skips be enough?
For completeness I added all areas but I'll remove those and leave that as an
exercise for folks writing the tests with problems.
Done.

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/src/Expressi...
File utils/tests/css/src/ExpressionTest.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/tests/css/src/Expressi...
utils/tests/css/src/ExpressionTest.dart:26: Parser parser0 = new Parser(new
lang.SourceFile(
I don't expect this to be how it's used but I've hooked this up for now to test
the current completeness of the CSS selector parsing outside of @{} syntax.  As
I add the next part of full CSS I'll change this.  The entry point expression is
 also not quite right yet other than it's a different entry point to test
general parsing.

On 2011/11/10 17:58:56, jimhug wrote:
> Am I missing something, or could you split this into two methods and avoid the
> need for parser0/parser1 and exprTree0/exprTree1?

[jimhug, 2011-11-17 16:49:27]

LGTM!

And welcome back!

http://codereview.chromium.org/8498020/diff/17001/utils/css/test.dart
File utils/css/test.dart (right):

http://codereview.chromium.org/8498020/diff/17001/utils/css/test.dart#newcode68
utils/css/test.dart:68: final String bgcolor = templateValid ? "white" : "red";
Nit: The String type is not needed for a final declaration.  I think that both
the human and certainly the compiler/tools can figure this one out pretty
easily.

[terry, 2011-11-22 16:40:47]

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart
File frog/css/parser_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:149: if (!_maybeEat(TokenKind.END_OF_FILE)) {
On 2011/11/10 00:04:39, nweiz wrote:
> Can you just call isPrematureEndOfFile?

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:195: return ++matches;
On 2011/11/10 00:04:39, nweiz wrote:
> Style nit: matches + 1, since you aren't actually mutating the variable.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:212: return --matches;
On 2011/11/10 00:04:39, nweiz wrote:
> Style nit: matches - 1

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:223: validateTemplate(List<Node> selectors, CssWorld
cssWorld) {
Will be moving this out in the next checkin.

On 2011/11/10 00:04:39, nweiz wrote:
> Does this really belong in the parser? It's not actually parsing anything, nor
> is it used in the parser.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:227: bool matches = 0;                   // < 0
IdSelectors, > 0 ClassSelector
On 2011/11/10 00:04:39, nweiz wrote:
> You're declaring an int as a bool here. Also, it seems very kludgey to use a
> variable with such odd semantics. It might be a little more verbose to use
> separate id and class counters, but it would be much more comprehensible.

Thanks, caught this earlier when I merged with the new type checking code in
frog.
Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:234: for (className in cssWorld.classes) {
You're probably right but I've err'd on the side of simplicity for now.  Will
revisit soon.  I'll add a TODO.

On 2011/11/10 00:04:39, nweiz wrote:
> Perhaps cssWorld.classes should be a hash table so you don't have to iterate
> over all the classes in the world for each class in each template?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:238: break;
No each selector is validated if any selector isn't found the selector with the
error is flagged and the error is reported.  However, better comments could help
to solve this confusion I'll add more comments.
Thx.
On 2011/11/10 00:04:39, nweiz wrote:
> If you break after you find a single valid selector, don't you fail to
validate
> the remainder of the template?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:269: 'Invalid selector $badSelector', selector.span);
On 2011/11/10 00:04:39, nweiz wrote:
> This is a confusing error message; it implies that any non-class/id selector
is
> invalid, when in fact they're only disallowed in templates.

Good point I'll change the error.
Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:273: errorSelector = selector;             // Flag the
problem selector.
On 2011/11/10 00:04:39, nweiz wrote:
> If this is the only place you set errorSelector, why not just throw the error
> here?

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:291: selectorExpression() {
It's gone.
On 2011/11/10 00:04:39, nweiz wrote:
> This method doesn't seem to do anything.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:295: simpleSelectorSequence() {
On 2011/11/10 00:04:39, nweiz wrote:
> selector()

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:309: combinator() {
On 2011/11/10 00:04:39, nweiz wrote:
> simpleSelectorSequence()

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:324: }
I had't completed the code but have now so sequence of simple selectors is now
covered.

On 2011/11/10 00:04:39, nweiz wrote:
> You never seem to be parsing a sequence of simple selectors; you just parse a
> full selector (that is, multiple sequences of simple selectors separated by
> combinators).

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:346: namespaceElementUniversal(Token combinator) {
On 2011/11/10 00:04:39, nweiz wrote:
> simpleSelector()

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:359: if (first != null) {
On 2011/11/10 00:04:39, nweiz wrote:
> Would be cleaner to return immediately if first == null, rather than indent
> everything and have a dangling else.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:364: switch (_peek()) {
For now I like the element vs/ namespace parsing I'll add a TODO to consolidate.

On 2011/11/10 00:04:39, nweiz wrote:
> I don't like this duplicated logic. Could you check for namespace first, then
> only check for universal/element once?

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:388: selectorNameType(Token combinator) {
On 2011/11/10 00:04:39, nweiz wrote:
> simpleSelectorTail()?

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:392: int startHash = _peekToken.start;
On 2011/11/10 00:04:39, nweiz wrote:
> Unused variable.

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:403: bool pseudoClass = _peek() != TokenKind.PSEUDO;
On 2011/11/10 00:04:39, nweiz wrote:
> Seems like parsing : vs :: is a job for the tokenizer.
Your right.  I'll fix in the next checkin.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:409: new PseudoElementSelector(name, combinator);
Yep, I haven't finished this yet I'll add a TODO.

On 2011/11/10 00:04:39, nweiz wrote:
> TODO: Some pseudo-class and -element selectors have parameters.

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:417: if (!TokenKind.isIdentifier(tok.kind)) {
Why for better error recovery?  Currently, error recovery is poor in frog and
css parsing.  However, did clean up the error handling didn't need two calls to
_error.

On 2011/11/10 00:04:39, nweiz wrote:
> Why aren't you just using _eat here?

http://codereview.chromium.org/8498020/diff/1/frog/css/test.dart
File frog/css/test.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/test.dart#newcode37
frog/css/test.dart:37: if (parseOnly) {
On 2011/11/10 00:04:39, nweiz wrote:
> } else if (parseOnly) {

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart
File frog/css/tokenizer_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:17: if (_interpStack != null && _interpStack.depth
== 0) {
I expect to be using this in next checkin.
On 2011/11/10 00:04:39, nweiz wrote:
> Unused code

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:83: int getIdentifierKind() {
This will be used real soon with other known idents.

On 2011/11/10 00:04:39, nweiz wrote:
> Not useful, remove

http://codereview.chromium.org/8498020/diff/1/frog/css/tokenizer_css.dart#new...
frog/css/tokenizer_css.dart:96: return ((c >= 97/*a*/ && c <= 122/*z*/) || (c >=
65/*A*/ && c <= 90/*Z*/) || c == 95/*_*/);
On 2011/11/10 00:04:39, nweiz wrote:
> Line length (also below)

Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart
File frog/css/tree_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode31
frog/css/tree_css.dart:31: List<Node> selectors;
On 2011/11/10 00:04:39, nweiz wrote:
> Why isn't this List<SimpleSelector>?
Should be.
Done.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode46
frog/css/tree_css.dart:46: 
Yep added combinator descendant (space) and NONE.
On 2011/11/10 00:04:39, nweiz wrote:
> As mentioned elsewhere, there should be a representation of sequences of
simple
> selectors as distinct from selectors and groups of selectors.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode51
frog/css/tree_css.dart:51: Token _combinator;     // +, >, ~ or NONE (space)
I have for now maybe I'm being too pedantic (even though the syntax doesn't
require it) I'll recheck why I thought I needed this.
On 2011/11/10 00:04:39, nweiz wrote:
> Related to the sequence of simple selector issue, it doesn't make sense to map
> simple selectors one-to-one with combinators. ".foo.bar" is different than
".foo
> .bar".

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode54
frog/css/tree_css.dart:54: // PseudoClassSelector, PseudoElementSelector,
NotSelector, or Attribute
No comment is outdated.
On 2011/11/10 00:04:39, nweiz wrote:
> Is this really true? Aren't all these subclasses of SimpleSelector? Why does
it
> have a field that's a subclass of itself?

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode61
frog/css/tree_css.dart:61: String get name() => _name is Identifier ? _name.name
: _name;
It's an identifier or wildcard nothing else String was bad I eventually removed
that.
On 2011/11/10 00:04:39, nweiz wrote:
> Ah, so _name is an Identifier or a String. Why does it have two different
types?
> Is the Identifier form ever used? Don't some simple selectors not really even
> have names?

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode78
frog/css/tree_css.dart:78: bool isWildcard() => name == '*';
Your right I removed it when I added notion of wildcard for name.
On 2011/11/10 00:04:39, nweiz wrote:
> A universal selector is not really the same thing as an element selector.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode90
frog/css/tree_css.dart:90: [Token combinator = TokenKind.COMBINATOR_NONE]) :
super(name, combinator);
Right it's an identifier or wildcard.
On 2011/11/10 00:04:39, nweiz wrote:
> I really don't like the overloading of _name that's going on here. The element
> contained by a namespace selector is not its name.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode92
frog/css/tree_css.dart:92: String get namespace() => _namespace is Identifier ?
namespace is never a string either Identifier or Wildcard.
On 2011/11/10 00:04:39, nweiz wrote:
> Same comment as SimpleSelector._name above.

http://codereview.chromium.org/8498020/diff/1/frog/css/tree_css.dart#newcode95
frog/css/tree_css.dart:95: bool isWildcardElement() => _name.isWildcard();
Shouldn't it's on simpleSelector.
On 2011/11/10 00:04:39, nweiz wrote:
> Why is this something that goes through NamespaceSelector?

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart
File utils/css/tokenkind.dart (right):

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart#new...
utils/css/tokenkind.dart:21: static final int COMBINATOR_TILDE = 12;
You have a valid point I'll clean this up next checkin.  Added TODO.
On 2011/11/10 00:04:40, nweiz wrote:
> I don't like how some of these token names encode the semantics of how the
token
> is used in CSS (NAMESPACE, PSEUDO, PRIVATE_NAME), others refer only to the
> character that makes up the token (AT, HASH, ASTERISK, COMMA), and still
others
> (COMBINATOR_*) include both. I'd say choose one and make it consistent.

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart#new...
utils/css/tokenkind.dart:45: static final int INCOMPLETE_MULTILINE_STRING_DQ =
509;
I think I'll be using these soon enough.
On 2011/11/10 00:04:40, nweiz wrote:
> Unused constants

http://codereview.chromium.org/8498020/diff/5019/utils/css/tokenkind.dart#new...
utils/css/tokenkind.dart:97: TokenKind() {
For now I'm trying to mimic frog w/o the Python class creations.
On 2011/11/10 00:04:40, nweiz wrote:
> Why is this a class that gets instantiated? Wouldn't it be just as easy, and
> much more efficient and less verbose, to manually assign the relevant
character
> codes to constants?

[nweiz, 2011-11-23 00:59:44]

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart
File frog/css/parser_css.dart (right):

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode66
frog/css/parser_css.dart:66: return TokenKind.isIdentifier(_peekToken.kind);
Frog's parser needs it because there are multiple tokens that count as
identifiers. That's not the case in CSS.

On 2011/11/16 14:00:22, terry wrote:
> This is mimicked after frog's parser.
> 
> On 2011/11/10 00:04:39, nweiz wrote:
> > Why can't you use _peekKind(TokenKind.IDENTIFIER)?
>

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcode85
frog/css/parser_css.dart:85: void _eatSemicolon() {
I don't think it'll be useful, actually... there aren't any places in CSS where
a semicolon is mandatory; it could always just be the end of a block instead.

On 2011/11/16 14:00:22, terry wrote:
> Unused now will use later.
> 
> On 2011/11/10 00:04:39, nweiz wrote:
> > Unused method.
>

http://codereview.chromium.org/8498020/diff/1/frog/css/parser_css.dart#newcod...
frog/css/parser_css.dart:190: 'Can not mix Id selector with class selector(s).
Id ' +
On 2011/11/16 14:00:22, terry wrote:
> Of course, however this is used by the validator for css literals @{...} in
that
> context this is not a valid selector.
> 
> On 2011/11/10 00:04:39, nweiz wrote:
> > This isn't actually true. "#foo.bar" is a valid selector, and can even be
> useful
> > for matching different elements across multiple pages.
> 

Why aren't we allowing arbitrary selectors in selector literals? Allowing most
selectors but banning a few is bound to be confusing for users.

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

http://codereview.chromium.org/8498020/diff/30018/utils/css/parser.dart#newco...
utils/css/parser.dart:109: List<SelectorGroup> preprocess() {
I don't like "preprocess" here either. This method doesn't actually do any
processing, it just parses. It would be consistent with the dart parser and the
rest of this parser to name this after the production that it parses (that is,
"selectorGroup").

http://codereview.chromium.org/8498020/diff/30018/utils/css/parser.dart#newco...
utils/css/parser.dart:185: var errorSelector;                  // signal which
selector didn't match.
Unused variable.

http://codereview.chromium.org/8498020/diff/30018/utils/css/parser.dart#newco...
utils/css/parser.dart:264: simpleSelectorSequence(bool forceCombinatorNone) {
While this technically works by treating sequences of simple selectors as though
they were separated by a special combinator, the parse tree is going to look
pretty different from how selectors are described in the spec. ".foo.bar .baz"
isn't three simple selectors separated by two combinators; it's two sequences of
simple selectors separated by one combinator. It should parse as so:

[SimpleSelectorSequence([
     SimpleSelector(.foo),
     SimpleSelector(.bar)
   ], TokenKind.COMBINATOR_NONE),
 SimpleSelectorSequence(
   [SimpleSelector.baz],
   TokenKind.COMBINATOR_DESCENDANT
 )]

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

http://codereview.chromium.org/8498020/diff/30018/utils/css/tree.dart#newcode29
utils/css/tree.dart:29: class SelectorGroup extends lang.Node {
As per discussion of selector hierarchy elsewhere in this CL and over email:

A "selector group" is a comma-separated list of "selectors". The class should
contain a List<Selector>.

A "selector" is a combinator-separated list of "sequences of simple selectors".
The class should contain a List<SimpleSelectorSequence>.

A "sequence of simple selectors" is a list of "simple selectors". The class
should contain a combinator and a List<SimpleSelector>.

A "simple selector" is basically what you have here, without the combinator.

http://codereview.chromium.org/8498020/diff/30018/utils/css/tree.dart#newcode77
utils/css/tree.dart:77: String get name() => isWildcard() ? '*' : _name.name;
I don't understand the need to have a "name" property for simple selectors. If
you want a string representation, use toString. Sure, most simple selectors do
have names, but it's not a necessary aspect of them.

The semantics of the name also differ from subclass to subclass. Having a
wildcard for a name only makes sense for ElementSelector, for example.

I suggest making name a property of each subclass that needs it individually.

http://codereview.chromium.org/8498020/diff/30018/utils/css/tree.dart#newcode79
utils/css/tree.dart:79: bool isWildcard() => _name is Wildcard;
If you're going to have a separate Wildcard class for _name, you might as well
just return that for the name property rather than coercing it to a string. That
was the user can use the more-modular "sel.name is Wildcard" and "sel.namespace
is Wildcard".

http://codereview.chromium.org/8498020/diff/30018/utils/css/tree.dart#newcode119
utils/css/tree.dart:119: class NamespaceSelector extends SimpleSelector {
Namespaces aren't separate simple selectors. A namespace is a property of
element selectors (and universal selectors, but it looks like you're parsing
those as element selectors). I suggest removing this class and adding a
namespace field to ElementSelector.