Support for observable models, fixes #259

Adds support for Observable objects in web_ui.
This includes @observable annotation for automatic transformation by DWC. It also includes ObservableList/Map/Set types.

Observables use a batch-notification system where changes are queued up and sent to the handler during the "next microtask". This is similar to ES7 Object.observe proposal.

We don't have detailed change records yet but I plan to work on that in a follow up change. Keep that in mind when reviewing ObservableList/Map/Set types because they're in for an overhaul. The CL for that isn't ready but for comparison purposes you can find it here: https://chromiumcodereview.appspot.com/12096106/

All tests have been converted. Since I had to go through every single test, I took the liberty of removing incorrect <title> attributes throughout. 

I still need to convert the MDV and explainer examples. I'm also interested in more perf tests. I'll work on that now, but would prefer not expanding the scope of this (already big) change :)

Feature request: https://github.com/dart-lang/web-ui/issues/259

Committed: https://github.com/dart-lang/web-ui/commit/749924d

[Siggi Cherem (dart-lang), 2013-02-13 01:43:24]

This is looking awesome John!

I'm still working through it, but I have to interrupt for a bit, so I wanted to
send you some preliminary comments.

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/ed...
File example/todomvc/editable_label.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/ed...
example/todomvc/editable_label.html:48: deliverChangesSync();
interesting that we needed this... I wonder if we only need this because we
don't have a way to express 'on-enter' or 'on-insert' (which would be triggered
when the element is added)

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/mo...
File example/todomvc/model.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/mo...
example/todomvc/model.dart:49: // TODO(jmesserly): should methods on
ObservableList return Observables?
Seems reasonable to do so...

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:7: * Similar in spirit to EcmaScript 6
6 => 7
(and below)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:15: // batches are tracked on a per-observed expression basis,
instead of per-observer basis.
long line

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:23: import 'src/utils.dart' show setImmediate;
funny, I never considered using 'src/utils.dart' for the runtime apps (only for
the compiler) =)

FYI - not necessarily for this change, but we should get rid of setImmediate and
use Future.immedate instead. Future.immediate is already asynchronous, but I
think right now it is doing setTimout(0) instead of setImmediate. Once that's
fixed, we should switch to it.

See: 
https://groups.google.com/a/dartlang.org/forum/?hl=en&fromgroups=#!searchin/m...

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:35: // implementating advanced observable functionality. The
need to be public, but
The => they

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:114: * [onObserveUnhandledError]
add '.' at the end of the line

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:164: Object notifyRead(fieldObservers) {
maybe document the possible types for fieldObservers?
(_ExpressionObserver, and List?)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:177: }
maybe assert (fieldObservers is List)?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:184: * non-null list of [observers]. For example:
would it be sensible to accept a null list and return immediately?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:227: int circularNotificationLimit = 100;
100 feels big, but maybe because in watchers 10 felt big? What's the longest
number you have reached in some common samples (e.g. in jacob's app?)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:260: // an unnecessary copy. If that gets fixed we can use
.values here.
should we file a bug?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:272: void _diagnoseCircularLimit(Map<int, _ExpressionObserver>
pendingObservers) {
would it also be possible to detect circularity during notifyRead (possible
future TODO)?

I think we briefly chatted about this last week, but basically I'm thinking
that, if the user turns on a flag, we can detect if there are any writes while
doing the read-barriers. If we detect that, we could throw an exception, which
will point the user to the violating write.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:287: i++;
consider only incrementing and printing the top-10 observers that actually
changed?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:294: // We throw an error because it probably means a bug in
the program structure.
In watchers it was nice that we kept going - a bug in one part of the app will
not make the entire app stop.

Alternatively, report it to the onObserverUnhandledError?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:332: // but we don't need it in observers, except to pass it to
the ChangeObserver.
In that case, wouldn't an event get trigger as soon as you nullify the
expression? in which case the reference will be gone very soon afterwards (on
the next event cycle?)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:408: if (_dead) return null;
can _dead change in _observe?, can _dead change in ==? I guess it could if the
user has side-effects in their expression or operator==... is that the purpose
for these extra checks? and the try-catch block in line 411?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/html.dart
File lib/observe/html.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/html.d...
lib/observe/html.dart:5: // TODO(jmesserly): can we handle this better? This
seems like an unforunate
nit: unforunate -> unfor[t]unate

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/html.d...
lib/observe/html.dart:6: // limitation of our read barriers.
I guess there is the general problem of handling native properties (similarly
with dom properties). I wonder how far can we get by using mutation observers...

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.dart
File lib/observe/list.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:46: // See
http://code.google.com/p/dart/issues/detail?id=947
947 seems to be marked as fixed... it seems that 'void remove(Object)' is in
Collection.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:58: bool contains(E item) => Collections.contains(_list,
item);
Are you using Collections to skip over any overhead from observables that comes
from using _list.contains(item)?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:109: // methods above, so they correctly track
reads/writes.
consider moving contains, insertAt, first, and iterator below this line?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:209: // TODO(jmesserly): copy+paste from collection-dev
I assume collection-dev can't be used, or we shoudln't use it?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.da...
lib/observe/map.dart:111: _notifyWriteLength(len);
should this be separate? I mean,

if (oldValue != value) _notifyWriteKey(key);
if (len != map.length) _notifyWriteLengh(len);

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/set.dart
File lib/observe/set.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/set.da...
lib/observe/set.dart:17: class ObservableSet<E> extends Collection<E> implements
Set<E> {
Any reasons why not to wrap observable map instead? Then this code could be an
abstract wrapper that has no observers of it's own.

Ideally - the core libraries should let us wrap any map as a set (see:
https://code.google.com/p/dart/issues/detail?id=5603). I saw a TODO from florian
that map.keys should return a set, maybe this will come along =)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/compiler.d...
lib/src/compiler.dart:249: // TODO(jmesserly): what about
ObservableList/Map/Set?
how about if you have an import to those libraries (check in _processImports)?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/compiler.d...
lib/src/compiler.dart:371: .relativeTo(fileOutputPath.directoryPath).toString();
newUri = _pathInfo.relativePath(library, importInfo).toString();

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
File lib/src/dart_parser.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:69: String codeAfterDirectives() =>
code.substring(directivesEnd);
nit: make it a property?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:85: return (directive as
NamespaceDirective).libraryUri;
if you haven't, it might be nice to give them feedback on this type (e.g. make
Directive have a getter/setter for uri?)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:93: (directive as NamespaceDirective).libraryUri2 =
uri;
ohh weird - bad code generation =)...

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/emitters.d...
lib/src/emitters.dart:605: var libraryName = codeInfo.libraryName;
unused variable

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/info.dart#...
lib/src/info.dart:118: if (outputPath.isAbsolute) {
how did you run into this?

[Jennifer Messerly, 2013-02-13 05:43:15]

Thanks Siggi!

Addressed comments so far -- PTAL

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/ed...
File example/todomvc/editable_label.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/ed...
example/todomvc/editable_label.html:48: deliverChangesSync();
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> interesting that we needed this... I wonder if we only need this because we
> don't have a way to express 'on-enter' or 'on-insert' (which would be
triggered
> when the element is added)

Agreed, that's the issue here. on-insert would be nice.

Another idea is to refactor it using observe or setImmediate. I was kinda being
lazy and trying to avoid testing IE again :). Maybe if I add a TODO here?

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/mo...
File example/todomvc/model.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/example/todomvc/mo...
example/todomvc/model.dart:49: // TODO(jmesserly): should methods on
ObservableList return Observables?
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> Seems reasonable to do so...

