[mdv] implement full DOM stability

implement full DOM stability
Port of https://github.com/Polymer/mdv/commit/3e0b8ebe1d431b80cd92b40c4ce7e29ef086d555

Also includes a follow up that fixed a bug in the original CL:
Dont unbind instance nodes that have been re-used from the instanceCache
https://github.com/Polymer/mdv/commit/033257abab40e404924c844db8a1a7551571e4ba

(Note: this is based on https://codereview.chromium.org/17706003/)

R=justinfagnani@google.com

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

[Jennifer Messerly, 2013-06-26 03:28:09]

fyi, hold off on this one -- I didn't upload the test changes.

[Jennifer Messerly, 2013-06-26 17:08:06]

PTAL! Now includes test

[Jennifer Messerly, 2013-06-26 18:30:27]

it sounds like Pete is interested in reviewing this one

[justinfagnani, 2013-06-26 19:05:15]

On 2013/06/26 18:30:27, John Messerly wrote:
> it sounds like Pete is interested in reviewing this one

I'm taking a look now. I'd like to get familiar with as much of this code as
possible.

[justinfagnani, 2013-06-26 20:01:13]

lgtm

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/lib/src/bindings.dart
File pkg/mdv/lib/src/bindings.dart (right):

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/lib/src/bindings.d...
pkg/mdv/lib/src/bindings.dart:328: if (value is! List) {
List -> Iterable?

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/lib/src/bindings.d...
pkg/mdv/lib/src/bindings.dart:481: if (node is Element && (node as
Element).isTemplate) {
this cast is unfortunate

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
File pkg/mdv/test/template_element_test.dart (right):

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:77: void addExpandos(node) {
Consider a more descriptive name than addExpandos(), preferably including 'text'
since this is for the text property.

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:296: recursivelySetTemplateModel(div,
model);
what is the reason for setting the model on all <template>s in the fragment?

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:300: var template = div.$dom_firstChild;
I've always wondered about the $-prefixed methods. Why not div.nodes.first?

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:324: expect(div.nodes[5].text, "2");
do you want to check that the nodes are not reused?

[Jennifer Messerly, 2013-06-26 20:47:50]

thanks!

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/lib/src/bindings.dart
File pkg/mdv/lib/src/bindings.dart (right):

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/lib/src/bindings.d...
pkg/mdv/lib/src/bindings.dart:328: if (value is! List) {
On 2013/06/26 20:01:13, justinfagnani wrote:
> List -> Iterable?

good question. We do use indexing, but maybe if it's an Iterable we should call
.toList on it. 
I filed https://code.google.com/p/dart/issues/detail?id=11535

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
File pkg/mdv/test/template_element_test.dart (right):

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:77: void addExpandos(node) {
On 2013/06/26 20:01:13, justinfagnani wrote:
> Consider a more descriptive name than addExpandos(), preferably including
'text'
> since this is for the text property.

I usually try to keep names as close as possible to the original to make future
porting easier (other than Dart-specific changes, like changing underscores).

However they did have these as local functions. Maybe I should copy it back into
the two test cases?
Other possible courses of action: comment on original CL and/or file a pull
request to change MDV.

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:296: recursivelySetTemplateModel(div,
model);
On 2013/06/26 20:01:13, justinfagnani wrote:
> what is the reason for setting the model on all <template>s in the fragment?

it's just shorthand used for the tests

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:300: var template = div.$dom_firstChild;
On 2013/06/26 20:01:13, justinfagnani wrote:
> I've always wondered about the $-prefixed methods. Why not div.nodes.first?

firstChild was just unhidden. Yay!

https://codereview.chromium.org/17770006/diff/4001/pkg/mdv/test/template_elem...
pkg/mdv/test/template_element_test.dart:324: expect(div.nodes[5].text, "2");
On 2013/06/26 20:01:13, justinfagnani wrote:
> do you want to check that the nodes are not reused?

that seems like a valid question to ask on the original CL. Do you think it's
worth sending a pull request to MDV for this?

[Jennifer Messerly, 2013-06-27 21:35:40]

Committed patchset #4 manually as r24553 (presubmit successful).