Reviewers: rossberg,
Description:
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).
Please review this at https://codereview.chromium.org/11820004/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/objects.cc
M test/mjsunit/harmony/object-observe.js
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()) {
+ 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()) {
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;
Index: test/mjsunit/harmony/object-observe.js
diff --git a/test/mjsunit/harmony/object-observe.js
b/test/mjsunit/harmony/object-observe.js
index
4836eaa453d9190af081c1cf55a0e17e71d433f8..b7ea22e32d04c36dd3a205fe82a71d3edf75c733
100644
--- a/test/mjsunit/harmony/object-observe.js
+++ b/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 },
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