Yeah. The only tricky bit is what if you want a normal list?

It definitely is interesting. One thing I don't like so far is how
ObservableLists are more in-your-face. (But then again observable systems can be
even more intrustive than this ...)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:7: * Similar in spirit to EcmaScript 6
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> 6 => 7
> (and below)

yeah. Or maybe "EcmaScript Harmony"--to match the wiki? I dunno how "committed"
ECMAScript is to shipping it in any particular version. I kind of don't trust ES
to stick to a particular version until something actually shows up in the draft
spec, especially since it changed already since I wrote this :)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:15: // batches are tracked on a per-observed expression basis,
instead of per-observer basis.
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> long line

Done.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:23: import 'src/utils.dart' show setImmediate;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> funny, I never considered using 'src/utils.dart' for the runtime apps (only
for
> the compiler) =)
> 
> FYI - not necessarily for this change, but we should get rid of setImmediate
and
> use Future.immedate instead. Future.immediate is already asynchronous, but I
> think right now it is doing setTimout(0) instead of setImmediate. Once that's
> fixed, we should switch to it.
> 
> See: 
>
https://groups.google.com/a/dartlang.org/forum/?hl=en&fromgroups=#%21searchin...

Agreed, I hope our version goes away soon. Added TODO!

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:35: // implementating advanced observable functionality. The
need to be public, but
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> The => they

Done. Good catch!

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:114: * [onObserveUnhandledError]
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> add '.' at the end of the line

Done.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:164: Object notifyRead(fieldObservers) {
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> maybe document the possible types for fieldObservers?
> (_ExpressionObserver, and List?)

Added comment inside the method. I intentionally don't want to say anything in
the public API.

Hoping this all gets replaced with detailed change records though.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:177: }
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> maybe assert (fieldObservers is List)?

Trying to avoid anything here that will make this code slower. VM should throw a
NoSuchMethod error if "add" doesn't work. The idea is notifyRead/notifyWrite are
very much internal use-at-your-own-risk APIs.

(That's why I don't like them and want to replace with something more friendly)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:184: * non-null list of [observers]. For example:
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> would it be sensible to accept a null list and return immediately?

It's faster to avoid the call in the common case where you aren't observing
anything. (Also avoids the equality compare).

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:227: int circularNotificationLimit = 100;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> 100 feels big, but maybe because in watchers 10 felt big? What's the longest
> number you have reached in some common samples (e.g. in jacob's app?)

Watchers are very different :). It meant you're computing every watched
expression in your entire program 10 times. It was 10x10000 == slow.

With observers, it can be more like 100x2 == very fast. 500 times faster than
the watcher limit :)

Also keep in mind Object.observe has no limit ("Object.deliverChangeRecords now
calls [[DeliverChangeRecords]] repeatedly until there no pending records to
deliver")

Apidoc is a simple read-only data viewer, I doubt it gets anywhere past 2, but
haven't measured. It couldn't be going by 10 -- because of our old limit :)

I actually suspect 100 is far too small to build anything like a spreadsheet in
it, which is why it's configurable. But maybe it should be a lot higher. It's
only intended as a way of breaking you out of your circular loop so you can
debug what went wrong.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:260: // an unnecessary copy. If that gets fixed we can use
.values here.
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> should we file a bug?

