Reviewers: Michael Starzinger,
Description:
Object.observe: Make array length and other magic data properties work
correctly.
Also, disable TestFastElementsLength test for now, since it flakes on
buildbots
for yet unknown reasons.
[email protected]
BUG=
Please review this at https://codereview.chromium.org/11554019/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/object-observe.js
M src/objects.cc
M src/property.h
M src/v8natives.js
M test/mjsunit/harmony/object-observe.js
Index: src/object-observe.js
diff --git a/src/object-observe.js b/src/object-observe.js
index
8c2895f4bdeaf7f14076feae20f4c6b790f96868..3ed27716837d9980a818f6c8d30cd665692eb78b
100644
--- a/src/object-observe.js
+++ b/src/object-observe.js
@@ -84,8 +84,8 @@ function ObjectObserve(object, callback) {
var objectInfo = objectInfoMap.get(object);
if (IS_UNDEFINED(objectInfo)) {
objectInfo = CreateObjectInfo(object);
- %SetIsObserved(object, true);
}
+ %SetIsObserved(object, true);
var changeObservers = objectInfo.changeObservers;
if (changeObservers.indexOf(callback) < 0)
@@ -106,8 +106,11 @@ function ObjectUnobserve(object, callback) {
var changeObservers = objectInfo.changeObservers;
var index = changeObservers.indexOf(callback);
- if (index >= 0)
+ if (index >= 0) {
changeObservers.splice(index, 1);
+ if (changeObservers.length === 0)
+ %SetIsObserved(object, false);
+ }
return object;
}
Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index
173cc7dd8590b04a7e8c4b9d5bada2560e0a9e9d..1d95a5c15271ab3d955e2cbd3f2b6ad1b270cc25
100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2943,8 +2943,9 @@ MaybeObject*
JSObject::SetPropertyForResult(LookupResult* lookup,
}
Handle<Object> old_value(heap->the_hole_value(), isolate);
- if (FLAG_harmony_observation && map()->is_observed()) {
- old_value = handle(lookup->GetLazyValue(), isolate);
+ if (FLAG_harmony_observation &&
+ map()->is_observed() && lookup->IsDataProperty()) {
+ old_value = Object::GetProperty(self, name);
}
// This is a real property that is not read-only, or it is a
@@ -3030,8 +3031,8 @@ MaybeObject*
JSObject::SetPropertyForResult(LookupResult* lookup,
} else {
LookupResult new_lookup(isolate);
self->LocalLookup(*name, &new_lookup, true);
- ASSERT(!new_lookup.GetLazyValue()->IsTheHole());
- if (!new_lookup.GetLazyValue()->SameValue(*old_value)) {
+ if (new_lookup.IsDataProperty() &&
+ !Object::GetProperty(self, name)->SameValue(*old_value)) {
EnqueueChangeRecord(self, "updated", name, old_value);
}
}
@@ -3110,15 +3111,7 @@ MaybeObject*
JSObject::SetLocalPropertyIgnoreAttributes(
PropertyAttributes old_attributes = ABSENT;
bool is_observed = FLAG_harmony_observation &&
self->map()->is_observed();
if (is_observed) {
- // Function prototypes are stored specially
- if (self->IsJSFunction() &&
- JSFunction::cast(*self)->should_have_prototype() &&
- name->Equals(isolate->heap()->prototype_symbol())) {
- MaybeObject* maybe = Accessors::FunctionGetPrototype(*self, NULL);
- if (!maybe->ToHandle(&old_value, isolate)) return maybe;
- } else {
- old_value = handle(lookup.GetLazyValue(), isolate);
- }
+ if (lookup.IsDataProperty()) old_value = Object::GetProperty(self,
name);
old_attributes = lookup.GetAttributes();
}
@@ -3188,11 +3181,11 @@ MaybeObject*
JSObject::SetLocalPropertyIgnoreAttributes(
} else {
LookupResult new_lookup(isolate);
self->LocalLookup(*name, &new_lookup, true);
- ASSERT(!new_lookup.GetLazyValue()->IsTheHole());
if (old_value->IsTheHole() ||
new_lookup.GetAttributes() != old_attributes) {
EnqueueChangeRecord(self, "reconfigured", name, old_value);
- } else if (!new_lookup.GetLazyValue()->SameValue(*old_value)) {
+ } else if (new_lookup.IsDataProperty() &&
+ !Object::GetProperty(self, name)->SameValue(*old_value)) {
EnqueueChangeRecord(self, "updated", name, old_value);
}
}
@@ -4255,8 +4248,8 @@ MaybeObject* JSObject::DeleteProperty(String* name,
DeleteMode mode) {
Handle<Object> old_value(isolate->heap()->the_hole_value());
bool is_observed = FLAG_harmony_observation &&
self->map()->is_observed();
- if (is_observed) {
- old_value = handle(lookup.GetLazyValue(), isolate);
+ if (is_observed && lookup.IsDataProperty()) {
+ old_value = Object::GetProperty(self, hname);
}
MaybeObject* result;
@@ -4947,7 +4940,9 @@ MaybeObject* JSObject::DefineAccessor(String*
name_raw,
LookupResult lookup(isolate);
LocalLookup(*name, &lookup, true);
preexists = lookup.IsProperty();
- if (preexists) old_value = handle(lookup.GetLazyValue(), isolate);
+ if (preexists && lookup.IsDataProperty()) {
+ old_value = Object::GetProperty(self, name);
+ }
}
}
Index: src/property.h
diff --git a/src/property.h b/src/property.h
index
c41c6dc816727db4dee43fab4638656891adcbe1..168b80a11a86c9cd397eadaf29321ad4d083fb5c
100644
--- a/src/property.h
+++ b/src/property.h
@@ -310,6 +310,19 @@ class LookupResult BASE_EMBEDDED {
return IsFound() && !IsTransition();
}
+ bool IsDataProperty() {
+ switch (type()) {
+ case FIELD:
+ case NORMAL:
+ case CONSTANT_FUNCTION:
+ return true;
+ case CALLBACKS:
+ return GetCallbackObject()->IsForeign();
+ default:
+ return false;
+ }
+ }
+
bool IsCacheable() { return cacheable_; }
void DisallowCaching() { cacheable_ = false; }
Index: src/v8natives.js
diff --git a/src/v8natives.js b/src/v8natives.js
index
e8752c84b8b1389dc30ac7315b394243d92fb947..3978e88685156e69ec1660f1ea76d2f8b845b3b9
100644
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -893,16 +893,35 @@ function DefineArrayProperty(obj, p, desc,
should_throw) {
}
// Make sure the below call to DefineObjectProperty() doesn't overwrite
// any magic "length" property by removing the value.
+ // TODO(mstarzinger): This hack should be removed once we have
addressed the
+ // respective TODO in Runtime_DefineOrRedefineDataProperty.
+ // For the time being, we need a hack to prevent Object.observe from
+ // generating two change records.
+ var isObserved = %IsObserved(obj);
+ if (isObserved) %SetIsObserved(obj, false);
obj.length = new_length;
desc.value_ = void 0;
desc.hasValue_ = false;
- if (!DefineObjectProperty(obj, "length", desc, should_throw) || threw)
{
+ threw = !DefineObjectProperty(obj, "length", desc, should_throw) ||
threw;
+ if (isObserved) %SetIsObserved(obj, true);
+ if (threw) {
if (should_throw) {
throw MakeTypeError("redefine_disallowed", [p]);
} else {
return false;
}
}
+ if (isObserved) {
+ var new_desc = GetOwnProperty(obj, "length");
+ var updated = length_desc.value_ !== new_desc.value_;
+ var reconfigured = length_desc.writable_ !== new_desc.writable_ ||
+ length_desc.configurable_ !== new_desc.configurable_ ||
+ length_desc.enumerable_ !== new_desc.configurable_;
+ if (updated || reconfigured) {
+ NotifyChange(reconfigured ? "reconfigured" : "updated",
+ obj, "length", length_desc.value_);
+ }
+ }
return true;
}
Index: test/mjsunit/harmony/object-observe.js
diff --git a/test/mjsunit/harmony/object-observe.js
b/test/mjsunit/harmony/object-observe.js
index
4c5953bec22f67095d8689412d29431663e3865d..4e2594fa1379c13812bd371489af1ff8436a4934
100644
--- a/test/mjsunit/harmony/object-observe.js
+++ b/test/mjsunit/harmony/object-observe.js
@@ -536,16 +536,13 @@ var objects = [
createProxy(Proxy.create, null),
createProxy(Proxy.createFunction, function(){}),
];
-var properties = ["a", "1", 1, "length", "prototype"];
+var properties = ["a", "1", 1, "length", "prototype", "name", "caller"];
// Cases that yield non-standard results.
-// TODO(observe): ...or don't work yet.
function blacklisted(obj, prop) {
return (obj instanceof Int32Array && prop == 1) ||
(obj instanceof Int32Array && prop === "length") ||
- (obj instanceof ArrayBuffer && prop == 1) ||
- // TODO(observe): oldValue when reconfiguring array length
- (obj instanceof Array && prop === "length")
+ (obj instanceof ArrayBuffer && prop == 1)
}
for (var i in objects) for (var j in properties) {
@@ -581,8 +578,10 @@ Object.observe(arr3, observer.callback);
arr.length = 2;
arr.length = 0;
arr.length = 10;
+Object.defineProperty(arr, 'length', {writable: false});
arr2.length = 0;
arr2.length = 1; // no change expected
+Object.defineProperty(arr2, 'length', {value: 1, writable: false});
arr3.length = 0;
Object.defineProperty(arr3, 'length', {value: 5});
Object.defineProperty(arr3, 'length', {value: 10, writable: false});
@@ -594,15 +593,15 @@ 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: arr2, name: '1', type: 'deleted', oldValue: 'beta' },
{ object: arr2, name: 'length', type: 'updated', oldValue: 2 },
+ { object: arr2, name: 'length', type: 'reconfigured', oldValue: 1 },
{ object: arr3, name: '2', type: 'deleted', oldValue: 'goodbye' },
{ object: arr3, name: '0', type: 'deleted', oldValue: 'hello' },
{ object: arr3, name: 'length', type: 'updated', oldValue: 6 },
{ object: arr3, name: 'length', type: 'updated', oldValue: 0 },
- { object: arr3, name: 'length', type: 'updated', oldValue: 5 },
- // TODO(adamk): This record should be merged with the above
- { object: arr3, name: 'length', type: 'reconfigured' },
+ { object: arr3, name: 'length', type: 'reconfigured', oldValue: 5 },
]);
// Assignments in loops (checking different IC states).
@@ -944,8 +943,11 @@ function TestFastElementsLength(polymorphic, optimize,
oldSize, newSize) {
}
}
+// TODO(rossberg): Still flaky on buildbots, disable for now...
+/*
for (var b1 = 0; b1 < 2; ++b1)
for (var b2 = 0; b2 < 2; ++b2)
for (var n1 = 0; n1 < 3; ++n1)
for (var n2 = 0; n2 < 3; ++n2)
TestFastElementsLength(b1 != 0, b2 != 0, 20*n1, 20*n2);
+*/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
