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

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index b46c2e0697175f94aa0f79495ace152846decc05..5fdc581614dcda1234f61eb09de4a8e64eb69ea5 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -3186,14 +3186,17 @@ MaybeObject* JSObject::SetLocalPropertyIgnoreAttributes(
   if (is_observed) {
     if (lookup.IsTransition()) {
       EnqueueChangeRecord(self, "new", name, old_value);
+    } else if (old_value->IsTheHole()) {

[rossberg, 2013/01/17 16:57:44]

Is this extra branch actually needed? It seems like the SameValue check below
should apply just fine, and you can keep the condition in the 'if' below.

[adamk, 2013/01/22 22:30:10]

I think I pulled this out to avoid an unnecessary GetProperty() call for the
oldValue-is-the-hole case, and to make it clear that the only thing triggering
this code path is the absence of the old value. Is your objection to the "extra"
call to EnqueueChangeRecord?

[rossberg, 2013/01/28 13:30:43]

Yeah, the fewer code paths the better, but I don't feel too strong about it in
this case. I leave it up to you.

+      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);
       }
     }
@@ -10256,10 +10259,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()) {

[rossberg, 2013/01/17 16:57:44]

Similarly here.

     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;