Done.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:272: void _diagnoseCircularLimit(Map<int, _ExpressionObserver>
pendingObservers) {
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> would it also be possible to detect circularity during notifyRead (possible
> future TODO)?
> 
> I think we briefly chatted about this last week, but basically I'm thinking
> that, if the user turns on a flag, we can detect if there are any writes while
> doing the read-barriers. If we detect that, we could throw an exception, which
> will point the user to the violating write.

Interesting idea. Essentially, a runtime check for purity in observed
expressions, like this?

notifyWrite(...) {
  if (observeReads) throw PurityError('your code is impure and must be purged.
Deleting it in 5..4..3..2..1..Gone!');
  ...
}

I think it's the write barrier, though, not the read barrier where we have a
problem.

It seems doable--I'll add a TODO. I think we saw at least one example where it
was a problem.

However circular references are usually not caused by violating purity, it's
more common that it's caused by the change notification callback. And callbacks
definitely need to be able to run imperative code.

I'm not sure how we can really detect that. If A -> B, then next batch B -> C,
then next back B -> A, is it a cycle? Maybe. Or maybe it stabilizes. Unless you
can solve the halting problem :), the only choice is to restrict the
computations people can do with observers. That's what we're doing with the
arbitrary 100 limit.

Spreadsheets have an advantage here -- they can statically determine there is a
cycle between cells. No such luck for us.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:287: i++;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> consider only incrementing and printing the top-10 observers that actually
> changed? 

Great idea. Done.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:294: // We throw an error because it probably means a bug in
the program structure.
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> In watchers it was nice that we kept going - a bug in one part of the app will
> not make the entire app stop.
> 
> Alternatively, report it to the onObserverUnhandledError?

Not sure you really want the application to keep going, though. After you throw
away the pending writes you're in an inconsistent state. If you do want your app
to keep going, it seems like turning off the limit by default might be the way
to go.

This all gets to what is the right policy for Dart to deal with unhandled errors
in events. In JS it handles them somewhat gracefully, whereas in Dartium it's
catastrophic. But that seems like more of a Dartium problem.

onObserverUnhandledError feels wrong here, but I implemented a related idea --
another callback for the diagnostic. Does it look better this way?

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:332: // but we don't need it in observers, except to pass it to
the ChangeObserver.
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> In that case, wouldn't an event get trigger as soon as you nullify the
> expression? in which case the reference will be gone very soon afterwards (on
> the next event cycle?)

Good point. Removed comment!

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:408: if (_dead) return null;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> can _dead change in _observe?, can _dead change in ==? I guess it could if the
> user has side-effects in their expression or operator==... is that the purpose
> for these extra checks? and the try-catch block in line 411?

added comment:

    // Note: whenever we run code we don't control, we need to check _dead again
    // in case they have unobserved this object. This means `_observe`, `==`,
    // need to check.

(This is another optimization I'll need to remove to make detailed change
records work).

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/html.dart
File lib/observe/html.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/html.d...
lib/observe/html.dart:5: // TODO(jmesserly): can we handle this better? This
seems like an unforunate
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> nit: unforunate -> unfor[t]unate

What an unforunate typo!

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/html.d...
lib/observe/html.dart:6: // limitation of our read barriers.
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> I guess there is the general problem of handling native properties (similarly
> with dom properties). I wonder how far can we get by using mutation
observers...

yeah, I dunno. Mostly we aren't observing the DOM though, so I think we should
be okay, as long as properties like location.hash are exposed.

I worry about discoverability though.

Yet another reason to encourage folks to use a routing package I guess.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.dart
File lib/observe/list.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:46: // See
http://code.google.com/p/dart/issues/detail?id=947
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> 947 seems to be marked as fixed... it seems that 'void remove(Object)' is in
> Collection.

yeah. It seems to be abstract though, so we still need an implementation.
Removed the TODO though.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:58: bool contains(E item) => Collections.contains(_list,
item);
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> Are you using Collections to skip over any overhead from observables that
comes
> from using _list.contains(item)?

I think _list.contains would have less overhead. But Collections.contains will
use our []-indexer, and cause read notifications to happen.

It'll make more sense when I move it down (as you suggested)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:109: // methods above, so they correctly track
reads/writes.
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> consider moving contains, insertAt, first, and iterator below this line?

good call

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/list.d...
lib/observe/list.dart:209: // TODO(jmesserly): copy+paste from collection-dev
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> I assume collection-dev can't be used, or we shoudln't use it?

yup. According to Florian/Lasse everything you need for implementing
List/Iterable etc, will be exposed through dart:collections. it's a work in
progress at the moment.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.da...
lib/observe/map.dart:111: _notifyWriteLength(len);
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> should this be separate? I mean,
> 
> if (oldValue != value) _notifyWriteKey(key);
> if (len != map.length) _notifyWriteLengh(len);

Not quite, it was a bit tricky to get right. Added comment:

    // Note: if length changed, it means the key was added, so we need to
    // _notifyWriteKey. Also _notifyWriteLength will check if length changed.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/set.dart
File lib/observe/set.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/set.da...
lib/observe/set.dart:17: class ObservableSet<E> extends Collection<E> implements
Set<E> {
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> Any reasons why not to wrap observable map instead? Then this code could be an
> abstract wrapper that has no observers of it's own.
> 
> Ideally - the core libraries should let us wrap any map as a set (see:
> https://code.google.com/p/dart/issues/detail?id=5603). I saw a TODO from
florian
> that map.keys should return a set, maybe this will come along =)
> 

Yeah that would be cool. Given any Map impl you should be able to get a Set.

I tried it twice, both here and in
https://codereview.chromium.org/12096106/diff/1/lib/observe/set.dart but
abandoned the approach, for reasons I no longer recall :(. The code seemed
simpler doing it this way. But there are so many constraints right now -- like
not having mixins, dart:collection in flux, etc, that I'm open to revisiting in
the future. Will add TODO.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/compiler.d...
lib/src/compiler.dart:249: // TODO(jmesserly): what about
ObservableList/Map/Set?
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> how about if you have an import to those libraries (check in _processImports)?

Yeah, the tricky part is they can be imported transitively via
package:web_ui/web_ui.dart, and everybody imports that.

But it does seems good to check for "package:web_ui/observe" and it might catch
some more cases. Good idea, done!

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/compiler.d...
lib/src/compiler.dart:371: .relativeTo(fileOutputPath.directoryPath).toString();
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> newUri = _pathInfo.relativePath(library, importInfo).toString();

Done.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
File lib/src/dart_parser.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:69: String codeAfterDirectives() =>
code.substring(directivesEnd);
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> nit: make it a property?

It could be repeated O(N)'s if used that way. Trying to encourage people to
cache it in a local.

http://www.dartlang.org/articles/style-guide/#do-use-a-getter-for-operations-...
"it's fast"

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:85: return (directive as
NamespaceDirective).libraryUri;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> if you haven't, it might be nice to give them feedback on this type (e.g. make
> Directive have a getter/setter for uri?)

Already done. I think it's already fixed, but not in our SDK yet :(

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:93: (directive as NamespaceDirective).libraryUri2 =
uri;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> ohh weird - bad code generation =)... 

hehe, yeah. I think Konstantin is looking into it

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/emitters.d...
lib/src/emitters.dart:605: var libraryName = codeInfo.libraryName;
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> unused variable

Done. Also fixed "pressent" typo I had copied

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/info.dart#...
lib/src/info.dart:118: if (outputPath.isAbsolute) {
On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> how did you run into this?

I don't recall :(

It was either apidoc or tests. One of them had an interesting import pattern. If
you'd like I can try and recreate it.

You can see here though:
http://api.dartlang.org/docs/releases/latest/dart_io/Path.html#segments

... it appears segments doesn't correctly handle absolute path. So I don't see
how this code could've worked if outputPath was absolute?

[Siggi Cherem (dart-lang), 2013-02-13 19:28:54]

... and here is the second round!

This is super cool!

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:272: void _diagnoseCircularLimit(Map<int, _ExpressionObserver>
pendingObservers) {
On 2013/02/13 05:43:15, John Messerly wrote:
> On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > would it also be possible to detect circularity during notifyRead (possible
> > future TODO)?
> > 
> > I think we briefly chatted about this last week, but basically I'm thinking
> > that, if the user turns on a flag, we can detect if there are any writes
while
> > doing the read-barriers. If we detect that, we could throw an exception,
which
> > will point the user to the violating write.
> 
> Interesting idea. Essentially, a runtime check for purity in observed
> expressions, like this?
> 
> notifyWrite(...) {
>   if (observeReads) throw PurityError('your code is impure and must be purged.
> Deleting it in 5..4..3..2..1..Gone!');
>   ...
> }
> 
> I think it's the write barrier, though, not the read barrier where we have a
> problem.

Yeah - basically I think of it as a combination of the two: a write barrier
while our read barriers are on (when we are observing reads). I'd then suggest
that it's ok to have impure expressions, as long as the writes are not affecting
the same expressions that you are reading. (You might want some side-effects for
logging and other purposes).

> 
> It seems doable--I'll add a TODO. I think we saw at least one example where it
> was a problem.
> 
> However circular references are usually not caused by violating purity, it's
> more common that it's caused by the change notification callback. And
callbacks
> definitely need to be able to run imperative code.

Good point, that's more likely the case...

> 
> I'm not sure how we can really detect that. If A -> B, then next batch B -> C,
> then next back B -> A, is it a cycle? Maybe. Or maybe it stabilizes. Unless
you
> can solve the halting problem :), the only choice is to restrict the
> computations people can do with observers. That's what we're doing with the
> arbitrary 100 limit.
> 
> Spreadsheets have an advantage here -- they can statically determine there is
a
> cycle between cells. No such luck for us.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.da...
lib/observe/map.dart:111: _notifyWriteLength(len);
On 2013/02/13 05:43:15, John Messerly wrote:
> On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > should this be separate? I mean,
> > 
> > if (oldValue != value) _notifyWriteKey(key);
> > if (len != map.length) _notifyWriteLengh(len);
> 
> Not quite, it was a bit tricky to get right. Added comment:
> 
>     // Note: if length changed, it means the key was added, so we need to
>     // _notifyWriteKey. Also _notifyWriteLength will check if length changed.

I see, so basically you are handling when the value is 'null'.

I was also thinking that we don't want to notify write on 'length' when you are
just replacing the value, though. Maybe it should be more like this?
   if (oldValue != value || len != map.length) _notifyWriteKey(key);
   if (len != map.length) _notifyWriteLengh(len);

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
File lib/src/dart_parser.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:69: String codeAfterDirectives() =>
code.substring(directivesEnd);
On 2013/02/13 05:43:15, John Messerly wrote:
> On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > nit: make it a property?
> 
> It could be repeated O(N)'s if used that way. Trying to encourage people to
> cache it in a local.
Shouldn't good inlining take care of that in this particular case?

> 
>
http://www.dartlang.org/articles/style-guide/#do-use-a-getter-for-operations-...
> "it's fast"

I think we forgot to update the style guide, but I recall we decided that
'speed' was no longer a requirement for a getter. It might be desirable, but not
required. I think we were leaning this way in part to make keys/values be
getters, but also because with lazy initialization even fields can be slow.

If we prefer to keep it as a method, then we need to rename it to something more
like verb (I think we also decided this on that discussion, but I can't find it
in the style guide).

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe.dart#ne...
lib/observe.dart:287: if (i < 10) {
nit: combine with previous condition?

if (change == null || i >= 10) return;

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe.dart#ne...
lib/observe.dart:321: * If it wasn't read, we can free it immediately
missing ending '.', same in the next comment.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe/map.dar...
lib/observe/map.dart:124: // _notifyWriteKey. Also _notifyWriteLength will check
if length changed.
yeah, this one is more obvious, because if it is not absent, nothing will
change.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/info.dart#n...
lib/src/info.dart:174: bool get codeAttached => userCode != null || externalFile
!= null;
I can see how this works, but it's a bit confusing because userCode can have
both inline or external code.

I think it's ok as it is, but here is a possibly wasteful suggestion that might
make things clearer. How about we put back inlinedCode, but make it a
DartCodeInfo. Then userCode is simply a getter:

DartCodeInfo get userCode =>
  externalFile != null ? externalFile.userCode : inlinedCode

Thoughts?

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
File lib/src/observable_transform.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:33: return false;
would this fit in the prev line?

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:42: } else {
nit: remove else...

if (identical(...)) return false;
// Replace the code ...

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:133: // AFTER:  Foo(bar, baz) : __$bar = bar,
__$baz = baz { ... }
why can't AFTER be:

Foo(this.__$bar, this.__$baz)?

Or is this mainly to handle named arguments?
Foo({this.bar});  ==>  Foo({bar}): __$bar = bar;

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:175: var type = 'dynamic';
do you prefer 'dynamic' over 'var'?

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:178: }
totally optional nit, consider using ternary expressions here and below?

var type = fields.type != null ? getOriginalCode(code, fields.type) : 'var';

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/refactor.dart
File lib/src/refactor.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/refactor.da...
lib/src/refactor.dart:65: consumed += edit.length;
alternatively, get rid of line 61 and write:
    consumed = edit.end;
here?

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/refactor.da...
lib/src/refactor.dart:87: String guessIndent(String code, int charOffset) {
neat! =)

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/templating.dart
File lib/templating.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/templating.dart...
lib/templating.dart:15: export 'src/utils.dart' show setImmediate;
unused import

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/com...
File test/data/input/component_field_2way_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/com...
test/data/input/component_field_2way_test.html:25: // a different property.
strange, because the purpose of 'title' in this test was exactly to bind DOM
attributes... maybe we should create an issue to keep track of this and
investigate how far can we get with mutation observers?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
File test/data/input/conditional4_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
test/data/input/conditional4_test.html:29: deliverChangesSync();
are these needed? with setTimeout (or setImmediate) the deliver should happen
now automatically =)

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
File test/data/input/conditional5_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
test/data/input/conditional5_test.html:23: deliverChangesSync();
same here and in other tests...

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/css...
File test/data/input/css_classes_binding_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/css...
test/data/input/css_classes_binding_test.html:33: @observable
yay! I was waiting for a test like this one =)

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/css...
test/data/input/css_classes_binding_test.html:52: deliverChangesSync();
remove?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/rec...
File test/data/input/recursive_component_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/rec...
test/data/input/recursive_component_test.html:24: @observable
as soon as we switch, would we want to make all subclasses of WebComponent
observable by default?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/sty...
File test/data/input/style_binding_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/sty...
test/data/input/style_binding_test.html:36: expect(error, new
isInstanceOf<DataBindingError>());
let's keep this with 'predicate' instead of 'isInstanceOf' (isInstanceOf is not
supported in dart2js):

