fixes to polymer, gets tests back to a stable state

Several fixes here to fix bugs and get our tests into a reliable state:

* customElementsReady future. We don't fire Polymer.onReady until custom elements are ready too

* deploy without dart.js by default. This is the expected deployment mode for real apps. Also make sure we test this on DRT and compiler==js, so Pete can change Dartium without causing build breaks :)

* stop using "package:observe" test "performMicrotaskCheckpoint" hack in Polymer. This was too blunt; it affected timing of all async operations. Instead we only trigger Observable.dirtyCheck. It's using the latest Zone API as well.

* mixin ChangeNotifier explicitly. This is mainly to fix a quirk of our test environment (test server loads "packages/polymer" in untransformed mode, causing it to pick up Observable instead of ChangeNotifier).

* fix change watchers to blacklist "attributeChanged" :)

* fix serializeValue to only use the runtime value

* fix deserializeValue to get the right Type for dart:core types

* cleanup prop_attr_reflection_test to use mutation observer

R=blois@google.com, sigmund@google.com

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

[Siggi Cherem (dart-lang), 2013-10-21 21:07:42]

this is looking really cool - mostly minor comments, and a couple questions, but
overall looks great

https://codereview.chromium.org/29823005/diff/50001/pkg/observe/test/transfor...
File pkg/observe/test/transform_test.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/observe/test/transfor...
pkg/observe/test/transform_test.dart:56: 'extends Base with Mixin,
ChangeNotifier implements Interface, Interface2');
80

