Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Commit

Permalink
Avoid accessing observable value twice during setup of CompoundObserver
Browse files Browse the repository at this point in the history
This was affecting polymer startup. The basic problem is that Observer.open must read the underlying value, and was called during addObserver, only shortly after to have check_ call discardChanges() -- which read the underlying value again.

The solution is to delay opening the Observable until the CompoundObserver is opened

R=arv
BUG=

Review URL: https://codereview.appspot.com/85570043
  • Loading branch information
rafaelw committed Apr 8, 2014
1 parent c007279 commit 812e8a4
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 11 deletions.
19 changes: 12 additions & 7 deletions src/observe.js
Expand Up @@ -535,8 +535,8 @@
addToAll(this);
this.callback_ = callback;
this.target_ = target;
this.state_ = OPENED;
this.connect_();
this.state_ = OPENED;
return this.value_;
},

Expand All @@ -545,11 +545,11 @@
return;

removeFromAll(this);
this.state_ = CLOSED;
this.disconnect_();
this.value_ = undefined;
this.callback_ = undefined;
this.target_ = undefined;
this.state_ = CLOSED;
},

deliver: function() {
Expand Down Expand Up @@ -906,7 +906,6 @@
if (this.state_ != UNOPENED && this.state_ != RESETTING)
throw Error('Cannot add observers once started.');

observer.open(this.deliver, this);
this.observed_.push(observerSentinel, observer);
},

Expand Down Expand Up @@ -939,11 +938,17 @@
check_: function(changeRecords, skipChanges) {
var oldValues;
for (var i = 0; i < this.observed_.length; i += 2) {
var pathOrObserver = this.observed_[i+1];
var object = this.observed_[i];
var value = object === observerSentinel ?
pathOrObserver.discardChanges() :
pathOrObserver.getValueFrom(object)
var path = this.observed_[i+1];
var value;
if (object === observerSentinel) {
var observable = path;
value = this.state_ === UNOPENED ?
observable.open(this.deliver, this) :
observable.discardChanges();
} else {
value = path.getValueFrom(object);
}

if (skipChanges) {
this.value_[i / 2] = value;
Expand Down
4 changes: 0 additions & 4 deletions tests/test.js
Expand Up @@ -1174,10 +1174,6 @@ suite('CompoundObserver Tests', function() {
function setValueFn(value) { return value / 2; }

var compound = new CompoundObserver;
assert.throws(function () {
compound.addObserver(1);
});

compound.addPath(model, 'a');
compound.addObserver(new ObserverTransform(new PathObserver(model, 'b'),
valueFn, setValueFn));
Expand Down

0 comments on commit 812e8a4

Please sign in to comment.