expect(error, matches(predicate((e) => e is DataBindingError));

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
File test/data/input/text_multiple_bindings_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
test/data/input/text_multiple_bindings_test.html:23: @observable var j1 =
'jumps';
I'm confused, why do we need to make them observable?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
File test/data/input/text_tostring_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
test/data/input/text_tostring_test.html:23: List list = new
ObservableList.from(['initial', 'list state']);
it might be worth providing a helper function that does this, but that has a
short nice name. For example, 'makeObservable', 'toObservable', or something
else?

List list = makeObservable(['a', 'b', 'c'])
Map map = makeObservable({ 'a' : 'c'});

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.dart
File test/observe_test.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:37: expect(called, 0, reason: 'changes delived async');
mmm. called is never updated?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:116: int count = 0;
unused variable

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:204: a.value = 777;
it's redundant, but maybe add expect(a.observers, null) here to see it
explicitly? (same thing when b is inactive?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:221: test('change limit set to null', () {
'... to null (unbounded)'

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:247: } finally {
alternatively, use setUp/tearTown?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:372: reason: 'item truncated and added back');
maybe add one where it's truncated but not added back?

If I add it back with the same number, will I get a notification?

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:557: });
consider a couple test adding a new key with a 'null' value

[Jennifer Messerly, 2013-02-13 20:21:20]

Thanks for review! Some great finds in there, especially for the tests. I'm
working through them now.

Here are replies to the ones needing more discussion. For the ones not replied
to here, I agree and am working on it :)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe.dart#n...
lib/observe.dart:272: void _diagnoseCircularLimit(Map<int, _ExpressionObserver>
pendingObservers) {
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> On 2013/02/13 05:43:15, John Messerly wrote:
> > On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > > would it also be possible to detect circularity during notifyRead
(possible
> > > future TODO)?
> > > 
> > > I think we briefly chatted about this last week, but basically I'm
thinking
> > > that, if the user turns on a flag, we can detect if there are any writes
> while
> > > doing the read-barriers. If we detect that, we could throw an exception,
> which
> > > will point the user to the violating write.
> > 
> > Interesting idea. Essentially, a runtime check for purity in observed
> > expressions, like this?
> > 
> > notifyWrite(...) {
> >   if (observeReads) throw PurityError('your code is impure and must be
purged.
> > Deleting it in 5..4..3..2..1..Gone!');
> >   ...
> > }
> > 
> > I think it's the write barrier, though, not the read barrier where we have a
> > problem.
> 
> Yeah - basically I think of it as a combination of the two: a write barrier
> while our read barriers are on (when we are observing reads). I'd then suggest
> that it's ok to have impure expressions, as long as the writes are not
affecting
> the same expressions that you are reading. (You might want some side-effects
for
> logging and other purposes).
> 

Yeah, I think the challenge is the cycles don't necessarily happen in one
iteration. It can be A -> B -> C -> D. So you'd have to run it some number of
iterations before you knew what happened.

FWIW, with detailed change records, I think you wouldn't need to use read
barriers, your observable expressions track the list of Observable objects and
which fields/indexes they depend on. We might be able to use that info for more
debugging.

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.da...
lib/observe/map.dart:111: _notifyWriteLength(len);
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> On 2013/02/13 05:43:15, John Messerly wrote:
> > On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > > should this be separate? I mean,
> > > 
> > > if (oldValue != value) _notifyWriteKey(key);
> > > if (len != map.length) _notifyWriteLengh(len);
> > 
> > Not quite, it was a bit tricky to get right. Added comment:
> > 
> >     // Note: if length changed, it means the key was added, so we need to
> >     // _notifyWriteKey. Also _notifyWriteLength will check if length
changed.
> 
> I see, so basically you are handling when the value is 'null'.
> 
> I was also thinking that we don't want to notify write on 'length' when you
are
> just replacing the value, though. Maybe it should be more like this?
>    if (oldValue != value || len != map.length) _notifyWriteKey(key);
>    if (len != map.length) _notifyWriteLengh(len);

Uh, as I said in comment, _notifyWriteLength will check length. We do not need
another check. I can walk you through the code in person if it helps, it's
possible I'm just really confused :)

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
File lib/src/dart_parser.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/src/dart_parse...
lib/src/dart_parser.dart:69: String codeAfterDirectives() =>
code.substring(directivesEnd);
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> On 2013/02/13 05:43:15, John Messerly wrote:
> > On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > > nit: make it a property?
> > 
> > It could be repeated O(N)'s if used that way. Trying to encourage people to
> > cache it in a local.
> Shouldn't good inlining take care of that in this particular case?

No. I think you mean inlining plus common subexpression elimination? The problem
is for that to work, the compiler would need to know that "this.code" isn't
modified and that "substring" is a pure operation. It would also need to inline
directivesEnd and deduce that .directives won't change and .map and .max are
pure expressions. I really doubt it can do that.

> If we prefer to keep it as a method, then we need to rename it to something
more
like verb (I think we also decided this on that discussion, but I can't find it
in the style guide).

Sure I can rename it getCodeAfterDirectives

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
File lib/src/observable_transform.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:133: // AFTER:  Foo(bar, baz) : __$bar = bar,
__$baz = baz { ... }
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> why can't AFTER be:
> 
> Foo(this.__$bar, this.__$baz)?
> 
> Or is this mainly to handle named arguments?
> Foo({this.bar});  ==>  Foo({bar}): __$bar = bar;

It's mainly to handle named arguments. However parameter names can also show up
in documentation.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:175: var type = 'dynamic';
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> do you prefer 'dynamic' over 'var'?

I had to change it because "var get fieldName" doesn't work. I can add a comment

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:178: }
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> totally optional nit, consider using ternary expressions here and below?
> 
> var type = fields.type != null ? getOriginalCode(code, fields.type) : 'var';

Here's my thinking: I like ternary expressions. But sometimes if code is going
to be close to 80 chars, I don't want to juggle the code around just because I
renamed something.

If I had to guess, this code was longer, then I refactored the names and
introduced "getOriginalCode" to make it more clear. However once I do that I
usually don't rejigger it back into ternary form. I don't consider minimizing
number of lines of code to be a goal in and of itself. If that makes any sense
:)

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/com...
File test/data/input/component_field_2way_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/com...
test/data/input/component_field_2way_test.html:25: // a different property.
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> strange, because the purpose of 'title' in this test was exactly to bind DOM
> attributes... maybe we should create an issue to keep track of this and
> investigate how far can we get with mutation observers?