https://codereview.chromium.org/29823005/diff/50001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/29823005/diff/50001/pkg/pkg.status#oldcode101
pkg/pkg.status:101: polymer/test/instance_attrs_test: Pass, Timeout # Issue
13260
do you think your changes fix these too?

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.dart
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:248: * True to include the JS script tag directly,
with the
with -> without?

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:272: *   * `--no-js`: deploy replaces *.dart
scripts with *.dart.js. This
remove 'no-'

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:305: defaultsTo: true)
eventually I agree that should be the default. I wonder if it should be the
default today though.

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
pkg/polymer/lib/src/build/common.dart:57: bool directlyIncludeJS;
should we make any (or all) of these fields final?

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
File pkg/polymer/lib/src/build/polyfill_injector.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
pkg/polymer/lib/src/build/polyfill_injector.dart:25: * with a script tag that
loads the dart2js compiled code3 directly.
code3 -> code

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:54: void initPolymerOptimized(List<String>
libraries) {
mmm.. this might have to change slightly once I let people call initPolymer in
their code (init.dart is optional). My thought is to make the build step do some
setup first, but keep the original call to 'initPolymer', so it behaves like the
optimized version.

no need to do anything here, just a heads up that I'll be changing this shortly
=)

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/test/listorder_test.dart (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/listorder_test.dart:35:
endOfMicrotask(expectAsync0(() {
... it would be nice to have a future for endOfMicrotask.

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/listorder_test.dart:39: // component is at a
different path from the main HTML document. For now we
80

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/test/listorder_test.html (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/listorder_test.html:18: <script
type="application/dart" src="listorder_test.dart" defer></script>
is this needed?

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/test/markdone_test.dart (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/markdone_test.dart:65: expect(label is
LabelElement, isTrue, reason: 'text is in a label: $label');
80

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/web/todo_row.dart (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/web/todo_row.dart:32: void removeTodo() {
appModel.todos.remove(todo); }
I can swear we had fixed this before

[blois, 2013-10-21 21:08:11]

https://codereview.chromium.org/29823005/diff/50001/pkg/custom_element/lib/po...
File pkg/custom_element/lib/polyfill.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/custom_element/lib/po...
pkg/custom_element/lib/polyfill.dart:38: return new
js.JsObject.fromBrowserObject(document).hasProperty('register');
document.supportsRegister?

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.dart
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:272: *   * `--no-js`: deploy replaces *.dart
scripts with *.dart.js. This
comment is out of sync with code (--no-js vs --js).

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:37: // this is run after all HTML imports are
resolved.
Should there be a check to validate that HTML imports are loaded by checking the
polyfill (or if not polyfilled, document.readyState)?

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:41: // TODO(14262): Schedule a dummy microtask
in the root Zone to prevent
Still necessary?

[Siggi Cherem (dart-lang), 2013-10-21 21:10:24]

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:37: // this is run after all HTML imports are
resolved.
On 2013/10/21 21:08:12, blois wrote:
> Should there be a check to validate that HTML imports are loaded by checking
the
> polyfill (or if not polyfilled, document.readyState)?

Note that we are not using the polyfill for this - this part of the code is for
Dartium only. Maybe we should make a note of that too

[Siggi Cherem (dart-lang), 2013-10-21 21:32:24]

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.dart
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:272: *   * `--no-js`: deploy replaces *.dart
scripts with *.dart.js. This
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> remove 'no-'

OK - after re-reading I understand this better. Feel free to leave it as it is,
maybe add the words "by default" somewhere to make it more obvious

[Jennifer Messerly, 2013-10-21 21:42:56]

https://codereview.chromium.org/29823005/diff/50001/pkg/custom_element/lib/po...
File pkg/custom_element/lib/polyfill.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/custom_element/lib/po...
pkg/custom_element/lib/polyfill.dart:38: return new
js.JsObject.fromBrowserObject(document).hasProperty('register');
On 2013/10/21 21:08:12, blois wrote:
> document.supportsRegister?

Done.

https://codereview.chromium.org/29823005/diff/50001/pkg/observe/test/transfor...
File pkg/observe/test/transform_test.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/observe/test/transfor...
pkg/observe/test/transform_test.dart:56: 'extends Base with Mixin,
ChangeNotifier implements Interface, Interface2');
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> 80

Done.

https://codereview.chromium.org/29823005/diff/50001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/29823005/diff/50001/pkg/pkg.status#oldcode101
pkg/pkg.status:101: polymer/test/instance_attrs_test: Pass, Timeout # Issue
13260
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> do you think your changes fix these too?

Not sure. It could be windows bots are just slow. I'm willing to try it.

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.dart
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:248: * True to include the JS script tag directly,
with the
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> with -> without?

Done.

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:272: *   * `--no-js`: deploy replaces *.dart
scripts with *.dart.js. This
On 2013/10/21 21:08:12, blois wrote:
> comment is out of sync with code (--no-js vs --js).

the "--no-" prefix is from args.dart. it's how you negate a boolean option.
attempted to clarify comment.

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/builder.d...
pkg/polymer/lib/builder.dart:305: defaultsTo: true)
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> eventually I agree that should be the default. I wonder if it should be the
> default today though.

it should. browser/dart.js needs to go away for 1.0 :)

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
pkg/polymer/lib/src/build/common.dart:57: bool directlyIncludeJS;
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> should we make any (or all) of these fields final?

can't. it is set from elsewhere.

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
File pkg/polymer/lib/src/build/polyfill_injector.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/build...
pkg/polymer/lib/src/build/polyfill_injector.dart:25: * with a script tag that
loads the dart2js compiled code3 directly.
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> code3 -> code

oops. fixed

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:37: // this is run after all HTML imports are
resolved.
On 2013/10/21 21:10:24, Siggi Cherem (dart-lang) wrote:
> On 2013/10/21 21:08:12, blois wrote:
> > Should there be a check to validate that HTML imports are loaded by checking
> the
> > polyfill (or if not polyfilled, document.readyState)?
> 
> Note that we are not using the polyfill for this - this part of the code is
for
> Dartium only. Maybe we should make a note of that too

Siggi do you mind following up and adding a comment?

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:41: // TODO(14262): Schedule a dummy microtask
in the root Zone to prevent
On 2013/10/21 21:08:12, blois wrote:
> Still necessary?

nope.

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:54: void initPolymerOptimized(List<String>
libraries) {
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> mmm.. this might have to change slightly once I let people call initPolymer in
> their code (init.dart is optional). My thought is to make the build step do
some
> setup first, but keep the original call to 'initPolymer', so it behaves like
the
> optimized version.
> 
> no need to do anything here, just a heads up that I'll be changing this
shortly
> =)

yeah, I expected it would change :)
just illustrating how we can skip the zone now.

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/test/listorder_test.dart (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/listorder_test.dart:35:
endOfMicrotask(expectAsync0(() {
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> ... it would be nice to have a future for endOfMicrotask.

agree. this is covered by another bug.

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/listorder_test.dart:39: // component is at a
different path from the main HTML document. For now we
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> 80

Done.

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/test/listorder_test.html (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/listorder_test.html:18: <script
type="application/dart" src="listorder_test.dart" defer></script>
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> is this needed?

i'll give it another try now that we have the other fixes.

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/test/markdone_test.dart (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/test/markdone_test.dart:65: expect(label is
LabelElement, isTrue, reason: 'text is in a label: $label');
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> 80

Done.

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
File samples/third_party/todomvc/web/todo_row.dart (right):

https://codereview.chromium.org/29823005/diff/50001/samples/third_party/todom...
samples/third_party/todomvc/web/todo_row.dart:32: void removeTodo() {
appModel.todos.remove(todo); }
On 2013/10/21 21:07:42, Siggi Cherem (dart-lang) wrote:
> I can swear we had fixed this before

yeah, not sure :)

[blois, 2013-10-21 21:47:29]

lgtm

[Siggi Cherem (dart-lang), 2013-10-21 21:48:03]

lgtm

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/29823005/diff/50001/pkg/polymer/lib/src/loade...
pkg/polymer/lib/src/loader.dart:37: // this is run after all HTML imports are
resolved.
On 2013/10/21 21:42:56, John Messerly wrote:
> On 2013/10/21 21:10:24, Siggi Cherem (dart-lang) wrote:
> > On 2013/10/21 21:08:12, blois wrote:
> > > Should there be a check to validate that HTML imports are loaded by
checking
> > the
> > > polyfill (or if not polyfilled, document.readyState)?
> > 
> > Note that we are not using the polyfill for this - this part of the code is
> for
> > Dartium only. Maybe we should make a note of that too
> 
> Siggi do you mind following up and adding a comment?

Not a problem

[Jennifer Messerly, 2013-10-21 21:49:35]

thanks! FYI "defer" was not needed, so I removed. I can't upload due to
appengine 500 errors tho.

[Jennifer Messerly, 2013-10-21 21:50:11]

Committed patchset #8 manually as r28956 (presubmit successful).