Roll polymer packages to version 0.3.4

Roll polymer packages to version 0.3.4

R=jakemac@google.com, jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2014-07-25 02:11:00]

This is pretty much ready for review. I got all but 1 test timing out locally
(publish_inherited_properties_test) which I plan to take a look tomorrow.

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/pubspec.yaml
File pkg/polymer/pubspec.yaml (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/pubspec.yaml...
pkg/polymer/pubspec.yaml:2: version: 0.12.0-pre.1.dev
FYI - 0.12.1-dev has not been published. I made that change earlier today and
didn't realize that I should use a different number here. Honestly this should
just be 0.12.0-dev, but we already published that (according to Natalie we
should not publish -dev while our code lives in the Dart repo to make the
protocol easier on others), so I'm using this other version in the interim.

By the time we are done, we will be able to publish it as 0.12.0.

[jakemac, 2014-07-25 16:40:34]

Some of these comments you can probably ignore (especially the ones in
observable) since its inherited logic from the js side.

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:236: observe(obj, _segments[0]);
Is segments[0] what we want here? It might be worth throwing in a comment or
something since its not immediately clear to me what this function is intended
to be doing (or more specifically the new 2nd argument).

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:604: _observed[i + 1].close();
at first this looks like it could be out of bounds, even though _observed has an
even # of items its probably best to just ensure this, s/i < _observed.length/i
< _observed.length - 1

Or maybe this should be a list of classes that wrap up these two items that are
always together so its a bit less confusing? Or even just a generic pair/tuple
class?

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:889: bool
_allRootObjectNonObservedProperties(List<ChangeRecord> records) {
This function doesn't seem to be named very well, but if its copied over from
the JS side I guess its probably best to stay consistent. From the name I would
be expecting a list of the non-observed properties as a return value.

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/test/path_ob...
File pkg/observe/test/path_observer_test.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:489: expectedOldValues,
expectedObserved, [dontDeliver = false]) {
[deliver = true] would be easier to comprehend here imo

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:51: ///       @published double volume3;
we are going to make sure we update our examples/etc to match

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:116: ///       // Because `x` only appears
inside method `m`, we will not notice
Is there any way currently to work around this? Does polymer.js provide
anything?

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:816: // TODO(sigmund): depreacte this.
s/depreacte/depricate

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1285: // supported in Dart using a new style
of @publised property declaration using
@publised/@published

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/prop...
File pkg/polymer/lib/src/property_accessor.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/prop...
pkg/polymer/lib/src/property_accessor.dart:41:
_target.emitPropertyChangeRecord(_name, oldValue, newValue);
I noticed that newValue and oldValue are swapped here compared to the JS?

[Siggi Cherem (dart-lang), 2014-07-25 17:45:13]

Thanks Jake!

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:236: observe(obj, _segments[0]);
On 2014/07/25 16:40:33, jakemac wrote:
> Is segments[0] what we want here? It might be worth throwing in a comment or
> something since its not immediately clear to me what this function is intended
> to be doing (or more specifically the new 2nd argument).

yeah - basically the second argument is to optimize _ObserveSet - instead of
listening for every property on [obj], they want to make sure they listen on the
subset of properties that are used.

I added a comment here and in _ObserveSet as well.

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:604: _observed[i + 1].close();
On 2014/07/25 16:40:33, jakemac wrote:
> at first this looks like it could be out of bounds, even though _observed has
an
> even # of items its probably best to just ensure this, s/i <
_observed.length/i
> < _observed.length - 1
> 
> Or maybe this should be a list of classes that wrap up these two items that
are
> always together so its a bit less confusing? Or even just a generic pair/tuple
> class?

Yeah - I also dislike using this representation with pair of elements in the
array. I kept it as is just to continue to have the code as close to the
original.

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/lib/src/path...
pkg/observe/lib/src/path_observer.dart:889: bool
_allRootObjectNonObservedProperties(List<ChangeRecord> records) {
On 2014/07/25 16:40:34, jakemac wrote:
> This function doesn't seem to be named very well, but if its copied over from
> the JS side I guess its probably best to stay consistent. From the name I
would
> be expecting a list of the non-observed properties as a return value.

Done. Yeah - this seems local enough that renaming it should be OK. I did the
rename and added a Dart note for it. That way we can still search for the old
name and find it.

If this starts getting used in other places (not just privately in this class),
then we should probably rename it back.

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/test/path_ob...
File pkg/observe/test/path_observer_test.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/observe/test/path_ob...
pkg/observe/test/path_observer_test.dart:489: expectedOldValues,
expectedObserved, [dontDeliver = false]) {
On 2014/07/25 16:40:33, jakemac wrote:
> [deliver = true] would be easier to comprehend here imo

agree :) - I renamed it and made it a named argument, that way we don't get
confused in the calls below.

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
File pkg/polymer/lib/src/instance.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:51: ///       @published double volume3;
On 2014/07/25 16:40:34, jakemac wrote:
> we are going to make sure we update our examples/etc to match 

yeah, we need to coordinate with Kathy too.

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:116: ///       // Because `x` only appears
inside method `m`, we will not notice
On 2014/07/25 16:40:34, jakemac wrote:
> Is there any way currently to work around this? Does polymer.js provide
> anything?

Not really. JS will have the same issue. We used to have a mechanism to support
this in the past, but we moved away from it. We used to add read-barriers on
each getter so we could record what was read when evaluating an expression, then
we would know what to observe.

Now that we have the readValue/writeValue that could be possible for
CustomElement properties, but it will still not work for normal observable
objects unless we require the compiler in the loop always.

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:816: // TODO(sigmund): depreacte this.
On 2014/07/25 16:40:34, jakemac wrote:
> s/depreacte/depricate

Done. but s/depricate/deprecate  :-)

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/inst...
pkg/polymer/lib/src/instance.dart:1285: // supported in Dart using a new style
of @publised property declaration using
On 2014/07/25 16:40:34, jakemac wrote:
> @publised/@published

Done. Thx!

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/prop...
File pkg/polymer/lib/src/property_accessor.dart (right):

https://codereview.chromium.org/420673002/diff/20001/pkg/polymer/lib/src/prop...
pkg/polymer/lib/src/property_accessor.dart:41:
_target.emitPropertyChangeRecord(_name, oldValue, newValue);
On 2014/07/25 16:40:33, jakemac wrote:
> I noticed that newValue and oldValue are swapped here compared to the JS?

good catch! I swapped them subconsciously because notifyPropertyChange is the
other way around... I swapped them back.

[jakemac, 2014-07-25 18:08:22]

lgtm

[Jennifer Messerly, 2014-07-28 19:21:14]

LGTM+++  bringing the way properties work in sync is great.

[Siggi Cherem (dart-lang), 2014-07-28 19:57:31]

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