Use [[DefineOwnProperty]] to create properties of changeRecord in NotifierPrototype.notify rather t… (issue 11377171)

[raf...@chromium.org]

Reviewers: rossberg, adamk, arv,

Description:
Use [[DefineOwnProperty]] to create properties of changeRecord in
NotifierPrototype.notify rather than [[Put]]

Note: The test here requires https://codereview.chromium.org/11364237/ to
land
in order to pass because Object.freeze calls Object.getOwnPropertyNames().

BUG=v8:2411

Please review this at https://codereview.chromium.org/11377171/

SVN Base: git://github.com/v8/v8.git@bleeding_edge

Affected files:
M src/object-observe.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
1b0a491a03215765b07ea6a9efa3a0988871daa0..c4c2009aa312a297fb8d112695ae1ef9b6f2214e
100644
--- a/src/object-observe.js
+++ b/src/object-observe.js
@@ -29,6 +29,7 @@

var InternalObjectIsFrozen = $Object.isFrozen;
var InternalObjectFreeze = $Object.freeze;
+var InternalObjectDefineProperty = $Object.defineProperty;

var observationState = %GetObservationState();
if (IS_UNDEFINED(observationState.observerInfoMap)) {
@@ -164,7 +165,11 @@ function ObjectNotifierNotify(changeRecord) {
for (var prop in changeRecord) {
if (prop === 'object')
continue;
- newRecord[prop] = changeRecord[prop];
+
+ InternalObjectDefineProperty(newRecord, prop, {
+ value: changeRecord[prop],
+ enumerable: true
+ });
}
InternalObjectFreeze(newRecord);

Index: test/mjsunit/harmony/object-observe.js
diff --git a/test/mjsunit/harmony/object-observe.js
b/test/mjsunit/harmony/object-observe.js
index
51a07aad4aa275f46fb5863cff440fcee9d4e413..bb73af14ccf61f18cfc20f0bec493d0984419c39
100644
--- a/test/mjsunit/harmony/object-observe.js
+++ b/test/mjsunit/harmony/object-observe.js
@@ -128,8 +128,24 @@ assertFalse(recordCreated); // not observed yet
// Object.deliverChangeRecords
assertThrows(function() { Object.deliverChangeRecords(nonFunction); },
TypeError);

-// Multiple records are delivered.
Object.observe(obj, observer.callback);
+
+// notify uses to [[CreateOwnProperty]] to create changeRecord;
+reset();
+var protoExpandoAccessed = false;
+Object.defineProperty(Object.prototype, 'protoExpando',
+ {
+ configurable: true,
+ set: function() { protoExpandoAccessed = true; }
+ }
+);
+notifier.notify({ type: 'foo', protoExpando: 'val'});
+assertFalse(protoExpandoAccessed);
+delete Object.prototype.protoExpando;
+Object.deliverChangeRecords(observer.callback);
+
+// Multiple records are delivered.
+reset();
notifier.notify({
type: 'updated',
name: 'foo',

[ad...@chromium.org]

https://codereview.chromium.org/11377171/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11377171/diff/1/src/object-observe.js#newcode32
src/object-observe.js:32: var InternalObjectDefineProperty =
$Object.defineProperty;
Not sure these are worth it, at least not until we actually segregate
the globals of these different builtins. Might as well just call
ObjectDefineProperty(), ObjectIsFrozen(), and ObjectFreeze() (all of
which exist and correspond to the proper methods on Object).

https://codereview.chromium.org/11377171/

[raf...@chromium.org]

https://codereview.chromium.org/11377171/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11377171/diff/1/src/object-observe.js#newcode32
src/object-observe.js:32: var InternalObjectDefineProperty =
$Object.defineProperty;

Removed.

[Erik Arvidsson]

v8-natives.js has more efficient functions for this.

[ross...@chromium.org]

LGTM, with nit.


https://codereview.chromium.org/11377171/diff/5001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11377171/diff/5001/src/object-observe.js#newcode165
src/object-observe.js:165: ObjectDefineProperty(newRecord, prop, {
You should be able to use

%DefineOrRedefineDataProperty(newRecord, prop, changeRecord[prop],
READ_ONLY + DONT_DELETE)

which has less overhead.

https://codereview.chromium.org/11377171/

[raf...@chromium.org]

https://codereview.chromium.org/11377171/diff/5001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11377171/diff/5001/src/object-observe.js#newcode165
src/object-observe.js:165: ObjectDefineProperty(newRecord, prop, {

done.