We do have another test verifying that you can bind "title".
This one just tests that it's a two way binding.

It was kind of an accident this worked before :)

I added a note to https://github.com/dart-lang/web-ui/issues/90.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
File test/data/input/text_tostring_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
test/data/input/text_tostring_test.html:23: List list = new
ObservableList.from(['initial', 'list state']);
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> it might be worth providing a helper function that does this, but that has a
> short nice name. For example, 'makeObservable', 'toObservable', or something
> else?
> 
> List list = makeObservable(['a', 'b', 'c'])
> Map map = makeObservable({ 'a' : 'c'});


Yeah, it's bugging me too. But I dunno if that is short enough to be worth it?
Some more ideas:

23 chars: new ObservableList.from
14 chars: makeObservable
19 chars: ObservableList.from // static method
16 chars: ObserveList.from
10 chars: observable // top level function. @Observable() would become a
constructor
12 chars: toObservable

Maybe we can make a play on the "observe" metaphor, find another word to
describe "object that can be observed"?

Maybe we can chat in person about this.

[Siggi Cherem (dart-lang), 2013-02-13 20:41:13]

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/12001/lib/observe/map.da...
lib/observe/map.dart:111: _notifyWriteLength(len);
On 2013/02/13 20:21:20, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > On 2013/02/13 05:43:15, John Messerly wrote:
> > > On 2013/02/13 01:43:24, Siggi Cherem (dart-lang) wrote:
> > > > should this be separate? I mean,
> > > > 
> > > > if (oldValue != value) _notifyWriteKey(key);
> > > > if (len != map.length) _notifyWriteLengh(len);
> > > 
> > > Not quite, it was a bit tricky to get right. Added comment:
> > > 
> > >     // Note: if length changed, it means the key was added, so we need to
> > >     // _notifyWriteKey. Also _notifyWriteLength will check if length
> changed.
> > 
> > I see, so basically you are handling when the value is 'null'.
> > 
> > I was also thinking that we don't want to notify write on 'length' when you
> are
> > just replacing the value, though. Maybe it should be more like this?
> >    if (oldValue != value || len != map.length) _notifyWriteKey(key);
> >    if (len != map.length) _notifyWriteLengh(len);
> 
> Uh, as I said in comment, _notifyWriteLength will check length. We do not need
> another check. I can walk you through the code in person if it helps, it's
> possible I'm just really confused :)

