Implement Model-Driven-Views spec for Dart

Implement Model-Driven-Views spec for Dart

This has code ported from a few different sources:
* a port of https://github.com/toolkitchen/mdv polyfill
* a port of the observable layer:  https://github.com/dart-lang/web-ui/tree/master/lib/observe
* a port of list change summary: https://github.com/rafaelw/ChangeSummary/blob/master/change_summary.js

The observePath/PathObserver code is written from scratch, but inspired by change_summary. I tried to keep it simpler; it does not support dirty checking. There's no good way to do that until we get dart:mirrors.

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

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

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

[Jennifer Messerly, 2013-05-01 00:42:43]

At long last, here it is. *Real MDV for Dart*!

Since it's a lot of code, I thought it might make sense to parallelize the
review

Pete: can you have a look at the html code?
Siggi: can you have a look at the observer code?

Keep in mind, other than "observe_path", it isn't original code. It probably
helps to have the original source handy when doing the review. I tried to put a
comment at the top indicating where it came from.

Since this is a big CL it probably helps to indicate what fixes should be done
before checkin vs what is acceptable to do in a follow up.

By the way, this is passing Chrome and Firefox, I'm in the process of testing
Dartium, then IE.


Cheers and thanks in advance! :D
- John

[Jennifer Messerly, 2013-05-01 00:44:58]

