Fix 19029: handle special case when on-handler is empty in polymer, add better

Fix 19029: handle special case when on-handler is empty in polymer, add better
validation of symbols in smoke.

BUG=https://code.google.com/p/dart/issues/detail?id=19029
R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2014-05-29 00:05:18]

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/test/build/s...
File pkg/polymer/test/build/script_compactor_test.dart (right):

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/test/build/s...
pkg/polymer/test/build/script_compactor_test.dart:664: '<input
value="{{this.iS.twoWay}}">'
"is" is a reserved word, so now smoke made this part of the test fail.

[Jennifer Messerly, 2014-05-29 00:27:42]

one comment in linter, but otherwise LGTM

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:370: _logger.warning('Event handler
"$name" will be interpreted as an inline'
hmmm. at some point we might consider loosening this, since it's warning about
valid HTML attributes (for better or worse)...

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:384: if (value == '' || value == "{{}}" ||
value.contains('(')) {
hmmm, not sure about this one ...

any reason to special case empty string? should this just warn any time it
doesn't start with {{ and end with }} ?

also curlies can have whitespace inside, AFAIK. but i'm looking at the "new"
code so maybe the .trim() call is an addition...

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:387: 'and use the form
$name="{{handlerName}}".',
nice catch!

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:516: if
(!stringExpression.startsWith("@")) {
fyi -- should be trimming this if we aren't already

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/test/build/s...
File pkg/polymer/test/build/script_compactor_test.dart (right):

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/test/build/s...
pkg/polymer/test/build/script_compactor_test.dart:664: '<input
value="{{this.iS.twoWay}}">'
On 2014/05/29 00:05:18, Siggi Cherem (dart-lang) wrote:
> "is" is a reserved word, so now smoke made this part of the test fail.

interesting! longer term we might have an issue there. filed
https://code.google.com/p/dart/issues/detail?id=19069

[Siggi Cherem (dart-lang), 2014-05-29 01:04:21]

ptal

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:370: _logger.warning('Event handler
"$name" will be interpreted as an inline'
On 2014/05/29 00:27:42, John Messerly wrote:
> hmmm. at some point we might consider loosening this, since it's warning about
> valid HTML attributes (for better or worse)...

yeah, good point. Added TODO for now.

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:384: if (value == '' || value == "{{}}" ||
value.contains('(')) {
On 2014/05/29 00:27:42, John Messerly wrote:
> hmmm, not sure about this one ...
> 
> any reason to special case empty string? should this just warn any time it
> doesn't start with {{ and end with }} ?

Good point. Done.

 
> also curlies can have whitespace inside, AFAIK. but i'm looking at the "new"
> code so maybe the .trim() call is an addition...

added .trim since at this stage we haven't parsed the mustache expressions.

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/303003003/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:516: if
(!stringExpression.startsWith("@")) {
On 2014/05/29 00:27:42, John Messerly wrote:
> fyi -- should be trimming this if we aren't already

good question - these are being trimmed in the mustache parser, I added a couple
test cases to make sure we don't regress.

[Jennifer Messerly, 2014-05-29 02:39:14]

lgtm * 2

[Siggi Cherem (dart-lang), 2014-05-29 03:28:48]

Committed patchset #2 manually as r36783 (presubmit successful).