My bad, I don't know how but I managed to completely ignore the second sentence
of your comment and missed entirely that it was being checked in
notifyWriteLength.... sorry for all the confusion.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
File lib/src/observable_transform.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:175: var type = 'dynamic';
On 2013/02/13 20:21:20, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > do you prefer 'dynamic' over 'var'?
> 
> I had to change it because "var get fieldName" doesn't work. I can add a
comment

makes sense, thanks

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:178: }
On 2013/02/13 20:21:20, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > totally optional nit, consider using ternary expressions here and below?
> > 
> > var type = fields.type != null ? getOriginalCode(code, fields.type) : 'var';
> 
> Here's my thinking: I like ternary expressions. But sometimes if code is going
> to be close to 80 chars, I don't want to juggle the code around just because I
> renamed something.
> 
> If I had to guess, this code was longer, then I refactored the names and
> introduced "getOriginalCode" to make it more clear. However once I do that I
> usually don't rejigger it back into ternary form. I don't consider minimizing
> number of lines of code to be a goal in and of itself. If that makes any sense
> :)

np - I figured it had something to do with long lines, if you have to use
'dynamic', then you end up in a long line anyways.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
File test/data/input/text_tostring_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
test/data/input/text_tostring_test.html:23: List list = new
ObservableList.from(['initial', 'list state']);
On 2013/02/13 20:21:20, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > it might be worth providing a helper function that does this, but that has a
> > short nice name. For example, 'makeObservable', 'toObservable', or something
> > else?
> > 
> > List list = makeObservable(['a', 'b', 'c'])
> > Map map = makeObservable({ 'a' : 'c'});
> 
> 
> Yeah, it's bugging me too. But I dunno if that is short enough to be worth it?
> Some more ideas:
> 
> 23 chars: new ObservableList.from
> 14 chars: makeObservable
> 19 chars: ObservableList.from // static method
> 16 chars: ObserveList.from
> 10 chars: observable // top level function. @Observable() would become a
> constructor
> 12 chars: toObservable
> 
> Maybe we can make a play on the "observe" metaphor, find another word to
> describe "object that can be observed"?
> 
> Maybe we can chat in person about this.

