[proxies] Update Object.defineProperty/ies for JSProxies

[proxies] Update Object.defineProperty/ies for JSProxies

BUG=v8:1543
LOG=n

Committed: https://crrev.com/6f0850c63d47defd37c76ac4d654df6a3919635e
Cr-Commit-Position: refs/heads/master@{#32103}

[Jakob Kummerow, 2015-11-17 16:32:40]

jkummerow@chromium.org changed reviewers:
+ neis@chromium.org

[Jakob Kummerow, 2015-11-17 16:32:41]

PTAL.

[neis, 2015-11-17 21:37:40]

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6069
src/objects.cc:6069: if (!object->IsJSObject()) return true;
This condition will now always be false.

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6659
src/objects.cc:6659: // 3. If handler is null, throw a TypeError exception.
I would prefer using an IsRevoked function here.

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6668
src/objects.cc:6668: DCHECK(object->target()->IsSpecObject());
What is the relationship between IsSpecObject and IsJSReceiver?

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6698
src/objects.cc:6698: if (trap_result_obj->BooleanValue() == false) {
if (!...)

https://codereview.chromium.org/1456613002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1456613002/diff/1/src/objects.h#newcode9549
src/objects.h:9549: 
Can we get rid of the "WithHandler" suffix?

https://codereview.chromium.org/1456613002/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-define-property.js (right):

https://codereview.chromium.org/1456613002/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-define-property.js:30:
assertThrows("Object.defineProperty(proxy, 'foo', {value: 2})");
Here and below, check that it throws a TypeError.

https://codereview.chromium.org/1456613002/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-define-property.js:59:
assertThrows("Object.defineProperty(proxy, 'foo', desc)");
Maybe also test the special case of this special case ("[...] unless there
exists a corresponding non-configurable own property of the target object.")

[Jakob Kummerow, 2015-11-18 14:35:50]

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6069
src/objects.cc:6069: if (!object->IsJSObject()) return true;
On 2015/11/17 21:37:40, neis wrote:
> This condition will now always be false.

Done.

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6659
src/objects.cc:6659: // 3. If handler is null, throw a TypeError exception.
On 2015/11/17 21:37:40, neis wrote:
> I would prefer using an IsRevoked function here.

Yeah, well, we don't have that yet. I've added a TODO.

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6668
src/objects.cc:6668: DCHECK(object->target()->IsSpecObject());
On 2015/11/17 21:37:40, neis wrote:
> What is the relationship between IsSpecObject and IsJSReceiver?

They're identical. Consolidating the entire codebase on one of them is a
long-standing cleanup TODO.

https://codereview.chromium.org/1456613002/diff/1/src/objects.cc#newcode6698
src/objects.cc:6698: if (trap_result_obj->BooleanValue() == false) {
On 2015/11/17 21:37:40, neis wrote:
> if (!...)

Done. (I was intentionally staying close to the spec, but I don't care much.)

https://codereview.chromium.org/1456613002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1456613002/diff/1/src/objects.h#newcode9549
src/objects.h:9549: 
On 2015/11/17 21:37:40, neis wrote:
> Can we get rid of the "WithHandler" suffix?

Done.

https://codereview.chromium.org/1456613002/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-define-property.js (right):

https://codereview.chromium.org/1456613002/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-define-property.js:30:
assertThrows("Object.defineProperty(proxy, 'foo', {value: 2})");
On 2015/11/17 21:37:40, neis wrote:
> Here and below, check that it throws a TypeError.

Done.

https://codereview.chromium.org/1456613002/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-define-property.js:59:
assertThrows("Object.defineProperty(proxy, 'foo', desc)");
On 2015/11/17 21:37:40, neis wrote:
> Maybe also test the special case of this special case ("[...] unless there
> exists a corresponding non-configurable own property of the target object.")

Done.

[neis, 2015-11-18 21:27:11]

Thanks, lgtm.

[Jakob Kummerow, 2015-11-18 22:10:52]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2015-11-18 22:11:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456613002/20001

[commit-bot: I haz the power, 2015-11-18 22:13:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-18 22:14:00]

Try jobs failed on following builders:
  v8_win_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/11937)

[Jakob Kummerow, 2015-11-19 09:03:46]

The CQ bit was checked by jkummerow@chromium.org

[commit-bot: I haz the power, 2015-11-19 09:03:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456613002/20001

[commit-bot: I haz the power, 2015-11-19 09:21:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-19 09:22:00]

Patchset 2 (id:??) landed as
https://crrev.com/6f0850c63d47defd37c76ac4d654df6a3919635e
Cr-Commit-Position: refs/heads/master@{#32103}