Adding an inheriting data model to Node

Adding a model property to Node which is inherited through the tree.

This only works for trees which are in the document, but when a tree is added to the document everything should just 'start working'.

This should work on all browsers (tested on Chrome, FF and IE9).

Not sure about performance impact on large trees and lots of bindings (balance between optimizing for lots of bindings or lots of elements, few bindings), but it should not impact anything if models are not used.

BUG=

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

[Jennifer Messerly, 2013-03-15 03:20:46]

This is looking great. Sooooo cool to see the start of real MDV for Dart :)

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
File tools/dom/src/ModelTreeObserver.dart (right):

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:72: if (node._hasLocalModel != true &&
node._model != model &&
I wonder if it's worth noting that we're potentially calling into user code
(operator ==) here.

I think it's the right choice but might be worth a comment.

In observers, an exception from one observer could mess up the propagation of
other observers, so I take pains to wrap it in a try-catch and deliver the
exception to a handler:

https://github.com/dart-lang/web-ui/blob/master/lib/observe/observable.dart#L629

I don't know if that's necessarily a good pattern either, though. It feels
heavyweight to put in a guard, but since dart:html is a platform library it
might be worth it.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:367: _model = value;
bail if old model == value?

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:372:
_modelChangedStream.add(value);
Does this deliver synchronously?

If so, it seems like we might want to wait until after we've propagated? Mainly
to ensure that user code doesn't see inconsistent tree/model state. (Also they
could muck with the tree, not sure if that could have adverse effects)

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:381: */
mark this @Experimental too, along with onModelChanged?

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:382: void clearModel() {
out of curiosity, why not use `null` for this? Does it make sense to distinguish
between "not set" and "null"?

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:398: Stream get
onModelChanged {
One thing I was wondering about for this event. With templates, we'd potentially
be listening on a lot of fine-grained modelChanged events. Do we want to do
that? Would a global event handler be better? That would seem to imply some sort
of bubbling, which is bad. Maybe the callback API should look more like mutation
observers -- a batch of .model changes are delivered on the next microtask to
the template system. On the other hand, the current API is nice and simple.

WDYT?

[blois, 2013-03-15 18:26:05]

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
File tools/dom/src/ModelTreeObserver.dart (right):

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:72: if (node._hasLocalModel != true &&
node._model != model &&
On 2013/03/15 03:20:46, John Messerly wrote:
> I wonder if it's worth noting that we're potentially calling into user code
> (operator ==) here.
> 
> I think it's the right choice but might be worth a comment.
> 
> In observers, an exception from one observer could mess up the propagation of
> other observers, so I take pains to wrap it in a try-catch and deliver the
> exception to a handler:
> 
>
https://github.com/dart-lang/web-ui/blob/master/lib/observe/observable.dart#L629
> 
> I don't know if that's necessarily a good pattern either, though. It feels
> heavyweight to put in a guard, but since dart:html is a platform library it
> might be worth it.

The error handling here is the question-  I believe there's supposed to be a
global unhandled exception handler- ideally this would pass the error onto that
and continue. Following up with Florian & Mads to see where that work is, adding
a TODO in the meantime.

We have a similar issue right now with setImmediate (and probably elsewhere).

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:367: _model = value;
On 2013/03/15 03:20:46, John Messerly wrote:
> bail if old model == value?

Added check to eliminate invalid change notification & unnecessary tree
traversal.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:372:
_modelChangedStream.add(value);
On 2013/03/15 03:20:46, John Messerly wrote:
> Does this deliver synchronously?
> 
> If so, it seems like we might want to wait until after we've propagated?
Mainly
> to ensure that user code doesn't see inconsistent tree/model state. (Also they
> could muck with the tree, not sure if that could have adverse effects)

Yeah, was going to mention that- still need to work on the timing a bit. Right
now there's a difference between the MutationObservers and MutationEvents code
which could cause problems, then there's when these events get fired.

I was going to do it as a separate CL, but can do it in this one- I think most
of the work is in the testing.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:381: */
On 2013/03/15 03:20:46, John Messerly wrote:
> mark this @Experimental too, along with onModelChanged?

Done.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:382: void clearModel() {
On 2013/03/15 03:20:46, John Messerly wrote:
> out of curiosity, why not use `null` for this? Does it make sense to
distinguish
> between "not set" and "null"?

If a detail view is set display the selected item and there is no selection then
you don't want it inheriting the model from further up the tree- this could
cause bindings to pick up values which are incorrect. So null can be set and
will inherit down.

FWIW, MDV addresses this by differentiating between null and undefined, where
null is set and undefined is inherit.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Node.darttemplate:398: Stream get
onModelChanged {
On 2013/03/15 03:20:46, John Messerly wrote:
> One thing I was wondering about for this event. With templates, we'd
potentially
> be listening on a lot of fine-grained modelChanged events. Do we want to do
> that? Would a global event handler be better? That would seem to imply some
sort
> of bubbling, which is bad. Maybe the callback API should look more like
mutation
> observers -- a batch of .model changes are delivered on the next microtask to
> the template system. On the other hand, the current API is nice and simple.
> 
> WDYT?
> 

One thing that I was thinking about is that we should be able to transparently
redirect these change notifications to the node which the model is set on- so
when the model is changed we just loop through the set of change watchers. 

This would make it efficient to repeatedly modify the model on a specific
element, which I believe is the primary scenario and would remove the tree
traversal for model changes.

My concern is getting the timings right with the tree change events- it may
require better tracking of whether items are in the tree or not. Again, more
tests needed.

I also want to make sure that this works well with custom elements, where users
are going to care directly about the model changing and will be poking new
values in at random times.

I'd like to see how far we can get with this fine-grained level of notifications
and see where the performance issues are.

[Jennifer Messerly, 2013-03-15 18:49:21]

LGTM! Agree we can keep interesting in follow ups. Would be awesome to get this
in so I can start using it in the template side :)

https://codereview.chromium.org/12754013/diff/13007/tools/dom/src/ModelTreeOb...
File tools/dom/src/ModelTreeObserver.dart (right):

https://codereview.chromium.org/12754013/diff/13007/tools/dom/src/ModelTreeOb...
tools/dom/src/ModelTreeObserver.dart:72: // TODO(blois): calling into user code
with the != call, should catch errors
FWIW, I noticed the pattern in stream_impl is to wrap callbacks in:

      } on AsyncError catch (e) {
        e.throwDelayed();
      } catch (e, s) {
        new AsyncError(e, s).throwDelayed();
      }

I'll probably switch to that pattern in observers

https://codereview.chromium.org/12754013/diff/13007/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12754013/diff/13007/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Node.darttemplate:379:
_modelChangedStream.add(value);
A couple more questions:

* Should this be the node instead? The benefit to that is you can get the model
from the node but not vice versa. Then I could have one closure that handles
modelChanged for a bunch of nodes. Kind of analogous to .NET's (object sender,
EventArgs args) pattern.

Then again, I could also close over the node at the cost of an extra allocation.
Maybe it's premature to be optimizing :)

* Maybe this should be lighter weight than a stream, since it's kind of an
implementation detail? In observers I used callbacks, because I was trying to
keep it low-level. OTOH, maybe that was a mistake and I should use streams there
too.

[Jacob, 2013-03-15 18:50:19]

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
File tools/dom/src/ModelTreeObserver.dart (right):

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:17: 
What about when there are true shadow DOMs on the page?
Don't you need to add observers to the root of each shadow DOM as well?
Probably just add a TODO that you don't support ShadowDOM and file a bug.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:43: var node = e.target;
mutation observers are async while DOMNodeInserted is sync.
You should force the DOMNodeInserted and DOMNodeRemoved to batch and execute on
the next microtask so that behavior is consistent across old and new browsers.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:77: for (var child = node.$dom_firstChild;
child != null;
high level: I think it would be a lot more efficient to lazily compute the
models than to aggressively propagate models.  For example, when compiled to JS,
this code currently adds expandos to every Element and Text node on a page even
though most of them don't care at all.  I'd suggest instead making the getter
for model a bit more expensive.  For example, if the model hasn't been computed,
it should walk up the tree to find the first valid model.

[blois, 2013-03-15 20:14:10]

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
File tools/dom/src/ModelTreeObserver.dart (right):

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:17: 
On 2013/03/15 18:50:19, Jacob wrote:
> What about when there are true shadow DOMs on the page?
> Don't you need to add observers to the root of each shadow DOM as well?
> Probably just add a TODO that you don't support ShadowDOM and file a bug.

Done.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:43: var node = e.target;
On 2013/03/15 18:50:19, Jacob wrote:
> mutation observers are async while DOMNodeInserted is sync.
> You should force the DOMNodeInserted and DOMNodeRemoved to batch and execute
on
> the next microtask so that behavior is consistent across old and new browsers.

Done.

https://codereview.chromium.org/12754013/diff/4001/tools/dom/src/ModelTreeObs...
tools/dom/src/ModelTreeObserver.dart:77: for (var child = node.$dom_firstChild;
child != null;
On 2013/03/15 18:50:19, Jacob wrote:
> high level: I think it would be a lot more efficient to lazily compute the
> models than to aggressively propagate models.  For example, when compiled to
JS,
> this code currently adds expandos to every Element and Text node on a page
even
> though most of them don't care at all.  I'd suggest instead making the getter
> for model a bit more expensive.  For example, if the model hasn't been
computed,
> it should walk up the tree to find the first valid model.

Models are only cached locally when a change listener is on the node, otherwise
the model is found by traversing the tree.

https://codereview.chromium.org/12754013/diff/13007/tools/dom/src/ModelTreeOb...
File tools/dom/src/ModelTreeObserver.dart (right):

https://codereview.chromium.org/12754013/diff/13007/tools/dom/src/ModelTreeOb...
tools/dom/src/ModelTreeObserver.dart:72: // TODO(blois): calling into user code
with the != call, should catch errors
On 2013/03/15 18:49:21, John Messerly wrote:
> FWIW, I noticed the pattern in stream_impl is to wrap callbacks in:
> 
>       } on AsyncError catch (e) {
>         e.throwDelayed();
>       } catch (e, s) {
>         new AsyncError(e, s).throwDelayed();
>       }
> 
> I'll probably switch to that pattern in observers

Looks good.

https://codereview.chromium.org/12754013/diff/13007/tools/dom/templates/html/...
File tools/dom/templates/html/impl/impl_Node.darttemplate (right):

https://codereview.chromium.org/12754013/diff/13007/tools/dom/templates/html/...
tools/dom/templates/html/impl/impl_Node.darttemplate:379:
_modelChangedStream.add(value);
On 2013/03/15 18:49:21, John Messerly wrote:
> A couple more questions:
> 
> * Should this be the node instead? The benefit to that is you can get the
model
> from the node but not vice versa. Then I could have one closure that handles
> modelChanged for a bunch of nodes. Kind of analogous to .NET's (object sender,
> EventArgs args) pattern.
> 
> Then again, I could also close over the node at the cost of an extra
allocation.
> Maybe it's premature to be optimizing :)
> 
> * Maybe this should be lighter weight than a stream, since it's kind of an
> implementation detail? In observers I used callbacks, because I was trying to
> keep it low-level. OTOH, maybe that was a mistake and I should use streams
there
> too.

Changed callback to be Node rather than model, should be efficient as the model
is always cached locally when generating change events.

Discussed streams, will use for now, but not committed to them long-term.

[blois, 2013-03-15 20:23:37]

Committed patchset #3 manually as r20104 (presubmit successful).

[ngeoffray, 2013-03-15 21:09:49]

https://codereview.chromium.org/12754013/diff/26001/sdk/lib/html/dart2js/html...
File sdk/lib/html/dart2js/html_dart2js.dart (right):

https://codereview.chromium.org/12754013/diff/26001/sdk/lib/html/dart2js/html...
sdk/lib/html/dart2js/html_dart2js.dart:17747: +  var _model;
FYI: dart2js is not happy with a field typed as var, as it now thinks anything
can be instantiated.