From 6c7b0cfc4d7625792a199879d2389e5d6a6c485e Mon Sep 17 00:00:00 2001
From: Rafael Weinstein <rafaelw@chromium.org>
Date: Fri, 30 Aug 2013 12:24:34 -0700
Subject: [PATCH] Dirty-check underlying value -- not computed value

R=arv
BUG=

Review URL: https://codereview.appspot.com/13379044
---
 src/observe.js | 26 ++++++++++++++------
 tests/test.js  | 67 +++++++++++++++++++++++++++++---------------------
 2 files changed, 57 insertions(+), 36 deletions(-)

diff --git a/src/observe.js b/src/observe.js
index 1dbc9d6..cf02435 100644
--- a/src/observe.js
+++ b/src/observe.js
@@ -207,6 +207,8 @@
       observer.report();
       cycles++;
     }
+    if (global.testingExposeCycleCount)
+      global.dirtyCheckCycleCount = cycles;
   }
 
   function objectIsEmpty(object) {
@@ -417,6 +419,9 @@
       }
     } while (cycles < MAX_DIRTY_CHECK_CYCLES && results.anyChanged);
 
+    if (global.testingExposeCycleCount)
+      global.dirtyCheckCycleCount = cycles;
+
     Observer._allObserversCount = allObservers.length;
     runningMicrotaskCheckpoint = false;
   };
@@ -589,21 +594,24 @@
     if (!path) {
       // Invalid path.
       this.closed = true;
-      this.value = valueFn ? valueFn() : undefined;
+      this.value_ = undefined;
+      this.value = valueFn ? valueFn(this.value_) : this.value_;
       return;
     }
 
     if (!path.length) {
       // 0-length path.
       this.closed = true;
-      this.value = valueFn ? valueFn(object) : object;
+      this.value_ = object;
+      this.value = valueFn ? valueFn(this.value_) : this.value_;
       return;
     }
 
     if (!isObject(object)) {
       // non-object & non-0-length path.
       this.closed = true;
-      this.value = valueFn ? valueFn() : undefined;
+      this.value_ = undefined;
+      this.value = valueFn ? valueFn(this.value_) : this.value_;
       return;
     }
 
@@ -626,6 +634,7 @@
 
     disconnect: function() {
       this.value = undefined;
+      this.value_ = undefined;
       if (this.observedSet) {
         this.observedSet.reset();
         this.observedSet.cleanup();
@@ -639,15 +648,15 @@
       if (this.observedSet)
         this.observedSet.reset();
 
-      var newValue = this.path.getValueFrom(this.object, this.observedSet)
-      this.value = this.valueFn ? this.valueFn(newValue) : newValue;
+      this.value_ = this.path.getValueFrom(this.object, this.observedSet);
 
       if (this.observedSet)
         this.observedSet.cleanup();
 
-      if (areSameValue(this.value, this.oldValue))
+      if (areSameValue(this.value_, this.oldValue_))
         return false;
 
+      this.value = this.valueFn ? this.valueFn(this.value_) : this.value_;
       this.reportArgs = [this.value, this.oldValue];
       return true;
     },
@@ -657,13 +666,14 @@
         if (this.observedSet)
           this.observedSet.reset();
 
-        var newValue = this.path.getValueFrom(this.object, this.observedSet)
-        this.value = this.valueFn ? this.valueFn(newValue) : newValue;
+        this.value_ = this.path.getValueFrom(this.object, this.observedSet);
+        this.value = this.valueFn ? this.valueFn(this.value_) : this.value_;
 
         if (this.observedSet)
           this.observedSet.cleanup();
       }
 
+      this.oldValue_ = this.value_;
       this.oldValue = this.value;
     },
 
diff --git a/tests/test.js b/tests/test.js
index e27102d..3714e19 100644
--- a/tests/test.js
+++ b/tests/test.js
@@ -16,6 +16,8 @@ var observer;
 var callbackArgs = undefined;
 var callbackInvoked = false;
 
+window.testingExposeCycleCount = true;
+
 function callback() {
   callbackArgs = Array.prototype.slice.apply(arguments);
   callbackInvoked = true;
@@ -34,6 +36,23 @@ function assertNoChanges() {
   assert.isUndefined(callbackArgs);
 }
 
+function assertPathChanges(expectNewValue, expectOldValue) {
+  observer.deliver();
+
+  assert.isTrue(callbackInvoked);
+
+  var newValue = callbackArgs[0];
+  var oldValue = callbackArgs[1];
+  assert.deepEqual(expectNewValue, newValue);
+  assert.deepEqual(expectOldValue, oldValue);
+
+  assert.isTrue(window.dirtyCheckCycleCount === undefined ||
+                window.dirtyCheckCycleCount === 1);
+
+  callbackArgs = undefined;
+  callbackInvoked = false;
+}
+
 var createObject = ('__proto__' in {}) ?
   function(obj) { return obj; } :
   function(obj) {
@@ -117,20 +136,6 @@ suite('PathObserver Tests', function() {
 
   teardown(doTeardown);
 
-  function assertPathChanges(expectNewValue, expectOldValue) {
-    observer.deliver();
-
-    assert.isTrue(callbackInvoked);
-
-    var newValue = callbackArgs[0];
-    var oldValue = callbackArgs[1];
-    assert.deepEqual(expectNewValue, newValue);
-    assert.deepEqual(expectOldValue, oldValue);
-
-    callbackArgs = undefined;
-    callbackInvoked = false;
-  }
-
   test('Close Invokes Close', function() {
     var called = false;
     var obj = { foo: 1, close: function() { called = true }};
@@ -385,6 +390,26 @@ suite('PathObserver Tests', function() {
     observer.close();
   });
 
+  test('valueFn - return object literal', function() {
+    var model = { };
+
+    function valueFn(value) {
+      return isNaN(value) ? value : [ value ];
+    }
+
+    observer = new PathObserver(model, 'foo', callback, undefined, undefined,
+                                valueFn);
+
+    model.foo = 1;
+    assertPathChanges([1], undefined);
+
+    model.foo = 3;
+    assertPathChanges([3], [1]);
+
+    observer.close();
+  });
+
+
   test('Path With Indices', function() {
     var model = [];
 
@@ -714,20 +739,6 @@ suite('CompoundPathObserver Tests', function() {
 
   teardown(doTeardown);
 
-  function assertPathChanges(expectNewValue, expectOldValue) {
-    observer.deliver();
-
-    assert.isTrue(callbackInvoked);
-
-    var newValue = callbackArgs[0];
-    var oldValue = callbackArgs[1];
-    assert.deepEqual(expectNewValue, newValue);
-    assert.deepEqual(expectOldValue, oldValue);
-
-    callbackArgs = undefined;
-    callbackInvoked = false;
-  }
-
   test('CompoundPath Simple', function() {
     var model = { a: 1, b: 2, c: 3 };