Object.observe: generate change records for indexed properties.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index bc4a63cc02e2c43b24e23a2aac5a1ae99cebfd45..8ba7044e300baac558bb7e87dd4282216de7487d 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -3302,8 +3302,7 @@ PropertyAttributes JSReceiver::GetLocalPropertyAttribute(String* name) {
   // Check whether the name is an array index.
   uint32_t index = 0;
   if (IsJSObject() && name->AsArrayIndex(&index)) {
-    if (JSObject::cast(this)->HasLocalElement(index)) return NONE;
-    return ABSENT;
+    return GetLocalElementAttribute(index);
   }
   // Named property.
   LookupResult lookup(GetIsolate());
@@ -3312,6 +3311,27 @@ PropertyAttributes JSReceiver::GetLocalPropertyAttribute(String* name) {
 }
 
 
+PropertyAttributes JSObject::GetLocalElementAttribute(uint32_t index) {
+  // Check access rights if needed.
+  if (IsAccessCheckNeeded()) {
+    Heap* heap = GetHeap();
+    if (!heap->isolate()->MayIndexedAccess(this, index, v8::ACCESS_HAS)) {
+      heap->isolate()->ReportFailedAccessCheck(this, v8::ACCESS_HAS);
+      return ABSENT;
+    }
+  }
+
+  if (IsJSGlobalProxy()) {
+    Object* proto = GetPrototype();
+    if (proto->IsNull()) return ABSENT;
+    ASSERT(proto->IsJSGlobalObject());
+    return JSReceiver::cast(proto)->GetLocalElementAttribute(index);
+  }
+
+  return GetElementsAccessor()->GetAttributes(this, this, index);
+}
+
+
 MaybeObject* NormalizedMapCache::Get(JSObject* obj,
                                      PropertyNormalizationMode mode) {
   Isolate* isolate = obj->GetIsolate();
@@ -4033,15 +4053,43 @@ MaybeObject* JSObject::DeleteElement(uint32_t index, DeleteMode mode) {
     return JSGlobalObject::cast(proto)->DeleteElement(index, mode);
   }
 
-  if (HasIndexedInterceptor()) {
-    // Skip interceptor if forcing deletion.
-    if (mode != FORCE_DELETION) {
-      return DeleteElementWithInterceptor(index);
+  // From this point on everything needs to be handlified.
+  HandleScope scope(isolate);
+  Handle<JSObject> self(this);
+
+  Handle<String> name;
+  Handle<Object> old_value(isolate->heap()->the_hole_value());
+  bool preexists = false;
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    name = isolate->factory()->Uint32ToString(index);
+    preexists = self->HasLocalElement(index);
+    if (preexists) {
+      // TODO(observe): only read & set old_value if it's not an accessor
+      old_value = Object::GetElement(self, index);
     }
-    mode = JSReceiver::FORCE_DELETION;
   }
 
-  return GetElementsAccessor()->Delete(this, index, mode);
+  MaybeObject* result;
+  // Skip interceptor if forcing deletion.
+  if (self->HasIndexedInterceptor()) {

[Toon Verwaest, 2012/11/07 10:06:52]

Maybe use (self->HasIndexedInterceptor() && mode != FORCE_DELETION) to reduce
the number of paths. There is no need to set the mode to FORCE_DELETION if it is
already FORCE_DELETION.

Otherwise put { } around if/else branches.

[rossberg, 2012/11/07 18:41:42]

Done.

+    if (mode != FORCE_DELETION)
+      result = self->DeleteElementWithInterceptor(index);
+    else
+      result = self->GetElementsAccessor()->Delete(
+          *self, index, JSReceiver::FORCE_DELETION);
+  } else {
+    result = self->GetElementsAccessor()->Delete(*self, index, mode);
+  }
+
+  Handle<Object> hresult;
+  if (!result->ToHandle(&hresult)) return result;
+
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    if (preexists && !self->HasLocalElement(index))
+      self->EnqueueChangeRecord("deleted", name, old_value);
+  }
+
+  return *hresult;
 }
 
 
@@ -10114,28 +10162,31 @@ Handle<Object> JSObject::SetElement(Handle<JSObject> object,
 
 
 MaybeObject* JSObject::SetElement(uint32_t index,
-                                  Object* value,
+                                  Object* value_raw,
                                   PropertyAttributes attributes,
                                   StrictModeFlag strict_mode,
                                   bool check_prototype,
                                   SetPropertyMode set_mode) {
+  Isolate* isolate = GetIsolate();
+  HandleScope scope(isolate);
+  Handle<JSObject> self(this);
+  Handle<Object> value(value_raw);
+
   // Check access rights if needed.
   if (IsAccessCheckNeeded()) {
     Heap* heap = GetHeap();
-    if (!heap->isolate()->MayIndexedAccess(this, index, v8::ACCESS_SET)) {
-      HandleScope scope(heap->isolate());
-      Handle<Object> value_handle(value);
-      heap->isolate()->ReportFailedAccessCheck(this, v8::ACCESS_SET);
-      return *value_handle;
+    if (!heap->isolate()->MayIndexedAccess(*self, index, v8::ACCESS_SET)) {
+      heap->isolate()->ReportFailedAccessCheck(*self, v8::ACCESS_SET);
+      return *value;
     }
   }
 
   if (IsJSGlobalProxy()) {
     Object* proto = GetPrototype();
-    if (proto->IsNull()) return value;
+    if (proto->IsNull()) return *value;
     ASSERT(proto->IsJSGlobalObject());
     return JSObject::cast(proto)->SetElement(index,
-                                             value,
+                                             *value,
                                              attributes,
                                              strict_mode,
                                              check_prototype,
@@ -10144,10 +10195,8 @@ MaybeObject* JSObject::SetElement(uint32_t index,
 
   // Don't allow element properties to be redefined for external arrays.
   if (HasExternalArrayElements() && set_mode == DEFINE_PROPERTY) {
-    Isolate* isolate = GetHeap()->isolate();
-    Handle<Object> receiver(this);
     Handle<Object> number = isolate->factory()->NewNumberFromUint(index);
-    Handle<Object> args[] = { receiver, number };
+    Handle<Object> args[] = { self, number };
     Handle<Object> error = isolate->factory()->NewTypeError(
         "redef_external_array_element", HandleVector(args, ARRAY_SIZE(args)));
     return isolate->Throw(*error);
@@ -10162,22 +10211,56 @@ MaybeObject* JSObject::SetElement(uint32_t index,
     dictionary->set_requires_slow_elements();
   }
 
+  // From here on, everything has to be handlified.
+  Handle<String> name;
+  Handle<Object> old_value(isolate->heap()->the_hole_value());
+  PropertyAttributes old_attributes;
+  bool preexists = false;
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    name = isolate->factory()->Uint32ToString(index);
+    preexists = self->HasLocalElement(index);
+    if (preexists) {
+      old_attributes = self->GetLocalPropertyAttribute(*name);
+      // TODO(observe): only read & set old_value if we have a data property
+      old_value = Object::GetElement(self, index);
+    }
+  }
+
   // Check for lookup interceptor
-  if (HasIndexedInterceptor()) {
-    return SetElementWithInterceptor(index,
-                                     value,
-                                     attributes,
-                                     strict_mode,
-                                     check_prototype,
-                                     set_mode);
+  MaybeObject* result = *value;

[Toon Verwaest, 2012/11/07 10:06:52]

This initialization seems superfluous since it's overwritten directly
afterwards.

[rossberg, 2012/11/07 18:41:42]

Done.

+  if (self->HasIndexedInterceptor()) {
+    result = self->SetElementWithInterceptor(index,
+                                             *value,
+                                             attributes,
+                                             strict_mode,
+                                             check_prototype,
+                                             set_mode);
+  } else {
+    result = self->SetElementWithoutInterceptor(index,
+                                                *value,
+                                                attributes,
+                                                strict_mode,
+                                                check_prototype,
+                                                set_mode);
   }
 
-  return SetElementWithoutInterceptor(index,
-                                      value,
-                                      attributes,
-                                      strict_mode,
-                                      check_prototype,
-                                      set_mode);
+  Handle<Object> hresult;
+  if (!result->ToHandle(&hresult)) return result;
+
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    PropertyAttributes new_attributes = self->GetLocalPropertyAttribute(*name);
+    if (!preexists) {
+      self->EnqueueChangeRecord("new", name, old_value);
+    } else if (new_attributes != old_attributes || old_value->IsTheHole()) {
+      self->EnqueueChangeRecord("reconfigured", name, old_value);
+    } else {
+      Handle<Object> newValue = Object::GetElement(self, index);
+      if (!newValue->SameValue(*old_value))
+        self->EnqueueChangeRecord("updated", name, old_value);
+    }
+  }
+
+  return *hresult;
 }