Object.observe: don't unnecessarily emit oldValue for reconfigurations of data properties

When a data property has its attributes changed but its value remains the same,
don't emit an oldValue. This makes the API more consistent by only emitting
oldValue when the value of a property has actually changed (or been removed,
in the case of a reconfiguration as an accessor property or a deletion).

Review URL: https://codereview.chromium.org/11820004

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@13565 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/objects.cc

@@ -3177,14 +3177,17 @@ MaybeObject* JSObject::SetLocalPropertyIgnoreAttributes(
   if (is_observed) {
     if (lookup.IsTransition()) {
       EnqueueChangeRecord(self, "new", name, old_value);
+    } else if (old_value->IsTheHole()) {
+      EnqueueChangeRecord(self, "reconfigured", name, old_value);
     } else {
       LookupResult new_lookup(isolate);
       self->LocalLookup(*name, &new_lookup, true);
-      if (old_value->IsTheHole() ||
-          new_lookup.GetAttributes() != old_attributes) {
+      bool value_changed = new_lookup.IsDataProperty() &&
+          !old_value->SameValue(*Object::GetProperty(self, name));
+      if (new_lookup.GetAttributes() != old_attributes) {
+        if (!value_changed) old_value = isolate->factory()->the_hole_value();
         EnqueueChangeRecord(self, "reconfigured", name, old_value);
-      } else if (new_lookup.IsDataProperty() &&
-          !Object::GetProperty(self, name)->SameValue(*old_value)) {
+      } else if (value_changed) {
         EnqueueChangeRecord(self, "updated", name, old_value);
       }
     }
@@ -10337,10 +10340,17 @@ MaybeObject* JSObject::SetElement(uint32_t index,
       EnqueueChangeRecord(
           self, "updated", isolate->factory()->length_symbol(), old_length);
     }
-  } else if (old_attributes != new_attributes || old_value->IsTheHole()) {
+  } else if (old_value->IsTheHole()) {
     EnqueueChangeRecord(self, "reconfigured", name, old_value);
-  } else if (!old_value->SameValue(*Object::GetElement(self, index))) {
-    EnqueueChangeRecord(self, "updated", name, old_value);
+  } else {
+    bool value_changed =
+        !old_value->SameValue(*Object::GetElement(self, index));
+    if (old_attributes != new_attributes) {
+      if (!value_changed) old_value = isolate->factory()->the_hole_value();
+      EnqueueChangeRecord(self, "reconfigured", name, old_value);
+    } else if (value_changed) {
+      EnqueueChangeRecord(self, "updated", name, old_value);
+    }
   }
 
   return *hresult;

test/mjsunit/harmony/object-observe.js

@@ -371,7 +371,7 @@ observer.assertCallbackRecords([
   { object: obj, name: "a", type: "new" },
   { object: obj, name: "a", type: "updated", oldValue: 4 },
   { object: obj, name: "a", type: "updated", oldValue: 5 },
-  { object: obj, name: "a", type: "reconfigured", oldValue: 6 },
+  { object: obj, name: "a", type: "reconfigured" },
   { object: obj, name: "a", type: "updated", oldValue: 6 },
   { object: obj, name: "a", type: "reconfigured", oldValue: 8 },
   { object: obj, name: "a", type: "reconfigured", oldValue: 7 },
@@ -429,7 +429,7 @@ observer.assertCallbackRecords([
   { object: obj, name: "1", type: "new" },
   { object: obj, name: "1", type: "updated", oldValue: 4 },
   { object: obj, name: "1", type: "updated", oldValue: 5 },
-  { object: obj, name: "1", type: "reconfigured", oldValue: 6 },
+  { object: obj, name: "1", type: "reconfigured" },
   { object: obj, name: "1", type: "updated", oldValue: 6 },
   { object: obj, name: "1", type: "reconfigured", oldValue: 8 },
   { object: obj, name: "1", type: "reconfigured", oldValue: 7 },
@@ -493,7 +493,7 @@ function TestObserveConfigurable(obj, prop) {
     { object: obj, name: prop, type: "new" },
     { object: obj, name: prop, type: "updated", oldValue: 4 },
     { object: obj, name: prop, type: "updated", oldValue: 5 },
-    { object: obj, name: prop, type: "reconfigured", oldValue: 6 },
+    { object: obj, name: prop, type: "reconfigured" },
     { object: obj, name: prop, type: "updated", oldValue: 6 },
     { object: obj, name: prop, type: "reconfigured", oldValue: 8 },
     { object: obj, name: prop, type: "reconfigured", oldValue: 7 },
@@ -537,7 +537,7 @@ function TestObserveNonConfigurable(obj, prop, desc) {
     { object: obj, name: prop, type: "updated", oldValue: 4 },
     { object: obj, name: prop, type: "updated", oldValue: 5 },
     { object: obj, name: prop, type: "updated", oldValue: 6 },
-    { object: obj, name: prop, type: "reconfigured", oldValue: 7 },
+    { object: obj, name: prop, type: "reconfigured" },
   ]);
   Object.unobserve(obj, observer.callback);
 }
@@ -657,7 +657,7 @@ observer.assertCallbackRecords([
   { object: arr, name: '1', type: 'deleted', oldValue: 'b' },
   { object: arr, name: 'length', type: 'updated', oldValue: 2 },
   { object: arr, name: 'length', type: 'updated', oldValue: 1 },
-  { object: arr, name: 'length', type: 'reconfigured', oldValue: 10 },
+  { object: arr, name: 'length', type: 'reconfigured' },
   { object: arr2, name: '1', type: 'deleted', oldValue: 'beta' },
   { object: arr2, name: 'length', type: 'updated', oldValue: 2 },
   { object: arr2, name: 'length', type: 'reconfigured', oldValue: 1 },