https://codereview.chromium.org/14732003/diff/2001/tools/dom/src/TemplateBind...
File tools/dom/src/TemplateBindings.dart (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/src/TemplateBind...
tools/dom/src/TemplateBindings.dart:9: // The code mostly comes from
src/template_element.js
What's up with JavaStyle file names in tools/dom/src?

Would it be okay if I rename this template_element.dart to match the JS file? Or
should we convert dom/src en-masse?

[Jennifer Messerly, 2013-05-01 02:33:52]

FYI, Dartium build change:
https://codereview.chromium.org/14744002

[blois, 2013-05-01 17:00:42]

Some initial comments

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
File sdk/lib/observe/list_diff.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:37: // ListChangeDeltas straight from
ObservableList? Then it's just a matter of
I think we should consider this. I'd also be interested in what a 'move'
operation would look like- it's fairly common in UI's and this API makes it
difficult to treat it as a move rather than separate insert/remove actions.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:66: List _removed;
final?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:21: toObservable(value, {bool deep: true}) {
Are there explicit scenarios for a deep observable? Is this primarily an
observable JSON block? Seems like an edge case.

In general this seems a bit deceptive as it returns new collections which must
be used (almost expected this API to make an observable of the original using
watchers).

I think I'd rather see explicit creation of ObservableList, etc.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:104: if (_observers != null && changes != null)
{
hasObservers instead?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:136: Object notifyChange(key, Object oldValue,
Object newValue,
Seems like this is a very common method to call, and I believe that named
parameters add a bit of overhead. Separate them out to different methods?

This probably applies for downstream APIs as well.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:108: var newValue = value;
I'm curious how often this will filter changes. I've seen multiple caches of
property values add too much overhead, seems like it's being cached at a couple
levels. It's a tricky cost/benefit calculation though, and probably not
something we can estimate until analyzing real apps.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:259: s = s.replaceAll(new RegExp('\\s'), '');
cache the regexp?

https://codereview.chromium.org/14732003/diff/2001/tests/lib/observe/list_cha...
File tests/lib/observe/list_change_test.dart (right):

https://codereview.chromium.org/14732003/diff/2001/tests/lib/observe/list_cha...
tests/lib/observe/list_change_test.dart:262: test('trunacte and add, sharing a
contiguous block', () {
truncate

https://codereview.chromium.org/14732003/diff/2001/tools/dom/dom.py
File tools/dom/dom.py (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/dom.py#newcode103
tools/dom/dom.py:103: os.chdir(os.path.join('..', '..', '..'))
Is this necessary? The script should reset the path after every command (see
init_dir())

https://codereview.chromium.org/14732003/diff/2001/tools/dom/src/TemplateBind...
File tools/dom/src/TemplateBindings.dart (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/src/TemplateBind...
tools/dom/src/TemplateBindings.dart:9: // The code mostly comes from
src/template_element.js
On 2013/05/01 00:44:58, John Messerly wrote:
> What's up with JavaStyle file names in tools/dom/src?
> 
> Would it be okay if I rename this template_element.dart to match the JS file?
Or
> should we convert dom/src en-masse?

We should rename en-masse. It keeps on bugging me, but just enough to not make
me change it.

I think it's this way because other DOM Dart files need to use IDL naming, but
these should be cleaned up. Also want to remove the impl_ prefixes!

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Document.darttemplate (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Document.darttemplate:40: Document
_templateContentsOwner;
Should this be on HtmlDocument?

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:625: Map<String,
_Binding> _attributeBindings;
I believe that you want the @Creates('Null') on every field you add to a native
class, otherwise the JS compiler thinks that the types may be able to be
instantiated.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:632: // only mix it in
to the elements that need it.
Once we have element subclassing, can we have a TemplateElement interface and an
Element subclass which implements it?

Could they be on TemplateElement and UnknownElement in the meantime?

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_HTMLInputElement.darttemplate (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLInputElement.darttemplate:43:
_CheckedBinding _checkedBinding;
valueAsNumber and valueAsDate in the future? Or should these go through value w/
type conversion?

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate
(right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:23: static
bool get supported => new Element.tag('template') is! UnknownElement;
Can use Element.isTagSupported instead.

Also, can put this in systemhtml.py (search for HTMLShadowElement), and it'll
default to true for Dartium.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:37: static
Stream<DocumentFragment> get instanceCreated {
Does this need to be public?

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:117:
document.head.nodes.add(style);
document.head.append(style) (it's faster).

[Jennifer Messerly, 2013-05-01 17:56:34]

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:21: toObservable(value, {bool deep: true}) {
On 2013/05/01 17:00:42, blois wrote:
> Are there explicit scenarios for a deep observable? Is this primarily an
> observable JSON block? Seems like an edge case.

Any time you have a array/map literal. It's the most common use case for this
API right now. But yeah, JSON data is the other obvious one.

> In general this seems a bit deceptive as it returns new collections which must
> be used (almost expected this API to make an observable of the original using
> watchers).
> 
> I think I'd rather see explicit creation of ObservableList, etc.

This is from
https://github.com/dart-lang/web-ui/blob/master/lib/observe/observable.dart#L245

It was an explicit request from the original code review. We went around on the
name but this was the best we could come up with. The hope is that by calling a
top-level function "toFoo", it makes it obvious you should use the return value
:)

Once we have dirty checking I think we can remove this, then people can just use
normal List/Maps...

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:136: Object notifyChange(key, Object oldValue,
Object newValue,
On 2013/05/01 17:00:42, blois wrote:
> Seems like this is a very common method to call, and I believe that named
> parameters add a bit of overhead. Separate them out to different methods?
> 
> This probably applies for downstream APIs as well.

Oh, I didn't know they had overhead. Is that true on VM and Dart2js even in the
case of a monomorphic/type inferrable call site? I hope this guy is always
monomorphic/inferrable because it's dispatched on "this" and there's no reason
to override it.

I'm happy to split it, but would appreciate suggestions on method name :)

notifyChange -> only fields
notifyChangeItem -> list/map?

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_HTMLInputElement.darttemplate (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLInputElement.darttemplate:43:
_CheckedBinding _checkedBinding;
On 2013/05/01 17:00:42, blois wrote:
> valueAsNumber and valueAsDate in the future? Or should these go through value
w/
> type conversion?

yeah, type conversion or filters or something for those two
I did file https://github.com/toolkitchen/mdv/issues/50
there are a few like selectedIndex that would be super useful.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate
(right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:37: static
Stream<DocumentFragment> get instanceCreated {
On 2013/05/01 17:00:42, blois wrote:
> Does this need to be public?

yup! for same reason as MDV has it.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:117:
document.head.nodes.add(style);
On 2013/05/01 17:00:42, blois wrote:
> document.head.append(style) (it's faster).

omg, I didn't know about .append! I'll fix that everywhere :)

[Jennifer Messerly, 2013-05-01 18:03:44]

https://codereview.chromium.org/14732003/diff/2001/tools/dom/dom.py
File tools/dom/dom.py (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/dom.py#newcode103
tools/dom/dom.py:103: os.chdir(os.path.join('..', '..', '..'))
On 2013/05/01 17:00:42, blois wrote:
> Is this necessary? The script should reset the path after every command (see
> init_dir())

Unless init_dir was a recent addition, yeah, this was a fix ;)

I was trying to run: "./tools/dom/dom.py gen test_drt <testName>" and it blew up
after "gen" because path was wrong... Hence this.

I can take a look and see why init_dir isn't working.

[Jennifer Messerly, 2013-05-01 18:11:46]

https://codereview.chromium.org/14732003/diff/2001/tools/dom/dom.py
File tools/dom/dom.py (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/dom.py#newcode103
tools/dom/dom.py:103: os.chdir(os.path.join('..', '..', '..'))
On 2013/05/01 18:03:44, John Messerly wrote:
> On 2013/05/01 17:00:42, blois wrote:
> > Is this necessary? The script should reset the path after every command (see
> > init_dir())
> 
> Unless init_dir was a recent addition, yeah, this was a fix ;)
> 
> I was trying to run: "./tools/dom/dom.py gen test_drt <testName>" and it blew
up
> after "gen" because path was wrong... Hence this.
> 
> I can take a look and see why init_dir isn't working.

I figured it out.
Python __file__ is a relative path, so the result changes on the second call to
init_dir and it thinks my `os.path.dirname(os.path.realpath(__file__))` is:
"/mnt/code/dart/dart/tools/dom/scripts/tools/dom"

I think the abspath to dart_dir has to be saved before any chdir's happen. I'll
send you a fix for it.

[Siggi Cherem (dart-lang), 2013-05-01 18:57:56]

John this is looking awesome... just some minor comments on the observable side
of things.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
File sdk/lib/observe/list_diff.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:123: distances[i][j] = north < west ? north :
west;
consider using math.min?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:160: if (west < north) {
ditto:
var m = math.min(northWest, math.min(west, north))

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:244: while (oldStart < oldEnd)
style: add braces for the loop

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:247: return [ splice ];
style, remove spaces after [ and before ]

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:249: return [ new ListChangeDelta(currentStart,
likewise

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:8: * Converts the [Iterable] or [Map] to an
[ObservableList] or [ObservableMap],
any reason not to bring over ObservableSet?
Is the idea that we just treat it as an iterable?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:23: if (value is Observable) return value;
nit: consider swapping the above two lines. I know is equivalent because you
return immediately in _deepObserve if value is Observable, but swapping the
lines makes this function slightly more self documenting (for a moment I thought
that you'll clone an observable map to make it's contents observable too)

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:62: Stream<List<ChangeRecord>> get changes;
cool that you got these working!

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:64: // TODO(jmesserly): remove these ASAP.
should we preemptively mark them @deprecated ?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:136: Object notifyChange(key, Object oldValue,
Object newValue,
On 2013/05/01 17:00:42, blois wrote:
> Seems like this is a very common method to call, and I believe that named
> parameters add a bit of overhead. Separate them out to different methods?
> 
> This probably applies for downstream APIs as well.

given that there is only one optional argument, how about a possitional optional
argument instead of splitting the API?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:207: {this.kind: ChangeRecord.FIELD});
I assume the same argument of notifyChange applies here?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:277: new Future(deliverChangeRecords);
how about:
  runAsync(deliverChangeRecords)
?
since deliverChangeRecords doesn't return anything anyways

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable_list.dart:82: // TODO(jmesserly): do we need this now
that we have [summarizeListChanges]?
I was wondering the same thing. Alternatively, is it possible to clean up the
events afterwards with a single pass over the change records? This might be
similar to summarizeListChanges, but with less processing. Just collapsing
consecutive events that cancel each other. 

For example:
compactRecords([
  'update length: 5 -> 6'
  'update [3]: a -> n'
  'update [4]: b -> a'
  'update [5]: c -> b'
  'update [6]: _ -> c'])

would return: 'insert 'n' @ index 3"

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:193: PathObserver _path;
final

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:194: _PropertyObserver _next;
consider making this final too? this would probably mean changing the
PathObserver constructor to build things in reverse order:

for (int i = path.length - 1; i >= 0; i--) {
  _observer = new PropertyObserver(this, path[i], _observer);
  if (_lastObserver == null) _lastObserver = _observer;
}

[Jennifer Messerly, 2013-05-02 02:58:33]

PTAL, initial feedback addressed. Also passes on Dartium and IE9 now (+Firefox
+Chrome).

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
File sdk/lib/observe/list_diff.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:123: distances[i][j] = north < west ? north :
west;
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> consider using math.min?

Done.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:160: if (west < north) {
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> ditto:
> var m = math.min(northWest, math.min(west, north))

Done.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:244: while (oldStart < oldEnd)
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> style: add braces for the loop 

thanks. I had to fix that in sooooo many places, looks like a few slipped
through >.<

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:247: return [ splice ];
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> style, remove spaces after [ and before ]

done, but last I checked, there is no agreed upon style rule here.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:249: return [ new ListChangeDelta(currentStart,
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> likewise

Done.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:8: * Converts the [Iterable] or [Map] to an
[ObservableList] or [ObservableMap],
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> any reason not to bring over ObservableSet?
> Is the idea that we just treat it as an iterable?

It's just more work. Honestly I had it but ported but bringing the the tests
over was time consuming and this CL is desperately needed to unblock other
things. I'd like to revive it eventually.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:23: if (value is Observable) return value;
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> nit: consider swapping the above two lines. I know is equivalent because you
> return immediately in _deepObserve if value is Observable, but swapping the
> lines makes this function slightly more self documenting (for a moment I
thought
> that you'll clone an observable map to make it's contents observable too)

yeah, mixing the deep and shallow stuff in the same method is confusing. I
reorganized the code, let me know if it helps.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:62: Stream<List<ChangeRecord>> get changes;
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> cool that you got these working!

yeah. Kinda worried about overhead, but I guess we'll have to see.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:64: // TODO(jmesserly): remove these ASAP.
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> should we preemptively mark them @deprecated ?

yeah, that would be cool. The trouble is: @observable transform needs to
generate code that implements them. I dunno if all bugs around implementing
@deprecated methods are fixed.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:104: if (_observers != null && changes != null)
{
On 2013/05/01 17:00:42, blois wrote:
> hasObservers instead?

Done.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:136: Object notifyChange(key, Object oldValue,
Object newValue,
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> On 2013/05/01 17:00:42, blois wrote:
> > Seems like this is a very common method to call, and I believe that named
> > parameters add a bit of overhead. Separate them out to different methods?
> > 
> > This probably applies for downstream APIs as well.
> 
> given that there is only one optional argument, how about a possitional
optional
> argument instead of splitting the API? 

positional would be fine too. does that have overhead?

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:207: {this.kind: ChangeRecord.FIELD});
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> I assume the same argument of notifyChange applies here?

I guess. Personally I'd rather not micro optimize too much, unless we're sure
it's a problem. Using Streams is probably adding gobs of overhead already.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:277: new Future(deliverChangeRecords);
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> how about:
>   runAsync(deliverChangeRecords)
> ?
> since deliverChangeRecords doesn't return anything anyways

Thanks! I didn't know about runAsync.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable_list.dart:82: // TODO(jmesserly): do we need this now
that we have [summarizeListChanges]?
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> I was wondering the same thing. Alternatively, is it possible to clean up the
> events afterwards with a single pass over the change records? This might be
> similar to summarizeListChanges, but with less processing. Just collapsing
> consecutive events that cancel each other. 
> 
> For example:
> compactRecords([
>   'update length: 5 -> 6'
>   'update [3]: a -> n'
>   'update [4]: b -> a'
>   'update [5]: c -> b'
>   'update [6]: _ -> c'])
> 
> would return: 'insert 'n' @ index 3"

I might be misunderstanding but ... I think you're describing
summarizeListChanges :)

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:108: var newValue = value;
On 2013/05/01 17:00:42, blois wrote:
> I'm curious how often this will filter changes. I've seen multiple caches of
> property values add too much overhead, seems like it's being cached at a
couple
> levels. It's a tricky cost/benefit calculation though, and probably not
> something we can estimate until analyzing real apps.

yeah, right now:

1. Each step along the path is cached. (_PropertyObserver)
2. the "original" value of the path expr is cached, so we can avoid notification
if it didn't change.

I don't think #2 can be avoided--users don't want spurious notifications.

#1 is tricky, because we need to store enough stuff to unobserve the path and
react to changes. It could probably be optimized though. Right now I was going
for simplicity+correctness.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:193: PathObserver _path;
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> final

Done.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:194: _PropertyObserver _next;
On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> consider making this final too? this would probably mean changing the
> PathObserver constructor to build things in reverse order:
> 
> for (int i = path.length - 1; i >= 0; i--) {
>   _observer = new PropertyObserver(this, path[i], _observer);
>   if (_lastObserver == null) _lastObserver = _observer;
> }

Great suggestion, done!

FWIW, originally the chain wasn't static or precomputed, but I realized doing so
made things a lot simpler :), nice to see it's even simpler than I had thought.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observe_pa...
sdk/lib/observe/observe_path.dart:259: s = s.replaceAll(new RegExp('\\s'), '');
On 2013/05/01 17:00:42, blois wrote:
> cache the regexp?

Done.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/src/TemplateBind...
File tools/dom/src/TemplateBindings.dart (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/src/TemplateBind...
tools/dom/src/TemplateBindings.dart:9: // The code mostly comes from
src/template_element.js
On 2013/05/01 17:00:42, blois wrote:
> On 2013/05/01 00:44:58, John Messerly wrote:
> > What's up with JavaStyle file names in tools/dom/src?
> > 
> > Would it be okay if I rename this template_element.dart to match the JS
file?
> Or
> > should we convert dom/src en-masse?
> 
> We should rename en-masse. It keeps on bugging me, but just enough to not make
> me change it.
> 
> I think it's this way because other DOM Dart files need to use IDL naming, but
> these should be cleaned up. Also want to remove the impl_ prefixes!

sounds good to me

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Document.darttemplate (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Document.darttemplate:40: Document
_templateContentsOwner;
On 2013/05/01 17:00:42, blois wrote:
> Should this be on HtmlDocument?

Done.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:625: Map<String,
_Binding> _attributeBindings;
On 2013/05/01 17:00:42, blois wrote:
> I believe that you want the @Creates('Null') on every field you add to a
native
> class, otherwise the JS compiler thinks that the types may be able to be
> instantiated.

Done. Are statics okay? (_instanceCreated)

also: holy verbosity batman! it adds 3 lines per field because of the ifdef.
It's not going to scale very well if we do more of these polyfills.

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_Element.darttemplate:632: // only mix it in
to the elements that need it.
On 2013/05/01 17:00:42, blois wrote:
> Once we have element subclassing, can we have a TemplateElement interface and
an
> Element subclass which implements it?

yeah we could have a mixin:

class TemplateMixin {
  ... template stuff ...
}

class UnknownElement with TemplateMixin ... { ... }
class TemplateElement with TemplateMixin ... { ... }
...

> 
> Could they be on TemplateElement and UnknownElement in the meantime?

It's also all of the table elements and OptionElement :|

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
File tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate
(right):

https://codereview.chromium.org/14732003/diff/2001/tools/dom/templates/html/i...
tools/dom/templates/html/impl/impl_HTMLTemplateElement.darttemplate:23: static
bool get supported => new Element.tag('template') is! UnknownElement;
On 2013/05/01 17:00:42, blois wrote:
> Can use Element.isTagSupported instead.
> 
> Also, can put this in systemhtml.py (search for HTMLShadowElement), and it'll
> default to true for Dartium.

Done.

Anywhere I can put the doc comment?
I didn't want folks to assume !TemplateElement.supported means no MDV...

[floitsch, 2013-05-02 13:21:45]

We are trying to avoid new "dart:" libraries. In fact, we are trying to remove
them wherever we can.
Please coordinate with us and discuss why this new library cannot be a pub
package.

[Siggi Cherem (dart-lang), 2013-05-02 16:21:08]

lgtm for the observable implementation

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
File sdk/lib/observe/list_diff.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/list_diff....
sdk/lib/observe/list_diff.dart:247: return [ splice ];
On 2013/05/02 02:58:33, John Messerly wrote:
> On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> > style, remove spaces after [ and before ]
> 
> done, but last I checked, there is no agreed upon style rule here.

I was surprised to find it there, but apparently we do =)

http://www.dartlang.org/articles/style-guide/#dont-use-a-space-after---and--o...

sesms like you might have missed the space on the close: " ]"

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:23: if (value is Observable) return value;
On 2013/05/02 02:58:33, John Messerly wrote:
> On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> > nit: consider swapping the above two lines. I know is equivalent because you
> > return immediately in _deepObserve if value is Observable, but swapping the
> > lines makes this function slightly more self documenting (for a moment I
> thought
> > that you'll clone an observable map to make it's contents observable too)
> 
> yeah, mixing the deep and shallow stuff in the same method is confusing. I
> reorganized the code, let me know if it helps.

looks good. Thanks!

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable.dart:64: // TODO(jmesserly): remove these ASAP.
On 2013/05/02 02:58:33, John Messerly wrote:
> On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> > should we preemptively mark them @deprecated ?
> 
> yeah, that would be cool. The trouble is: @observable transform needs to
> generate code that implements them. I dunno if all bugs around implementing
> @deprecated methods are fixed.

I see, makes sense to wait.

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable_list.dart:82: // TODO(jmesserly): do we need this now
that we have [summarizeListChanges]?
On 2013/05/02 02:58:33, John Messerly wrote:
> On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> > I was wondering the same thing. Alternatively, is it possible to clean up
the
> > events afterwards with a single pass over the change records? This might be
> > similar to summarizeListChanges, but with less processing. Just collapsing
> > consecutive events that cancel each other. 
> > 
> > For example:
> > compactRecords([
> >   'update length: 5 -> 6'
> >   'update [3]: a -> n'
> >   'update [4]: b -> a'
> >   'update [5]: c -> b'
> >   'update [6]: _ -> c'])
> > 
> > would return: 'insert 'n' @ index 3"
> 
> I might be misunderstanding but ... I think you're describing
> summarizeListChanges :)

=) - I was thinking that summarizeListChanges needs the full diff, so I was
wondering if there is an alternative that could be more localized and that can
avoid hitting the worse case quadratic complexity. For instance, we could
collapse a few records together, but not all. One possible heuristic, for
example, can be to collapse only the sequence of records between updates to
'length' (just brainstorming here, of course).

[Jennifer Messerly, 2013-05-02 17:59:06]

On 2013/05/02 13:21:45, floitsch wrote:
> We are trying to avoid new "dart:" libraries. In fact, we are trying to remove
> them wherever we can.
> Please coordinate with us and discuss why this new library cannot be a pub
> package.

Believe me, I would love to avoid dart: libraries! I fully appreciate that
concern.

Here is the problem:

MDV must be implemented in dart:html because it is impossible to add DOM APIs
from a pkg.

Observability is required for MDV.

dart:html cannot depend on a package.

In the meantime I will move these APIs into dart:html, but I don't think that is
correct. We have tried to split DOM into more libraries to keep it more
manageable for users (svg, web_audio, etc).

I would love it if Dart was a more flexible language, either in my ability to
extend DOM from a package (which is easy and natural to do in JavaScript!), or
in my ability to have DOM depend on a package that was loaded in from the user's
code. Both of those solutions would be lovely, but aren't possible AFAIK.

[blois, 2013-05-02 18:12:56]

On 2013/05/02 17:59:06, John Messerly wrote:
> On 2013/05/02 13:21:45, floitsch wrote:
> > We are trying to avoid new "dart:" libraries. In fact, we are trying to
remove
> > them wherever we can.
> > Please coordinate with us and discuss why this new library cannot be a pub
> > package.
> 
> Believe me, I would love to avoid dart: libraries! I fully appreciate that
> concern.
> 
> Here is the problem:
> 
> MDV must be implemented in dart:html because it is impossible to add DOM APIs
> from a pkg.
> 
> Observability is required for MDV.
> 
> dart:html cannot depend on a package.
> 
> In the meantime I will move these APIs into dart:html, but I don't think that
is
> correct. We have tried to split DOM into more libraries to keep it more
> manageable for users (svg, web_audio, etc).
> 
> I would love it if Dart was a more flexible language, either in my ability to
> extend DOM from a package (which is easy and natural to do in JavaScript!), or
> in my ability to have DOM depend on a package that was loaded in from the
user's
> code. Both of those solutions would be lovely, but aren't possible AFAIK.

I'm concerned about having these rolled into HTML- the classes which need to
implement Observable are the ones which would be expected to also execute on the
server side. Sharing these classes between server and client is the primary goal
of a common language for server & client. I'm not sure how far away this client
& server story is, but unless we have a plan for supporting a subset of
dart:html on the server then this is a step backwards.

[Jennifer Messerly, 2013-05-02 19:48:04]

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/2001/sdk/lib/observe/observable...
sdk/lib/observe/observable_list.dart:82: // TODO(jmesserly): do we need this now
that we have [summarizeListChanges]?
On 2013/05/02 16:21:08, Siggi Cherem (dart-lang) wrote:
> On 2013/05/02 02:58:33, John Messerly wrote:
> > On 2013/05/01 18:57:56, Siggi Cherem (dart-lang) wrote:
> > > I was wondering the same thing. Alternatively, is it possible to clean up
> the
> > > events afterwards with a single pass over the change records? This might
be
> > > similar to summarizeListChanges, but with less processing. Just collapsing
> > > consecutive events that cancel each other. 
> > > 
> > > For example:
> > > compactRecords([
> > >   'update length: 5 -> 6'
> > >   'update [3]: a -> n'
> > >   'update [4]: b -> a'
> > >   'update [5]: c -> b'
> > >   'update [6]: _ -> c'])
> > > 
> > > would return: 'insert 'n' @ index 3"
> > 
> > I might be misunderstanding but ... I think you're describing
> > summarizeListChanges :)
> 
> =) - I was thinking that summarizeListChanges needs the full diff, so I was
> wondering if there is an alternative that could be more localized and that can
> avoid hitting the worse case quadratic complexity. For instance, we could
> collapse a few records together, but not all. One possible heuristic, for
> example, can be to collapse only the sequence of records between updates to
> 'length' (just brainstorming here, of course).

Ah, makes sense. Sounds kind of like the first phase of summarizeListChanges,
"_createInitialSplicesFromDiff", which does an initial linear pass through the
change records.

However, I think we want minimal diff to keep DOM stable and match MDV's DOM
manipulation

[Jennifer Messerly, 2013-05-02 19:57:28]

On 2013/05/02 18:12:56, blois wrote:
> On 2013/05/02 17:59:06, John Messerly wrote:
> > On 2013/05/02 13:21:45, floitsch wrote:
> > > We are trying to avoid new "dart:" libraries. In fact, we are trying to
> remove
> > > them wherever we can.
> > > Please coordinate with us and discuss why this new library cannot be a pub
> > > package.
> > 
> > Believe me, I would love to avoid dart: libraries! I fully appreciate that
> > concern.
> > 
> > Here is the problem:
> > 
> > MDV must be implemented in dart:html because it is impossible to add DOM
APIs
> > from a pkg.
> > 
> > Observability is required for MDV.
> > 
> > dart:html cannot depend on a package.
> > 
> > In the meantime I will move these APIs into dart:html, but I don't think
that
> is
> > correct. We have tried to split DOM into more libraries to keep it more
> > manageable for users (svg, web_audio, etc).
> > 
> > I would love it if Dart was a more flexible language, either in my ability
to
> > extend DOM from a package (which is easy and natural to do in JavaScript!),
or
> > in my ability to have DOM depend on a package that was loaded in from the
> user's
> > code. Both of those solutions would be lovely, but aren't possible AFAIK.
> 
> I'm concerned about having these rolled into HTML- the classes which need to
> implement Observable are the ones which would be expected to also execute on
the
> server side. Sharing these classes between server and client is the primary
goal
> of a common language for server & client. I'm not sure how far away this
client
> & server story is, but unless we have a plan for supporting a subset of
> dart:html on the server then this is a step backwards.

Gotcha. I think this change is on hold then until we can figure something out.

I'm open to any and all ideas. Here's some brainstorming:

* inject observe APIs into html somehow? in main?
* somehow keep observe a minimal interface -- but what do we do about dependency
of DOM on the shape of ObservableList change records?
* wait for a while, then implement entirely on top of dirty checking--but that
needs to wait for mirrors.
* abandon implementing MDV in DOM; instead use Expandos and jquery style
wrapper:

   mdv(template).model = ...
   mdv(template).content
   mdv(node).bind(..., ..., ...)

Not sure what will happen when these start showing up in Blink IDL (like
"content"), but we probably have got a little while. We'd also have to fake
something so custom elements can override "bind", since that's a key part of the
design.

[Jennifer Messerly, 2013-05-03 01:26:24]

I redid this change in package form:

https://github.com/dart-lang/web-ui/tree/mdv/observe
https://github.com/dart-lang/web-ui/tree/mdv/mdv

Curious to hear opinions. Note: to use the DOM APIs, you must do:

    mdv(<node exp>).<api name>


This seems like the cleanest "extension method" pattern in Dart right now.

It probably has a ton of overhead because of Expandos, but, meh. You pick your
tradeoffs and you live with em :)

[blois, 2013-05-03 17:49:40]

On 2013/05/03 01:26:24, John Messerly wrote:
> I redid this change in package form:
> 
> https://github.com/dart-lang/web-ui/tree/mdv/observe
> https://github.com/dart-lang/web-ui/tree/mdv/mdv
> 
> Curious to hear opinions. Note: to use the DOM APIs, you must do:
> 
>     mdv(<node exp>).<api name>
> 
> 
> This seems like the cleanest "extension method" pattern in Dart right now.
> 
> It probably has a ton of overhead because of Expandos, but, meh. You pick your
> tradeoffs and you live with em :)

Safest route, but sadness.

[Lasse Reichstein Nielsen, 2013-05-06 10:58:58]

drive-by comments.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:149: if (oldValue == newValue) return newValue;
Consider using "identical" instead. That won't make assumptions about the ==
operator on the values, and will catch all actual changes, not just changes to
unequal values.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:204: /** The previous value of the member. */
Will this be null for an insert?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:208: final newValue;
Will this be null for a remove?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:215: return other is ChangeRecord && kind ==
other.kind && key == other.key &&
Consider using identical for the tests. At least the value tests. The key will
likely be using == as equivalence in a Map.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:238: int _hash3(x, y, z) => _hash2(_hash2(x, y),
z);
Unused?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:240: int _hash4(w, x, y, z) => _hash2(_hash2(w,
x), _hash2(y, z));
This gives x and y the same multiplier (31), resulting in
  w * 31^2 + (x+y) * 31 + z
so it's symmetric in x and y which might give worse results. 
At least it's used so that x is a key and y is a value, which are likely to have
different types.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:259: var deliverCallbacks = _deliverCallbacks;
Consider using a Queue instead to avoid doing the bookkeeping yourself. It's
doing exactly what you want (allow adding and removing in LIFO order) and it's
efficient too.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable_box.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_box.dart:11: * [ObservableMixin].
Document that the key will be the string "value".

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:87: notifyChange('length', len, len + 1,
kind: ChangeRecord.FIELD);
Have you considered using Symbol for field names instead of strings.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:119: this.length -= length;
Why notifying about the REMOVE here too?
The setRange will notify about the writes copying later elements down, and the
length= will notify about the removes of the last (end-start) elements (which
are not null).
The number of removes will be correct if you remove the notifyChange calls
above.

Or should the length assignment and setRange have been on _list?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:134: this.length += insertionLength;
Again, this does a number of inserts at the end of the list, then setRange
copies elements, so why are the assignments below considered INSERT, not INDEX?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.dart
File sdk/lib/observe/observe.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.d...
sdk/lib/observe/observe.dart:8: * This library is used to observe changes to
[Observable] types. It also
I think the Observable name is too generic.
This is very much a *property-change* observable, and I think it should be part
of the name.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.d...
sdk/lib/observe/observe.dart:41: *       // Asynchronously schedules delivery of
these changes
"Schedules asynchronous ..."? 
(Nitpick - the scheduling happens now, the delivery happens asynchronously :).

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:256: final _spacesRegExp = new RegExp('\\s');
Consider using raw string: r'\s'
Not shorter, but easier to read.

[Lasse Reichstein Nielsen, 2013-05-06 11:08:21]

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:20: //     var path = observePath(123,
'foo.bar.baz.qux');
This needs to be symbols to work properly, without blowing up the dart2js
minifier.

[floitsch, 2013-05-06 17:37:46]

LGTM with comments.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:21: toObservable(value, {bool deep: true}) =>
This is non-extensible and doesn't really fit. It should not be part of the
public API of dart:observe.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:54: * You can use [ObservableMixin] as a base
class or mixin to implement this.
Don't make ObservableMixin a base class. Either provide ObservableBase or let
users write extends Object with ObservableMixin.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:58: // the async batching?
I would prefer if streams stay asynchronous. I just committed a change that
makes Future Completers send their values asynchronously. So things go rather in
that direction.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:66: Stream<List<ChangeRecord>> get changes;
Why is this not just a Stream<ChangeRecord> ?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:162: class ChangeRecord {
I'm ok with Observable being a Stream of ChangeRecords. But this class is more
than just a ChangeRecord. It is a highly specialized version for properties.
This is a PropertyChangeRecord (or similar).

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:240: int _hash4(w, x, y, z) => _hash2(_hash2(w,
x), _hash2(y, z));
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> This gives x and y the same multiplier (31), resulting in
>   w * 31^2 + (x+y) * 31 + z
> so it's symmetric in x and y which might give worse results. 
> At least it's used so that x is a key and y is a value, which are likely to
have
> different types.

_hash2(w, _hash2(x, _hash2(y, z))) should solve this without more overhead.
correct?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:255: void deliverChangeRecords() {
This looks completely wrong.

Why is there global state for Observables? Buffer the stream if that's
necessary, but don't put all callbacks into a global list.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:13: implements List<E> {
Add TODO: remove implements List<E> when switching to "extends ListBase<E>".

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:35: new ObservableList<E>()..addAll(other);
Is this on purpose that we get the initial values as observable change?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.dart
File sdk/lib/observe/observe.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.d...
sdk/lib/observe/observe.dart:13: *     class Monster extends ObservableMixin {
As written in the other file: I don't like to extend a Mixin. It feels wrong.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:20: //     var path = observePath(123,
'foo.bar.baz.qux');
On 2013/05/06 11:08:22, Lasse Reichstein Nielsen wrote:
> This needs to be symbols to work properly, without blowing up the dart2js
> minifier.

That also means that it must be split by hand. [const Symbol('foo'), const
Symbol('bar'), const Symbol('baz'), const Symbol('qux')].

[Jennifer Messerly, 2013-05-07 05:43:38]

Hey guys, thanks for feedback.

PTAL at API changes. High level:

* dart:observe renamed observe_impl, is now internal+undocumented like
dart:html_common.
* package:observe added, intended for end users
* ChangeRecord split so map/list stuff is not in base class
  - old/new value is gone
* uses Symbols for field names
* path observers gone; now exposed as DataBinding from dart:html
* use identical for comparison, except maps
* removed list summary, instead produce change records in desired form straight
from ObservableList.

Note#1: I have not tested this yet, so the code might have silly mistakes. I
want to make sure these new APIs look good before rewriting nearly every test
and spending a day testing all browsers again :)

Note#2: I'm scared of the long term maintenance costs of diverging so much from
the JS implementation. But we can give it a shot.

Note#3: there are high level issues for the Dart project to figure out regarding
minification+mirrors. But let's not let this change be blocked on that. A lot of
users are waiting for these features, and a lot of Web UI bugs are waiting for
this to get landed.

Cheers!

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:21: toObservable(value, {bool deep: true}) =>
On 2013/05/06 17:37:46, floitsch wrote:
> This is non-extensible and doesn't really fit. It should not be part of the
> public API of dart:observe.

Done -> moved to package:observe.

For what it's worth, the purpose of this: we got strong feedback that map and
list literals were too convenient, and using "new ObservableList.from" was too
verbose. That's why it is non-extensible--it only needs to support things that
Dart have Dart literals/JSON syntax. (I got rid of Set for this reason. I expect
some pushback from users though.)

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:54: * You can use [ObservableMixin] as a base
class or mixin to implement this.
On 2013/05/06 17:37:46, floitsch wrote:
> Don't make ObservableMixin a base class. Either provide ObservableBase or let
> users write extends Object with ObservableMixin.

Done, added ObservableBase.

I'm curious though, can you explain more? I saw this in ListBase which is
defined as "Object extends ListMixin". I didn't understand why we'd want to
define two type names that do the same thing. Just aesthetics?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:58: // the async batching?
On 2013/05/06 17:37:46, floitsch wrote:
> I would prefer if streams stay asynchronous. I just committed a change that
> makes Future Completers send their values asynchronously. So things go rather
in
> that direction.

Ah, interesting. Would StreamController.add also be async eventually?

TODO removed, btw.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:66: Stream<List<ChangeRecord>> get changes;
On 2013/05/06 17:37:46, floitsch wrote:
> Why is this not just a Stream<ChangeRecord> ?

It's important to get a batch of changes, so you can efficiently compute deltas
and perform updates. They learned this lesson the hard way with DOM mutation
events. RafaelW (the guy driving MutationObservers and MDV) can probably tell
add more details.

(In fact, the JavaScript version is much more powerful. It allows a single
observer callback to get batches for multiple objects. ChangeSummary offers a
nice API for this: github.com/rafaelw/ChangeSummary)

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:149: if (oldValue == newValue) return newValue;
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Consider using "identical" instead. That won't make assumptions about the ==
> operator on the values, and will catch all actual changes, not just changes to
> unequal values.

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:162: class ChangeRecord {
On 2013/05/06 17:37:46, floitsch wrote:
> I'm ok with Observable being a Stream of ChangeRecords. But this class is more
> than just a ChangeRecord. It is a highly specialized version for properties.
> This is a PropertyChangeRecord (or similar).

Agreed. I will try and split it:

* ObjectChangeRecord
* MapChangeRecord
* ListChangeRecord

Objects -- has the field name as Symbol
Map -- has the key, plus insert/remove/update
List -- has the list change delta, which is what we want anyway

It might take some surgery; do you have strong preference if this happens before
or after initial checkin? It might be easier to see/work with in a follow up CL.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:204: /** The previous value of the member. */
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Will this be null for an insert?

That's what I had thought. It turns out for a List, you need the original value
so you can reconstruct the previous list state.

Per Florian's comment above, I'll try restructuring this so:
* only MapChangeRecord cares about insert/remove
* ListChangeRecord contains change records in the form we want
* ObjectChangeRecord has no notion of insert/remove

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:208: final newValue;
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Will this be null for a remove?

yes

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:215: return other is ChangeRecord && kind ==
other.kind && key == other.key &&
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Consider using identical for the tests. At least the value tests. The key will
> likely be using == as equivalence in a Map.

I removed == and hashCode

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:238: int _hash3(x, y, z) => _hash2(_hash2(x, y),
z);
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Unused?

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:240: int _hash4(w, x, y, z) => _hash2(_hash2(w,
x), _hash2(y, z));
On 2013/05/06 17:37:46, floitsch wrote:
> On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> > This gives x and y the same multiplier (31), resulting in
> >   w * 31^2 + (x+y) * 31 + z
> > so it's symmetric in x and y which might give worse results. 
> > At least it's used so that x is a key and y is a value, which are likely to
> have
> > different types.
> 
> _hash2(w, _hash2(x, _hash2(y, z))) should solve this without more overhead.
> correct?

removed hash helpers :)
it would be great to have these in the standard libs/pkgs though. *nudge* :)

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:255: void deliverChangeRecords() {
On 2013/05/06 17:37:46, floitsch wrote:
> This looks completely wrong.

It does to me too :)
The capability is needed, but I hate having global state.

> 
> Why is there global state for Observables? Buffer the stream if that's
> necessary, but don't put all callbacks into a global list.

we discussed this offline, but basically:

I need a way to get a timeslice *after* all observers have propagated. This is
critical for testing, and for applications to access their completed DOM before
it renders.

It sounds like runAsync will queue something before the browser renders, but it
isn't clear how to get a timeslice that happens after all observers have
finished. I'm open to ideas on how to solve this.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:259: var deliverCallbacks = _deliverCallbacks;
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Consider using a Queue instead to avoid doing the bookkeeping yourself. It's
> doing exactly what you want (allow adding and removing in LIFO order) and it's
> efficient too.

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable_box.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_box.dart:11: * [ObservableMixin].
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Document that the key will be the string "value".

Done. Also moving this type to package:observe, and using Symbol

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:13: implements List<E> {
On 2013/05/06 17:37:46, floitsch wrote:
> Add TODO: remove implements List<E> when switching to "extends ListBase<E>".

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:35: new ObservableList<E>()..addAll(other);
On 2013/05/06 17:37:46, floitsch wrote:
> Is this on purpose that we get the initial values as observable change?

Mainly just a simplification. It shouldn't record changes, since there are no
observers.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:87: notifyChange('length', len, len + 1,
kind: ChangeRecord.FIELD);
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Have you considered using Symbol for field names instead of strings.

yeah. Chatted about this with Florian. It's tricky because dart:html bindings
must use String paths (like "foo.bar.baz"), so it ends up implying "new Symbol"
inside dart:html binding code. Granted, that code is tree shaken out unless
you're using the new MDV APIs, so it shouldn't have an effect for typical
programs.

Here's a compromise for now:
I'll change it to Symbol at the package:observe level, and move the path
observation functionality to dart:html only.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:119: this.length -= length;
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Why notifying about the REMOVE here too?
> The setRange will notify about the writes copying later elements down, and the
> length= will notify about the removes of the last (end-start) elements (which
> are not null).
> The number of removes will be correct if you remove the notifyChange calls
> above.
> 
> Or should the length assignment and setRange have been on _list?

good catch.
we need setRange to notify about the copies so summarizeListChanges will work.
But we can probably skip notification of the removal of the tail items.

I'm also going to try to follow up on the TODO to have List produce smarter
change records, which will be much nicer. would that be okay in a follow up CL?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable_list.dart:134: this.length += insertionLength;
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Again, this does a number of inserts at the end of the list, then setRange
> copies elements, so why are the assignments below considered INSERT, not
INDEX?

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.dart
File sdk/lib/observe/observe.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.d...
sdk/lib/observe/observe.dart:8: * This library is used to observe changes to
[Observable] types. It also
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> I think the Observable name is too generic.
> This is very much a *property-change* observable, and I think it should be
part
> of the name.

It's intended for all observable objects. List and Map usage is not exactly
"property change". What other kinds of observables did you have in mind?

My fear is that "PropertyChangeObservable" is really long and wordy, especially
once it gets mixed in or extended. The JS version is simply "Object.observe".

I moved to package:observe, does that alleviate some of the concern?

We have plenty of time to improve this! :) I would love to get more help :).
Observability is a huge topic; no doubt one could spend a lot of time making it
better and more comprehensive. Reactive programming is hugely beneficial for UI
work, and customers are already using our existing Observable library,
incomplete though it may be.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.d...
sdk/lib/observe/observe.dart:13: *     class Monster extends ObservableMixin {
On 2013/05/06 17:37:46, floitsch wrote:
> As written in the other file: I don't like to extend a Mixin. It feels wrong.

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe.d...
sdk/lib/observe/observe.dart:41: *       // Asynchronously schedules delivery of
these changes
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> "Schedules asynchronous ..."? 
> (Nitpick - the scheduling happens now, the delivery happens asynchronously :).

Done.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:20: //     var path = observePath(123,
'foo.bar.baz.qux');
On 2013/05/06 17:37:46, floitsch wrote:
> On 2013/05/06 11:08:22, Lasse Reichstein Nielsen wrote:
> > This needs to be symbols to work properly, without blowing up the dart2js
> > minifier.
> 
> That also means that it must be split by hand. [const Symbol('foo'), const
> Symbol('bar'), const Symbol('baz'), const Symbol('qux')].

Here's the best compromise I could come up with:

* switched to Symbols in package:observe
* moved path observation into dart:html Binding class, based on strings.

MDV supports strings specified in custom syntax, or bindings that come from HTML
(which can be created at runtime). Those strings cannot reasonably be renamed. I
agree it's a good goal to shoot for someday, but people get by just fine using
Angular JS and similar frameworks without renaming their data-bound fields.
Someday, we'll need to interoperate with JavaScript components too. While I
would like the long term goal of whole world program analysis and renaming
across HTML+CSS+JS+Dart, I think providing "no rename" option for certain
fields/types is a much more feasible strategy for short to medium term.

What I do want to avoid is contagion: just because I'm reflecting on certain
types/fields at runtime, as an app developer I don't want it to break
minification/tree shaking for the entire program. That's an important goal. But
this seems to be a general problem with mirrors right now.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:256: final _spacesRegExp = new RegExp('\\s');
On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> Consider using raw string: r'\s'
> Not shorter, but easier to read.

Done.

[Lasse Reichstein Nielsen, 2013-05-07 07:35:45]

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:240: int _hash4(w, x, y, z) => _hash2(_hash2(w,
x), _hash2(y, z));
It doesn't parallelize as well, but
 _hash2(_hash2(w, x) * 31, _hash2(y, z));
would give the same result.

[floitsch, 2013-05-07 13:40:47]

Thought a little bit more about this.
Could we?:
- rename it to dart:mdv_observe
- remove the queueChangeRecords and deliverChangeRecords. From what I can see we
can get the same functionality if we add a "pushSynchronous" to the Observables.
The stream would then push all queued events synchronously. It's not something
we like, but it should work.

[floitsch, 2013-05-07 14:46:47]

LGTM.

This version of the CL is much easier to digest...
I already replied earlier before I had looked at the latest version.
So (repeating myself): I think that calling dart:mdv_observe makes it more
geared towards your needs and thus avoids these "it's not generic enough"
complaints.

From what I can see there is not that much that is in a pub package now. If we
rename this library to dart:mdv-observe I (didn't talk to Lasse) would feel ok
with moving these classes back to this package. But let's start small.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/list_diff...
File sdk/lib/observe/list_diff.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/list_diff...
sdk/lib/observe/list_diff.dart:33: List<ListChangeDelta>
summarizeListChanges(List list,
Maybe this could be done as a transformer on a ListObservable.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:54: * You can use [ObservableMixin] as a base
class or mixin to implement this.
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 17:37:46, floitsch wrote:
> > Don't make ObservableMixin a base class. Either provide ObservableBase or
let
> > users write extends Object with ObservableMixin.
> 
> Done, added ObservableBase.
> 
> I'm curious though, can you explain more? I saw this in ListBase which is
> defined as "Object extends ListMixin". I didn't understand why we'd want to
> define two type names that do the same thing. Just aesthetics?

They can have different semantics. A mixin might depend on another mixin. A base
class is, by definition self-contained.
For example:
  ObservableMixin
  ListMixin
  ObservableListMixin  // requires ObservableMixin and ListMixin

  ObservableListBase extends Object with ObservableListMixin, ListMixin,
ObservableMixin.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:58: // the async batching?
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 17:37:46, floitsch wrote:
> > I would prefer if streams stay asynchronous. I just committed a change that
> > makes Future Completers send their values asynchronously. So things go
rather
> in
> > that direction.
> 
> Ah, interesting. Would StreamController.add also be async eventually?
> 
> TODO removed, btw.

Yes. This is planned.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:66: Stream<List<ChangeRecord>> get changes;
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 17:37:46, floitsch wrote:
> > Why is this not just a Stream<ChangeRecord> ?
> 
> It's important to get a batch of changes, so you can efficiently compute
deltas
> and perform updates. They learned this lesson the hard way with DOM mutation
> events. RafaelW (the guy driving MutationObservers and MDV) can probably tell
> add more details.
> 
> (In fact, the JavaScript version is much more powerful. It allows a single
> observer callback to get batches for multiple objects. ChangeSummary offers a
> nice API for this: github.com/rafaelw/ChangeSummary)

But wouldn't it be the task of a transformer to group these things? Maybe even
merge them (thus getting batches for multiple objects)?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:162: class ChangeRecord {
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 17:37:46, floitsch wrote:
> > I'm ok with Observable being a Stream of ChangeRecords. But this class is
more
> > than just a ChangeRecord. It is a highly specialized version for properties.
> > This is a PropertyChangeRecord (or similar).
> 
> Agreed. I will try and split it:
> 
> * ObjectChangeRecord
> * MapChangeRecord
> * ListChangeRecord
> 
> Objects -- has the field name as Symbol
> Map -- has the key, plus insert/remove/update
> List -- has the list change delta, which is what we want anyway
> 
> It might take some surgery; do you have strong preference if this happens
before
> or after initial checkin? It might be easier to see/work with in a follow up
CL.

fine with later. I don't like the "ObjectChangeRecord" name, though. Everything
is an Object in Dart. FieldChangeRecord?

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:255: void deliverChangeRecords() {
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 17:37:46, floitsch wrote:
> > This looks completely wrong.
> 
> It does to me too :)
> The capability is needed, but I hate having global state.
> 
> > 
> > Why is there global state for Observables? Buffer the stream if that's
> > necessary, but don't put all callbacks into a global list.
> 
> we discussed this offline, but basically:
> 
> I need a way to get a timeslice *after* all observers have propagated. This is
> critical for testing, and for applications to access their completed DOM
before
> it renders.
> 
> It sounds like runAsync will queue something before the browser renders, but
it
> isn't clear how to get a timeslice that happens after all observers have
> finished. I'm open to ideas on how to solve this.

I would rather add support for adding something to the end of the instant. Since
it doesn't exist yet, keep it, but keep reminding us.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:20: //     var path = observePath(123,
'foo.bar.baz.qux');
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 17:37:46, floitsch wrote:
> > On 2013/05/06 11:08:22, Lasse Reichstein Nielsen wrote:
> > > This needs to be symbols to work properly, without blowing up the dart2js
> > > minifier.
> > 
> > That also means that it must be split by hand. [const Symbol('foo'), const
> > Symbol('bar'), const Symbol('baz'), const Symbol('qux')].
> 
> Here's the best compromise I could come up with:
> 
> * switched to Symbols in package:observe
> * moved path observation into dart:html Binding class, based on strings.
> 
> MDV supports strings specified in custom syntax, or bindings that come from
HTML
> (which can be created at runtime). Those strings cannot reasonably be renamed.
I
> agree it's a good goal to shoot for someday, but people get by just fine using
> Angular JS and similar frameworks without renaming their data-bound fields.
> Someday, we'll need to interoperate with JavaScript components too. While I
> would like the long term goal of whole world program analysis and renaming
> across HTML+CSS+JS+Dart, I think providing "no rename" option for certain
> fields/types is a much more feasible strategy for short to medium term.
> 
> What I do want to avoid is contagion: just because I'm reflecting on certain
> types/fields at runtime, as an app developer I don't want it to break
> minification/tree shaking for the entire program. That's an important goal.
But
> this seems to be a general problem with mirrors right now.

Talked to Kasper. We should be able to mark classes as "Keep mapping from
String->Symbol for those."
If the Symbol rewrite is horrible try to ping Kasper and Peter to see if
something can be worked out.
I guess you still want to signal that you are using the marked set, since you
still want to get warnings in other parts of the code if people write new
Symbol(someString).
So maybe you will end up using new Symbol.table(someString). Maybe even
Symbol.preAllocated[someString].

https://codereview.chromium.org/14732003/diff/62001/pkg/observe/lib/src/obser...
File pkg/observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/62001/pkg/observe/lib/src/obser...
pkg/observe/lib/src/observable_list.dart:133: void insertAll(int index,
Iterable<E> iterable) {
Why is this not just:
var oldLength = _list.length;
_list.insertAll(index, iterable);
var insertionLength = _list.length - OldLength;
...

Replicating the behavior of ListBase seems wrong.

Same for all the other methods.

[blois, 2013-05-07 16:04:49]

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:149: if (oldValue == newValue) return newValue;
On 2013/05/07 05:43:38, John Messerly wrote:
> On 2013/05/06 10:58:58, Lasse Reichstein Nielsen wrote:
> > Consider using "identical" instead. That won't make assumptions about the ==
> > operator on the values, and will catch all actual changes, not just changes
to
> > unequal values.
> 
> Done.

We should have a clear contract on what constitutes a value change- either at
this level or the path observer level, there is a good case for == behavior.

Using identical is probably correct for the return value, but the event may only
fire on !=?

I'm fine with identical for now, but have had to move away from strict identical
in the past for performance reasons.

[Jennifer Messerly, 2013-05-07 16:29:57]

On 2013/05/07 13:40:47, floitsch wrote:
> Thought a little bit more about this.
> Could we?:
> - rename it to dart:mdv_observe

Sure. Do you prefer having a package too (also mdv_observe) or just dart: ? I
would like to feel comfortable adding helper utilities as it comes up. Perhaps
the package gives us more comfort on adding thing?

> - remove the queueChangeRecords and deliverChangeRecords. From what I can see
we
> can get the same functionality if we add a "pushSynchronous" to the
Observables.
> The stream would then push all queued events synchronously. It's not something
> we like, but it should work.

You mean as a static? It has to be static because some bindings are created
inside dart:html and not exposed to the user. So they have no way to push those
except via this method. It was the only way I could make the tests work--my
original CL had a per-Observable deliverChanges instance method.

[Jennifer Messerly, 2013-05-09 18:01:29]

Meant to sent this out again yesterday.
I think I've got the final version; no major changes since the "API updates" I
sent out. Primarily a few renames:
* Pete asked me to rename DataBinding -> PathObserver, which coincidentally is
what MDV v3 is calling it.
* I used the word "property" in place of "field".

The big test change is using maps with Symbol keys instead of Strings. I'm not
sure it works well for the data binding of JSON scenario, but it's good enough
for the data-binding a Dart object which is the main scenario to enable. We can
revisit later if there's a way to make Maps work better w/ data binding.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
File sdk/lib/observe/observable.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:66: Stream<List<ChangeRecord>> get changes;
On 2013/05/07 14:46:48, floitsch wrote:
> On 2013/05/07 05:43:38, John Messerly wrote:
> > On 2013/05/06 17:37:46, floitsch wrote:
> > > Why is this not just a Stream<ChangeRecord> ?
> > 
> > It's important to get a batch of changes, so you can efficiently compute
> deltas
> > and perform updates. They learned this lesson the hard way with DOM mutation
> > events. RafaelW (the guy driving MutationObservers and MDV) can probably
tell
> > add more details.
> > 
> > (In fact, the JavaScript version is much more powerful. It allows a single
> > observer callback to get batches for multiple objects. ChangeSummary offers
a
> > nice API for this: github.com/rafaelw/ChangeSummary)
> 
> But wouldn't it be the task of a transformer to group these things? Maybe even
> merge them (thus getting batches for multiple objects)?

Yeah, as long as such a thing can be built, that would be enough for me. You'd
need a way for the stream to receive all pending records in a single operation.

Actually--do streams support this already? It seems like something you might
want for I/O: 'give me all the bytes so far'

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observabl...
sdk/lib/observe/observable.dart:162: class ChangeRecord {
On 2013/05/07 14:46:48, floitsch wrote:
> On 2013/05/07 05:43:38, John Messerly wrote:
> > On 2013/05/06 17:37:46, floitsch wrote:
> > > I'm ok with Observable being a Stream of ChangeRecords. But this class is
> more
> > > than just a ChangeRecord. It is a highly specialized version for
properties.
> > > This is a PropertyChangeRecord (or similar).
> > 
> > Agreed. I will try and split it:
> > 
> > * ObjectChangeRecord
> > * MapChangeRecord
> > * ListChangeRecord
> > 
> > Objects -- has the field name as Symbol
> > Map -- has the key, plus insert/remove/update
> > List -- has the list change delta, which is what we want anyway
> > 
> > It might take some surgery; do you have strong preference if this happens
> before
> > or after initial checkin? It might be easier to see/work with in a follow up
> CL.
> 
> fine with later. I don't like the "ObjectChangeRecord" name, though.
Everything
> is an Object in Dart. FieldChangeRecord?

Funny, I had the same thought last night after I went home. FieldChangeRecord
sounds great.

update: on further reflection I renamed "field" to "property" terminology.

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
File sdk/lib/observe/observe_path.dart (right):

https://codereview.chromium.org/14732003/diff/25038/sdk/lib/observe/observe_p...
sdk/lib/observe/observe_path.dart:20: //     var path = observePath(123,
'foo.bar.baz.qux');
On 2013/05/07 14:46:48, floitsch wrote:
> On 2013/05/07 05:43:38, John Messerly wrote:
> > On 2013/05/06 17:37:46, floitsch wrote:
> > > On 2013/05/06 11:08:22, Lasse Reichstein Nielsen wrote:
> > > > This needs to be symbols to work properly, without blowing up the
dart2js
> > > > minifier.
> > > 
> > > That also means that it must be split by hand. [const Symbol('foo'), const
> > > Symbol('bar'), const Symbol('baz'), const Symbol('qux')].
> > 
> > Here's the best compromise I could come up with:
> > 
> > * switched to Symbols in package:observe
> > * moved path observation into dart:html Binding class, based on strings.
> > 
> > MDV supports strings specified in custom syntax, or bindings that come from
> HTML
> > (which can be created at runtime). Those strings cannot reasonably be
renamed.
> I
> > agree it's a good goal to shoot for someday, but people get by just fine
using
> > Angular JS and similar frameworks without renaming their data-bound fields.
> > Someday, we'll need to interoperate with JavaScript components too. While I
> > would like the long term goal of whole world program analysis and renaming
> > across HTML+CSS+JS+Dart, I think providing "no rename" option for certain
> > fields/types is a much more feasible strategy for short to medium term.
> > 
> > What I do want to avoid is contagion: just because I'm reflecting on certain
> > types/fields at runtime, as an app developer I don't want it to break
> > minification/tree shaking for the entire program. That's an important goal.
> But
> > this seems to be a general problem with mirrors right now.
> 
> Talked to Kasper. We should be able to mark classes as "Keep mapping from
> String->Symbol for those."
> If the Symbol rewrite is horrible try to ping Kasper and Peter to see if
> something can be worked out.
> I guess you still want to signal that you are using the marked set, since you
> still want to get warnings in other parts of the code if people write new
> Symbol(someString).
> So maybe you will end up using new Symbol.table(someString). Maybe even
> Symbol.preAllocated[someString].

That would be perfect!

[Siggi Cherem (dart-lang), 2013-05-09 18:49:35]

lgtm

https://codereview.chromium.org/14732003/diff/70002/pkg/mdv_observe/lib/src/o...
File pkg/mdv_observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/70002/pkg/mdv_observe/lib/src/o...
pkg/mdv_observe/lib/src/observable_list.dart:244: items.removeRange(r.index,
r.index + r.removedCount);
I assume items.removeRange(x, x) is a noop? or would list throw an error in that
case?

https://codereview.chromium.org/14732003/diff/70002/pkg/mdv_observe/lib/src/o...
pkg/mdv_observe/lib/src/observable_list.dart:271: int actualIndex = index <
items.length ? items[index] : oldLength;
it took me a while to understand these two lines, but they make sense once you
draw an example on paper. 

Maybe it can help if we rename actualIndex and expectedIndex. It seems that we
have 2 concepts named 'index', the values stored in the items (which are
indices) and the meta-index represented by the index variable. So maybe we can
call them actualItem/exectedItem or actualValue/expectedValue?

[Jennifer Messerly, 2013-05-09 18:55:40]

https://codereview.chromium.org/14732003/diff/70002/pkg/mdv_observe/lib/src/o...
File pkg/mdv_observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/14732003/diff/70002/pkg/mdv_observe/lib/src/o...
pkg/mdv_observe/lib/src/observable_list.dart:244: items.removeRange(r.index,
r.index + r.removedCount);
On 2013/05/09 18:49:35, Siggi Cherem (dart-lang) wrote:
> I assume items.removeRange(x, x) is a noop? or would list throw an error in
that
> case?

yup! it's a no-op

https://codereview.chromium.org/14732003/diff/70002/pkg/mdv_observe/lib/src/o...
pkg/mdv_observe/lib/src/observable_list.dart:271: int actualIndex = index <
items.length ? items[index] : oldLength;
On 2013/05/09 18:49:35, Siggi Cherem (dart-lang) wrote:
> it took me a while to understand these two lines, but they make sense once you
> draw an example on paper. 
> 
> Maybe it can help if we rename actualIndex and expectedIndex. It seems that we
> have 2 concepts named 'index', the values stored in the items (which are
> indices) and the meta-index represented by the index variable. So maybe we can
> call them actualItem/exectedItem or actualValue/expectedValue?

good idea! went with *Item

[Jennifer Messerly, 2013-05-09 20:29:30]

Committed patchset #12 manually as r22561 (presubmit successful).

[Jennifer Messerly, 2013-05-09 21:39:16]

Committed patchset #14 manually as r22570 (presubmit successful).

[ahe, 2013-05-15 12:46:00]

https://codereview.chromium.org/14732003/diff/81001/tests/compiler/dart2js/an...
File tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/14732003/diff/81001/tests/compiler/dart2js/an...
tests/compiler/dart2js/analyze_api_test.dart:30: 'html_dart2js.dart': const
['Warning: Using "new Symbol"'], // Issue 10565.
This change is not cool.

By supressing this warning and using new Symbol in dart:html, you're making it
impossible for the dart2js team to implement mirrors and minification without
bloating *all* programs.

Also, why did you not consult with the dart2js team before submitting this CL?

[Jennifer Messerly, 2013-05-15 17:59:30]

https://codereview.chromium.org/14732003/diff/81001/tests/compiler/dart2js/an...
File tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/14732003/diff/81001/tests/compiler/dart2js/an...
tests/compiler/dart2js/analyze_api_test.dart:30: 'html_dart2js.dart': const
['Warning: Using "new Symbol"'], // Issue 10565.
On 2013/05/15 12:46:00, ahe wrote:
> This change is not cool.
> 
> By supressing this warning and using new Symbol in dart:html, you're making it
> impossible for the dart2js team to implement mirrors and minification without
> bloating *all* programs.

This is not correct. "new Symbol" is only required for *some* of the dart:html
APIs. If you aren't using them, you shouldn't pay for it.

In any case, as I have explained elsewhere, Model-Driven-Views needs mirrors. So
we must figure out a way to make mirrors not all-or-nothing.