pkg/csslib: fixed analysis error, more cleanup

pkg/csslib: fixed analysis error, more cleanup

R=terry@google.com

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

[kevmoo-old, 2013-11-07 18:10:28]

tests pass in checked mode
analyzer is warning free
csslib-test-suite is green

[terry, 2013-11-07 18:11:22]

Please make sure it passes csslib test and csslib test suite (~10,000) tests.

Few changes.

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/parser.dart
File pkg/csslib/lib/parser.dart (right):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/parser.dar...
pkg/csslib/lib/parser.dart:941: List<TreeNode> params = [];
Should stay as var.

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/parser.dar...
pkg/csslib/lib/parser.dart:949: List terms = [];
Should stay as var.

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.dart
File pkg/csslib/lib/src/tree.dart (left):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.d...
pkg/csslib/lib/src/tree.dart:130: SimpleSelector clone() => new
SimpleSelector(_name, span);
Why is clone removed?  IdSelector, ClassSelector, etc. needs clone too.

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.dart
File pkg/csslib/lib/src/tree.dart (right):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.d...
pkg/csslib/lib/src/tree.dart:282: void visit(VisitorBase visitor) =>
visitor.visitPseudoClassFunctionSelector(this);
>80

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/visitor.dart
File pkg/csslib/lib/visitor.dart (right):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/visitor.da...
pkg/csslib/lib/visitor.dart:114: void _visitNodeList(List<TreeNode> list) {
Should be List.

[terry, 2013-11-07 18:17:05]

Few more comments.

https://codereview.chromium.org/60983003/diff/50001/pkg/csslib/lib/parser.dart
File pkg/csslib/lib/parser.dart (right):

https://codereview.chromium.org/60983003/diff/50001/pkg/csslib/lib/parser.dar...
pkg/csslib/lib/parser.dart:958: params.add(terms);
Good I thought it should stay as add.

https://codereview.chromium.org/60983003/diff/50001/pkg/csslib/lib/src/tree_p...
File pkg/csslib/lib/src/tree_printer.dart (right):

https://codereview.chromium.org/60983003/diff/50001/pkg/csslib/lib/src/tree_p...
pkg/csslib/lib/src/tree_printer.dart:175: var flattened = node.args.expand((e)
=> e).toList();
Leave writeNodeList and writeList to take a List much simpler usage then having
to remember to flatten, etc.

[kevmoo-old, 2013-11-07 18:23:18]

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.dart
File pkg/csslib/lib/src/tree.dart (left):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.d...
pkg/csslib/lib/src/tree.dart:130: SimpleSelector clone() => new
SimpleSelector(_name, span);
Class was made abstract.

Subclasses now clone the correct type, which is what clone should do, no?

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.dart
File pkg/csslib/lib/src/tree.dart (right):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/src/tree.d...
pkg/csslib/lib/src/tree.dart:282: void visit(VisitorBase visitor) =>
visitor.visitPseudoClassFunctionSelector(this);
On 2013/11/07 18:11:22, terry wrote:
> >80

Done.

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/visitor.dart
File pkg/csslib/lib/visitor.dart (right):

https://codereview.chromium.org/60983003/diff/30001/pkg/csslib/lib/visitor.da...
pkg/csslib/lib/visitor.dart:114: void _visitNodeList(List<TreeNode> list) {
On 2013/11/07 18:11:22, terry wrote:
> Should be List.

It's assumed all items in the list have a 'visit' method, no? Implies TreeNode?

https://codereview.chromium.org/60983003/diff/50001/pkg/csslib/lib/src/tree_p...
File pkg/csslib/lib/src/tree_printer.dart (right):

https://codereview.chromium.org/60983003/diff/50001/pkg/csslib/lib/src/tree_p...
pkg/csslib/lib/src/tree_printer.dart:175: var flattened = node.args.expand((e)
=> e).toList();
On 2013/11/07 18:17:05, terry wrote:
> Leave writeNodeList and writeList to take a List much simpler usage then
having
> to remember to flatten, etc.

writeNodeList assumes each element in list hase a 'visit' method, no?

I don't see other uses of writeList, so I'm removing it

[kevmoo-old, 2013-11-16 22:39:08]

Rebased and tested. PTAL?

[terry, 2013-11-20 14:07:25]

LGTM just a few more cleanups.  Runs with css test and CSS test suite too?

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
File pkg/csslib/lib/parser.dart (right):

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:49: bool nested: true,
keep rest of params on same line indent +4

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:77: {List<Message> errors, List<String> options}) {
indent + 4

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:114: String source;
var source;

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1193: List<SimpleSelectorSequence> simpleSequences =
[];
var simpleSequences = [];

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1194: int start = _peekToken.start;
var start - _peekToken.start;

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1287: int start = _peekToken.start;
var start = ...

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1352: int start = _peekToken.start;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1357: bool hasWhiteSpace = false;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1403: bool pseudoElement =
_maybeEat(TokenKind.COLON);
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1464: int start = _peekToken.start;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1468: Token termToken;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1475: bool keepParsing = true;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1548: int start = _peekToken.start;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1553: int op;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1602: Declaration decl;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1645: List<SimpleSelectorSequence> simpleSequences =
[];
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1738: static _findStyle(String styleName) =>
_stylesToDart[styleName];
return type signature int

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1740: DartStyleExpression _styleForDart(Identifier
property, Expressions exprs, List dartStyles) {
new line after exprs param and indent +4 for remainder args.

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1741: int styleType =
_findStyle(property.name.toLowerCase());
var styleType

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:1759: List dartStyles) {
indent only +4 for new line.

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2006: Expression processExpr([bool ieFilter = false])
{
returns Expressions

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2007: int start = _peekToken.start;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2008: Expressions expressions = new
Expressions(_makeSpan(start));
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2010: bool keepGoing = true;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2091: int start = _peekToken.start;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2092: Token t;                          // token for
term's value
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2199: String rgbColor = TokenKind.decimalToHex(
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2255: LiteralTerm term;
style is to use var.

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2335: int start = _peekToken.start;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2492: if(c >= 48/*0*/ && c <= 57/*9*/) {
space between if and (

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2557: LengthTerm size;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2558: LineHeight lineHt;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2559: bool nextIsLineHeight = false;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2587: final List<String> family = <String>[];
var family = [];

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2619: FontExpression fontSize;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2620: FontExpression fontFamily;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2648: StringBuffer result = null;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/pa...
pkg/csslib/lib/parser.dart:2651: int code = text.codeUnitAt(i);
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
File pkg/csslib/lib/src/css_printer.dart (right):

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/css_printer.dart:107: int declsMarginLength =
declsMargin.length;
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/css_printer.dart:122: bool isStartingQuote(String ch) =>
('\'"'.indexOf(ch[0]) >= 0);
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/css_printer.dart:168: bool isStartingQuote(String ch) =>
('\'"'.indexOf(ch) >= 0);
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/css_printer.dart:250: String importantAsString() =>
node.important ? '$_sp!important' : '';
var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
File pkg/csslib/lib/src/tree.dart (right):

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/tree.dart:1202: //              support calc expressions on
units.
Add TODO above constructor and get rid of body.

[terry, 2013-11-20 16:45:08]

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
File pkg/csslib/lib/src/css_printer.dart (right):

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/css_printer.dart:168: bool isStartingQuote(String ch) =>
('\'"'.indexOf(ch) >= 0);
ignore.
On 2013/11/20 14:07:25, terry wrote:
> var

https://chromiumcodereview.appspot.com/60983003/diff/300001/pkg/csslib/lib/sr...
pkg/csslib/lib/src/css_printer.dart:250: String importantAsString() =>
node.important ? '$_sp!important' : '';
ignore
On 2013/11/20 14:07:25, terry wrote:
> var

[kevmoo-old, 2013-11-20 19:13:10]

I think I got most of the nits.

See comments about using var w/out an assignment.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.dart
File pkg/csslib/lib/parser.dart (right):

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:49: bool nested: true,
On 2013/11/20 14:07:25, terry wrote:
> keep rest of params on same line indent +4

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:77: {List<Message> errors, List<String> options}) {
On 2013/11/20 14:07:25, terry wrote:
> indent + 4

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:114: String source;
On 2013/11/20 14:07:25, terry wrote:
> var source;

Since 'source' is not assigned immediately, the type cannot be inferred. I
generally leave the type in these cases.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1193: List<SimpleSelectorSequence> simpleSequences =
[];
On 2013/11/20 14:07:25, terry wrote:
> var simpleSequences = [];

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1194: int start = _peekToken.start;
On 2013/11/20 14:07:25, terry wrote:
> var start - _peekToken.start;

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1352: int start = _peekToken.start;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1357: bool hasWhiteSpace = false;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1403: bool pseudoElement =
_maybeEat(TokenKind.COLON);
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1464: int start = _peekToken.start;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1468: Token termToken;
On 2013/11/20 14:07:25, terry wrote:
> var

Leaving type since it's not inferred from a first assign.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1475: bool keepParsing = true;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1553: int op;
On 2013/11/20 14:07:25, terry wrote:
> var

Not assigned. Keeping type.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1602: Declaration decl;
On 2013/11/20 14:07:25, terry wrote:
> var

Not assigned, keeping type.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1645: List<SimpleSelectorSequence> simpleSequences =
[];
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1738: static _findStyle(String styleName) =>
_stylesToDart[styleName];
On 2013/11/20 14:07:25, terry wrote:
> return type signature int

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1740: DartStyleExpression _styleForDart(Identifier
property, Expressions exprs, List dartStyles) {
On 2013/11/20 14:07:25, terry wrote:
> new line after exprs param and indent +4 for remainder args.

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1741: int styleType =
_findStyle(property.name.toLowerCase());
On 2013/11/20 14:07:25, terry wrote:
> var styleType

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:1759: List dartStyles) {
On 2013/11/20 14:07:25, terry wrote:
> indent only +4 for new line.

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:2006: Expression processExpr([bool ieFilter = false])
{
On 2013/11/20 14:07:25, terry wrote:
> returns Expressions

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:2007: int start = _peekToken.start;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:2008: Expressions expressions = new
Expressions(_makeSpan(start));
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:2010: bool keepGoing = true;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:2091: int start = _peekToken.start;
On 2013/11/20 14:07:25, terry wrote:
> var

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/parser.da...
pkg/csslib/lib/parser.dart:2492: if(c >= 48/*0*/ && c <= 57/*9*/) {
On 2013/11/20 14:07:25, terry wrote:
> space between if and (

Done.

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/src/tree....
File pkg/csslib/lib/src/tree.dart (right):

https://codereview.chromium.org/60983003/diff/300001/pkg/csslib/lib/src/tree....
pkg/csslib/lib/src/tree.dart:1202: //              support calc expressions on
units.
On 2013/11/20 14:07:25, terry wrote:
> Add TODO above constructor and get rid of body.

Done.

[kevmoo-old, 2013-11-20 19:15:48]

Committed patchset #7 manually as r30480 (presubmit successful).