sounds good - I think it also has value if we have a single function for all
types (Set, Map, List).

[Jennifer Messerly, 2013-02-14 00:38:09]

PTAL. Think I got everything *fingers crossed*

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe.dart#ne...
lib/observe.dart:287: if (i < 10) {
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> nit: combine with previous condition?
> 
> if (change == null || i >= 10) return;

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe.dart#ne...
lib/observe.dart:321: * If it wasn't read, we can free it immediately
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> missing ending '.', same in the next comment.

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe/map.dart
File lib/observe/map.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/observe/map.dar...
lib/observe/map.dart:124: // _notifyWriteKey. Also _notifyWriteLength will check
if length changed.
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> yeah, this one is more obvious, because if it is not absent, nothing will
> change. 

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/info.dart#n...
lib/src/info.dart:174: bool get codeAttached => userCode != null || externalFile
!= null;
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> I can see how this works, but it's a bit confusing because userCode can have
> both inline or external code.
> 
> I think it's ok as it is, but here is a possibly wasteful suggestion that
might
> make things clearer. How about we put back inlinedCode, but make it a
> DartCodeInfo. Then userCode is simply a getter:
> 
> DartCodeInfo get userCode =>
>   externalFile != null ? externalFile.userCode : inlinedCode
> 
> Thoughts?

Good idea! That will simplify things. :)

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
File lib/src/observable_transform.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:33: return false;
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> would this fit in the prev line?

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:42: } else {
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> nit: remove else...
> 
> if (identical(...)) return false;
> // Replace the code ...

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:175: var type = 'dynamic';
On 2013/02/13 20:41:13, Siggi Cherem (dart-lang) wrote:
> On 2013/02/13 20:21:20, John Messerly wrote:
> > On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > > do you prefer 'dynamic' over 'var'?
> > 
> > I had to change it because "var get fieldName" doesn't work. I can add a
> comment
> 
> makes sense, thanks

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/observable_...
lib/src/observable_transform.dart:178: }
On 2013/02/13 20:41:13, Siggi Cherem (dart-lang) wrote:
> On 2013/02/13 20:21:20, John Messerly wrote:
> > On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > > totally optional nit, consider using ternary expressions here and below?
> > > 
> > > var type = fields.type != null ? getOriginalCode(code, fields.type) :
'var';
> > 
> > Here's my thinking: I like ternary expressions. But sometimes if code is
going
> > to be close to 80 chars, I don't want to juggle the code around just because
I
> > renamed something.
> > 
> > If I had to guess, this code was longer, then I refactored the names and
> > introduced "getOriginalCode" to make it more clear. However once I do that I
> > usually don't rejigger it back into ternary form. I don't consider
minimizing
> > number of lines of code to be a goal in and of itself. If that makes any
sense
> > :)
> 
> np - I figured it had something to do with long lines, if you have to use
> 'dynamic', then you end up in a long line anyways.

gotcha :)

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/refactor.dart
File lib/src/refactor.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/refactor.da...
lib/src/refactor.dart:65: consumed += edit.length;
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> alternatively, get rid of line 61 and write:
>     consumed = edit.end;
> here?

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/src/refactor.da...
lib/src/refactor.dart:87: String guessIndent(String code, int charOffset) {
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> neat! =)

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/templating.dart
File lib/templating.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/lib/templating.dart...
lib/templating.dart:15: export 'src/utils.dart' show setImmediate;
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> unused import

It's an export, not an import :)
...but not important anymore since we have window.setImmediate, so removed.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
File test/data/input/conditional4_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
test/data/input/conditional4_test.html:29: deliverChangesSync();
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> are these needed? with setTimeout (or setImmediate) the deliver should happen
> now automatically =)

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
File test/data/input/conditional5_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/con...
test/data/input/conditional5_test.html:23: deliverChangesSync();
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> same here and in other tests...

Removed it everywhere where we check results async.
Kept it in places where we check results sync.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/css...
File test/data/input/css_classes_binding_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/css...
test/data/input/css_classes_binding_test.html:33: @observable
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> yay! I was waiting for a test like this one =)

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/css...
test/data/input/css_classes_binding_test.html:52: deliverChangesSync();
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> remove?

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/rec...
File test/data/input/recursive_component_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/rec...
test/data/input/recursive_component_test.html:24: @observable
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> as soon as we switch, would we want to make all subclasses of WebComponent
> observable by default?

