Revision: 12888
Author: [email protected]
Date: Wed Nov 7 06:14:50 2012
Log: Object.observe: include oldValue in change records,
plus more accurate distinction of different change types.
Required handlifying more code.
Also fixed a handlification bug in JSProxy::GetElementAttributeWithHandler.
[email protected]
BUG=
Review URL: https://codereview.chromium.org/11362115
http://code.google.com/p/v8/source/detail?r=12888
Modified:
/branches/bleeding_edge/src/objects-inl.h
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/src/property.h
/branches/bleeding_edge/test/mjsunit/harmony/object-observe.js
=======================================
--- /branches/bleeding_edge/src/objects-inl.h Tue Nov 6 08:47:15 2012
+++ /branches/bleeding_edge/src/objects-inl.h Wed Nov 7 06:14:50 2012
@@ -5087,6 +5087,7 @@
PropertyAttributes JSReceiver::GetPropertyAttribute(String* key) {
return GetPropertyAttributeWithReceiver(this, key);
}
+
// TODO(504): this may be useful in other places too where JSGlobalProxy
// is used.
=======================================
--- /branches/bleeding_edge/src/objects.cc Tue Nov 6 04:32:36 2012
+++ /branches/bleeding_edge/src/objects.cc Wed Nov 7 06:14:50 2012
@@ -2761,12 +2761,14 @@
MUST_USE_RESULT PropertyAttributes JSProxy::GetElementAttributeWithHandler(
- JSReceiver* receiver,
+ JSReceiver* receiver_raw,
uint32_t index) {
Isolate* isolate = GetIsolate();
HandleScope scope(isolate);
+ Handle<JSProxy> proxy(this);
+ Handle<JSReceiver> receiver(receiver_raw);
Handle<String> name = isolate->factory()->Uint32ToString(index);
- return GetPropertyAttributeWithHandler(receiver, *name);
+ return proxy->GetPropertyAttributeWithHandler(*receiver, *name);
}
@@ -2901,7 +2903,7 @@
Handle<Object> old_value(heap->the_hole_value());
if (FLAG_harmony_observation && map()->is_observed()) {
- // TODO(observe): save oldValue
+ old_value = handle(lookup->GetLazyValue());
}
// This is a real property that is not read-only, or it is a
@@ -2953,7 +2955,7 @@
result = self->ConvertDescriptorToField(*name, *value,
attributes);
}
} else if (details.type() == CALLBACKS) {
- result = ConvertDescriptorToField(*name, *value, attributes);
+ result = self->ConvertDescriptorToField(*name, *value, attributes);
} else {
ASSERT(details.type() == CONSTANT_FUNCTION);
@@ -2966,7 +2968,7 @@
} else {
// Otherwise, replace with a map transition to a new map with a
FIELD,
// even if the value is a constant function.
- result = ConvertTransitionToMapTransition(
+ result = self->ConvertTransitionToMapTransition(
lookup->GetTransitionIndex(), *name, *value, attributes);
}
}
@@ -2981,7 +2983,16 @@
if (!result->ToHandle(&hresult)) return result;
if (FLAG_harmony_observation && map()->is_observed()) {
- this->EnqueueChangeRecord("updated", name, old_value);
+ if (lookup->IsTransition()) {
+ self->EnqueueChangeRecord("new", name, old_value);
+ } else {
+ LookupResult new_lookup(self->GetIsolate());
+ self->LocalLookup(*name, &new_lookup);
+ ASSERT(!new_lookup.GetLazyValue()->IsTheHole());
+ if (!new_lookup.GetLazyValue()->SameValue(*old_value)) {
+ self->EnqueueChangeRecord("updated", name, old_value);
+ }
+ }
}
return *hresult;
@@ -3010,22 +3021,22 @@
MaybeObject* JSObject::SetLocalPropertyIgnoreAttributes(
- String* name,
- Object* value,
+ String* name_raw,
+ Object* value_raw,
PropertyAttributes attributes) {
// Make sure that the top context does not change when doing callbacks or
// interceptor calls.
AssertNoContextChange ncc;
Isolate* isolate = GetIsolate();
LookupResult lookup(isolate);
- LocalLookup(name, &lookup);
- if (!lookup.IsFound()) map()->LookupTransition(this, name, &lookup);
+ LocalLookup(name_raw, &lookup);
+ if (!lookup.IsFound()) map()->LookupTransition(this, name_raw, &lookup);
// Check access rights if needed.
if (IsAccessCheckNeeded()) {
- if (!isolate->MayNamedAccess(this, name, v8::ACCESS_SET)) {
+ if (!isolate->MayNamedAccess(this, name_raw, v8::ACCESS_SET)) {
return SetPropertyWithFailedAccessCheck(&lookup,
- name,
- value,
+ name_raw,
+ value_raw,
false,
kNonStrictMode);
}
@@ -3033,47 +3044,56 @@
if (IsJSGlobalProxy()) {
Object* proto = GetPrototype();
- if (proto->IsNull()) return value;
+ if (proto->IsNull()) return value_raw;
ASSERT(proto->IsJSGlobalObject());
return JSObject::cast(proto)->SetLocalPropertyIgnoreAttributes(
- name,
- value,
+ name_raw,
+ value_raw,
attributes);
}
// Check for accessor in prototype chain removed here in clone.
if (!lookup.IsFound()) {
// Neither properties nor transitions found.
- return AddProperty(name, value, attributes, kNonStrictMode);
+ return AddProperty(name_raw, value_raw, attributes, kNonStrictMode);
}
+
+ // From this point on everything needs to be handlified.
+ HandleScope scope(GetIsolate());
+ Handle<JSObject> self(this);
+ Handle<String> name(name_raw);
+ Handle<Object> value(value_raw);
Handle<Object> old_value(isolate->heap()->the_hole_value());
+ PropertyAttributes old_attributes = ABSENT;
if (FLAG_harmony_observation && map()->is_observed()) {
- // TODO(observe): save oldValue
+ old_value = handle(lookup.GetLazyValue());
+ old_attributes = lookup.GetAttributes();
}
// Check of IsReadOnly removed from here in clone.
- MaybeObject* result = value;
+ MaybeObject* result = *value;
switch (lookup.type()) {
case NORMAL: {
PropertyDetails details = PropertyDetails(attributes, NORMAL);
- result = SetNormalizedProperty(name, value, details);
+ result = self->SetNormalizedProperty(*name, *value, details);
break;
}
case FIELD:
- result = FastPropertyAtPut(lookup.GetFieldIndex(), value);
+ result = self->FastPropertyAtPut(lookup.GetFieldIndex(), *value);
break;
case CONSTANT_FUNCTION:
// Only replace the function if necessary.
- if (value == lookup.GetConstantFunction()) return value;
- // Preserve the attributes of this existing property.
- attributes = lookup.GetAttributes();
- result = ConvertDescriptorToField(name, value, attributes);
+ if (*value != lookup.GetConstantFunction()) {
+ // Preserve the attributes of this existing property.
+ attributes = lookup.GetAttributes();
+ result = self->ConvertDescriptorToField(*name, *value, attributes);
+ }
break;
case CALLBACKS:
case INTERCEPTOR:
// Override callback in clone
- result = ConvertDescriptorToField(name, value, attributes);
+ result = self->ConvertDescriptorToField(*name, *value, attributes);
break;
case TRANSITION: {
Map* transition_map = lookup.GetTransitionTarget();
@@ -3085,22 +3105,20 @@
if (details.type() == FIELD) {
if (attributes == details.attributes()) {
int field_index = descriptors->GetFieldIndex(descriptor);
- result = AddFastPropertyUsingMap(transition_map,
- name,
- value,
- field_index);
+ result = self->AddFastPropertyUsingMap(
+ transition_map, *name, *value, field_index);
} else {
- result = ConvertDescriptorToField(name, value, attributes);
+ result = self->ConvertDescriptorToField(*name, *value,
attributes);
}
} else if (details.type() == CALLBACKS) {
- result = ConvertDescriptorToField(name, value, attributes);
+ result = self->ConvertDescriptorToField(*name, *value, attributes);
} else {
ASSERT(details.type() == CONSTANT_FUNCTION);
// Replace transition to CONSTANT FUNCTION with a map transition
to a
// new map with a FIELD, even if the value is a function.
- result = ConvertTransitionToMapTransition(
- lookup.GetTransitionIndex(), name, value, attributes);
+ result = self->ConvertTransitionToMapTransition(
+ lookup.GetTransitionIndex(), *name, *value, attributes);
}
break;
}
@@ -3113,9 +3131,19 @@
if (!result->ToHandle(&hresult)) return result;
if (FLAG_harmony_observation && map()->is_observed()) {
- const char* type =
- attributes == lookup.GetAttributes() ? "updated" : "reconfigured";
- this->EnqueueChangeRecord(type, handle(name), old_value);
+ if (lookup.IsTransition()) {
+ self->EnqueueChangeRecord("new", name, old_value);
+ } else {
+ LookupResult new_lookup(isolate);
+ self->LocalLookup(*name, &new_lookup);
+ ASSERT(!new_lookup.GetLazyValue()->IsTheHole());
+ if (old_value->IsTheHole() ||
+ new_lookup.GetAttributes() != old_attributes) {
+ self->EnqueueChangeRecord("reconfigured", name, old_value);
+ } else if (!new_lookup.GetLazyValue()->SameValue(*old_value)) {
+ self->EnqueueChangeRecord("updated", name, old_value);
+ }
+ }
}
return *hresult;
@@ -3203,38 +3231,39 @@
? NONE : ABSENT;
}
// Named property.
- LookupResult result(GetIsolate());
- Lookup(key, &result);
- return GetPropertyAttribute(receiver, &result, key, true);
+ LookupResult lookup(GetIsolate());
+ Lookup(key, &lookup);
+ return GetPropertyAttributeForResult(receiver, &lookup, key, true);
}
-PropertyAttributes JSReceiver::GetPropertyAttribute(JSReceiver* receiver,
- LookupResult* result,
- String* name,
- bool continue_search) {
+PropertyAttributes JSReceiver::GetPropertyAttributeForResult(
+ JSReceiver* receiver,
+ LookupResult* lookup,
+ String* name,
+ bool continue_search) {
// Check access rights if needed.
if (IsAccessCheckNeeded()) {
JSObject* this_obj = JSObject::cast(this);
Heap* heap = GetHeap();
if (!heap->isolate()->MayNamedAccess(this_obj, name, v8::ACCESS_HAS)) {
return this_obj->GetPropertyAttributeWithFailedAccessCheck(
- receiver, result, name, continue_search);
+ receiver, lookup, name, continue_search);
}
}
- if (result->IsFound()) {
- switch (result->type()) {
+ if (lookup->IsFound()) {
+ switch (lookup->type()) {
case NORMAL: // fall through
case FIELD:
case CONSTANT_FUNCTION:
case CALLBACKS:
- return result->GetAttributes();
+ return lookup->GetAttributes();
case HANDLER: {
- return
JSProxy::cast(result->proxy())->GetPropertyAttributeWithHandler(
+ return
JSProxy::cast(lookup->proxy())->GetPropertyAttributeWithHandler(
receiver, name);
}
case INTERCEPTOR:
- return result->holder()->GetPropertyAttributeWithInterceptor(
+ return lookup->holder()->GetPropertyAttributeWithInterceptor(
JSObject::cast(receiver), name, continue_search);
case TRANSITION:
case NONEXISTENT:
@@ -3253,9 +3282,9 @@
return ABSENT;
}
// Named property.
- LookupResult result(GetIsolate());
- LocalLookup(name, &result);
- return GetPropertyAttribute(this, &result, name, false);
+ LookupResult lookup(GetIsolate());
+ LocalLookup(name, &lookup);
+ return GetPropertyAttributeForResult(this, &lookup, name, false);
}
@@ -4039,10 +4068,14 @@
return isolate->heap()->false_value();
}
+ // From this point on everything needs to be handlified.
HandleScope scope(isolate);
+ Handle<JSObject> self(this);
+ Handle<String> hname(name);
+
Handle<Object> old_value(isolate->heap()->the_hole_value());
if (FLAG_harmony_observation && map()->is_observed()) {
- // TODO(observe): save oldValue
+ old_value = handle(lookup.GetLazyValue());
}
MaybeObject* result;
@@ -4050,24 +4083,25 @@
if (lookup.IsInterceptor()) {
// Skip interceptor if forcing a deletion.
if (mode == FORCE_DELETION) {
- result = DeletePropertyPostInterceptor(name, mode);
+ result = self->DeletePropertyPostInterceptor(*hname, mode);
} else {
- result = DeletePropertyWithInterceptor(name);
+ result = self->DeletePropertyWithInterceptor(*hname);
}
} else {
// Normalize object if needed.
Object* obj;
- result = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
- if (!result->ToObject(&obj)) return result;
+ result = self->NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
+ if (!result->To(&obj)) return result;
// Make sure the properties are normalized before removing the entry.
- result = DeleteNormalizedProperty(name, mode);
+ result = self->DeleteNormalizedProperty(*hname, mode);
}
Handle<Object> hresult;
if (!result->ToHandle(&hresult)) return result;
if (FLAG_harmony_observation && map()->is_observed()) {
- this->EnqueueChangeRecord("deleted", handle(name), old_value);
+ if (!self->HasLocalProperty(*hname))
+ self->EnqueueChangeRecord("deleted", hname, old_value);
}
return *hresult;
@@ -4669,14 +4703,14 @@
object->DefineAccessor(*name, *getter, *setter, attributes));
}
-MaybeObject* JSObject::DefineAccessor(String* name,
- Object* getter,
- Object* setter,
+MaybeObject* JSObject::DefineAccessor(String* name_raw,
+ Object* getter_raw,
+ Object* setter_raw,
PropertyAttributes attributes) {
Isolate* isolate = GetIsolate();
// Check access rights if needed.
if (IsAccessCheckNeeded() &&
- !isolate->MayNamedAccess(this, name, v8::ACCESS_SET)) {
+ !isolate->MayNamedAccess(this, name_raw, v8::ACCESS_SET)) {
isolate->ReportFailedAccessCheck(this, v8::ACCESS_SET);
return isolate->heap()->undefined_value();
}
@@ -4686,7 +4720,7 @@
if (proto->IsNull()) return this;
ASSERT(proto->IsJSGlobalObject());
return JSObject::cast(proto)->DefineAccessor(
- name, getter, setter, attributes);
+ name_raw, getter_raw, setter_raw, attributes);
}
// Make sure that the top context does not change when doing callbacks or
@@ -4694,30 +4728,47 @@
AssertNoContextChange ncc;
// Try to flatten before operating on the string.
- name->TryFlatten();
+ name_raw->TryFlatten();
+
+ if (!CanSetCallback(name_raw)) return isolate->heap()->undefined_value();
- if (!CanSetCallback(name)) return isolate->heap()->undefined_value();
+ // From this point on everything needs to be handlified.
+ HandleScope scope(GetIsolate());
+ Handle<JSObject> self(this);
+ Handle<String> name(name_raw);
+ Handle<Object> getter(getter_raw);
+ Handle<Object> setter(setter_raw);
+
+ uint32_t index = 0;
+ bool is_element = name->AsArrayIndex(&index);
Handle<Object> old_value(isolate->heap()->the_hole_value());
bool preexists = false;
if (FLAG_harmony_observation && map()->is_observed()) {
- LookupResult result(isolate);
- LocalLookup(name, &result);
- preexists = result.IsFound();
- // TODO(observe): save oldValue
+ if (is_element) {
+ preexists = HasLocalElement(index);
+ if (preexists) {
+ // TODO(observe): distinguish the case where it's an accessor
+ old_value = Object::GetElement(self, index);
+ }
+ } else {
+ LookupResult lookup(isolate);
+ LocalLookup(*name, &lookup);
+ preexists = lookup.IsProperty();
+ if (preexists) old_value = handle(lookup.GetLazyValue());
+ }
}
- uint32_t index = 0;
- MaybeObject* result = name->AsArrayIndex(&index)
- ? DefineElementAccessor(index, getter, setter, attributes)
- : DefinePropertyAccessor(name, getter, setter, attributes);
+ MaybeObject* result = is_element ?
+ self->DefineElementAccessor(index, *getter, *setter, attributes) :
+ self->DefinePropertyAccessor(*name, *getter, *setter, attributes);
Handle<Object> hresult;
if (!result->ToHandle(&hresult)) return result;
if (FLAG_harmony_observation && map()->is_observed()) {
const char* type = preexists ? "reconfigured" : "new";
- this->EnqueueChangeRecord(type, handle(name), old_value);
+ self->EnqueueChangeRecord(type, name, old_value);
}
return *hresult;
=======================================
--- /branches/bleeding_edge/src/objects.h Tue Nov 6 08:47:15 2012
+++ /branches/bleeding_edge/src/objects.h Wed Nov 7 06:14:50 2012
@@ -1499,10 +1499,10 @@
Smi* GenerateIdentityHash();
private:
- PropertyAttributes GetPropertyAttribute(JSReceiver* receiver,
- LookupResult* result,
- String* name,
- bool continue_search);
+ PropertyAttributes GetPropertyAttributeForResult(JSReceiver* receiver,
+ LookupResult* result,
+ String* name,
+ bool continue_search);
DISALLOW_IMPLICIT_CONSTRUCTORS(JSReceiver);
};
=======================================
--- /branches/bleeding_edge/src/property.h Wed Sep 12 09:43:57 2012
+++ /branches/bleeding_edge/src/property.h Wed Nov 7 06:14:50 2012
@@ -290,7 +290,7 @@
case CONSTANT_FUNCTION:
return GetConstantFunction();
default:
- return Smi::FromInt(0);
+ return Isolate::Current()->heap()->the_hole_value();
}
}
=======================================
--- /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Tue Nov
6 04:32:36 2012
+++ /branches/bleeding_edge/test/mjsunit/harmony/object-observe.js Wed Nov
7 06:14:50 2012
@@ -250,26 +250,39 @@
obj["a"] = 3;
delete obj.a;
obj.a = 4;
+obj.a = 4; // ignored
obj.a = 5;
Object.defineProperty(obj, "a", {value: 6});
Object.defineProperty(obj, "a", {writable: false});
obj.a = 7; // ignored
Object.defineProperty(obj, "a", {value: 8});
+Object.defineProperty(obj, "a", {value: 7, writable: true});
Object.defineProperty(obj, "a", {get: function() {}});
-delete obj.a;
Object.defineProperty(obj, "a", {get: function() {}});
+delete obj.a;
+delete obj.a;
+Object.defineProperty(obj, "a", {get: function() {}, configurable: true});
+Object.defineProperty(obj, "a", {value: 9, writable: true});
+obj.a = 10;
+delete obj.a;
+Object.defineProperty(obj, "a", {value: 11, configurable: true});
Object.deliverChangeRecords(observer.callback);
-// TODO(observe): oldValue not included yet.
observer.assertCallbackRecords([
- { object: obj, name: "a", type: "updated" },
- { object: obj, name: "a", type: "updated" },
- { object: obj, name: "a", type: "deleted" },
+ { object: obj, name: "a", type: "updated", oldValue: 1 },
+ { object: obj, name: "a", type: "updated", oldValue: 2 },
+ { object: obj, name: "a", type: "deleted", oldValue: 3 },
{ object: obj, name: "a", type: "new" },
- { object: obj, name: "a", type: "updated" },
- { object: obj, name: "a", type: "updated" },
- { object: obj, name: "a", type: "reconfigured" },
- { object: obj, name: "a", type: "updated" },
+ { 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: "updated", oldValue: 6 },
+ { object: obj, name: "a", type: "reconfigured", oldValue: 8 },
+ { object: obj, name: "a", type: "reconfigured", oldValue: 7 },
{ object: obj, name: "a", type: "reconfigured" },
{ object: obj, name: "a", type: "deleted" },
{ object: obj, name: "a", type: "new" },
+ { object: obj, name: "a", type: "reconfigured" },
+ { object: obj, name: "a", type: "updated", oldValue: 9 },
+ { object: obj, name: "a", type: "deleted", oldValue: 10 },
+ { object: obj, name: "a", type: "new" },
]);
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