It's a good question. I'm not sure, because it adds overhead. I think it only
makes sense for components that need to support two-way binding of their fields.
Most of them probably don't. One way or final should be fine.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/sty...
File test/data/input/style_binding_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/sty...
test/data/input/style_binding_test.html:36: expect(error, new
isInstanceOf<DataBindingError>());
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> let's keep this with 'predicate' instead of 'isInstanceOf' (isInstanceOf is
not
> supported in dart2js):
> 
> expect(error, matches(predicate((e) => e is DataBindingError));

I think it is fixed:
https://code.google.com/p/dart/issues/detail?id=5096

The CL was committed.

I figure we can try it on dart2js, if we still see the bug we can switch back.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
File test/data/input/text_multiple_bindings_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
test/data/input/text_multiple_bindings_test.html:23: @observable var j1 =
'jumps';
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> I'm confused, why do we need to make them observable?

We don't. I was basically trying to have it turn on useObservers = true, so when
we turn on the deprecation message our own tests won't trigger it. I'll do this
instead: @observable library main

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
File test/data/input/text_tostring_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/tex...
test/data/input/text_tostring_test.html:23: List list = new
ObservableList.from(['initial', 'list state']);
On 2013/02/13 20:41:13, Siggi Cherem (dart-lang) wrote:
> On 2013/02/13 20:21:20, John Messerly wrote:
> > On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > > it might be worth providing a helper function that does this, but that has
a
> > > short nice name. For example, 'makeObservable', 'toObservable', or
something
> > > else?
> > > 
> > > List list = makeObservable(['a', 'b', 'c'])
> > > Map map = makeObservable({ 'a' : 'c'});
> > 
> > 
> > Yeah, it's bugging me too. But I dunno if that is short enough to be worth
it?
> > Some more ideas:
> > 
> > 23 chars: new ObservableList.from
> > 14 chars: makeObservable
> > 19 chars: ObservableList.from // static method
> > 16 chars: ObserveList.from
> > 10 chars: observable // top level function. @Observable() would become a
> > constructor
> > 12 chars: toObservable
> > 
> > Maybe we can make a play on the "observe" metaphor, find another word to
> > describe "object that can be observed"?
> > 
> > Maybe we can chat in person about this.
> 
> sounds good - I think it also has value if we have a single function for all
> types (Set, Map, List).

went with toObservable, cleaned up everything!

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.dart
File test/observe_test.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:37: expect(called, 0, reason: 'changes delived async');
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> mmm. called is never updated?

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:116: int count = 0;
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> unused variable

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:204: a.value = 777;
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> it's redundant, but maybe add expect(a.observers, null) here to see it
> explicitly? (same thing when b is inactive?

Added for "a.observers".

Testing for "b" doesn't work. It isn't cleared until the field is set. And
unfortunately we can't read "b.observers._dead" property.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:221: test('change limit set to null', () {
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> '... to null (unbounded)'

Done.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:247: } finally {
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> alternatively, use setUp/tearTown?

As far as I understand setUp is only for a group:
http://api.dartlang.org/docs/releases/latest/unittest.html#setUp

Since this is just a single test I don't see the benefit.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:372: reason: 'item truncated and added back');
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> maybe add one where it's truncated but not added back?

The problem is our list[1] will throw in that case :)

> 
> If I add it back with the same number, will I get a notification?

Nope. Added test.

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:557: });
On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> consider a couple test adding a new key with a 'null' value

I don't think Maps allow null keys:

https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/...

I'll add a test for null value though.

[Siggi Cherem (dart-lang), 2013-02-14 00:59:25]

lgtm!! (modulo fixing toObservable =))

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/sty...
File test/data/input/style_binding_test.html (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/data/input/sty...
test/data/input/style_binding_test.html:36: expect(error, new
isInstanceOf<DataBindingError>());
On 2013/02/14 00:38:09, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > let's keep this with 'predicate' instead of 'isInstanceOf' (isInstanceOf is
> not
> > supported in dart2js):
> > 
> > expect(error, matches(predicate((e) => e is DataBindingError));
> 
> I think it is fixed:
> https://code.google.com/p/dart/issues/detail?id=5096
> 
> The CL was committed.
> 
> I figure we can try it on dart2js, if we still see the bug we can switch back.

good to know...

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.dart
File test/observe_test.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:247: } finally {
On 2013/02/14 00:38:09, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > alternatively, use setUp/tearTown?
> 
> As far as I understand setUp is only for a group:
> http://api.dartlang.org/docs/releases/latest/unittest.html#setUp
> 
> Since this is just a single test I don't see the benefit.

sounds good

https://chromiumcodereview.appspot.com/12225039/diff/5034/test/observe_test.d...
test/observe_test.dart:557: });
On 2013/02/14 00:38:09, John Messerly wrote:
> On 2013/02/13 19:28:54, Siggi Cherem (dart-lang) wrote:
> > consider a couple test adding a new key with a 'null' value
> 
> I don't think Maps allow null keys:
> 
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/sdk/...
> 
> I'll add a test for null value though.

yeah, the one for value is what I wanted, thanks =)

https://chromiumcodereview.appspot.com/12225039/diff/29002/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/29002/lib/observe.dart#n...
lib/observe.dart:146: createMap: () => new LinkedHashMap());
does this fit in the prev line?

https://chromiumcodereview.appspot.com/12225039/diff/29002/lib/observe.dart#n...
lib/observe.dart:148: return toObservable(value);
infinite loop? 
find/replace was too eager? =)

[Jennifer Messerly, 2013-02-14 01:48:39]

https://chromiumcodereview.appspot.com/12225039/diff/29002/lib/observe.dart
File lib/observe.dart (right):

https://chromiumcodereview.appspot.com/12225039/diff/29002/lib/observe.dart#n...
lib/observe.dart:146: createMap: () => new LinkedHashMap());
On 2013/02/14 00:59:25, Siggi Cherem (dart-lang) wrote:
> does this fit in the prev line?

Done.

https://chromiumcodereview.appspot.com/12225039/diff/29002/lib/observe.dart#n...
lib/observe.dart:148: return toObservable(value);
On 2013/02/14 00:59:25, Siggi Cherem (dart-lang) wrote:
> infinite loop? 
> find/replace was too eager? =)

haha. Good catch. That's what I get for not rerunning tests right at the end
after the replace. :)

also refactored the createMap code slightly