Issue 6452004: Fix issues with using defineProperty on the global proxy object.

Issue 6452004: Fix issues with using defineProperty on the global proxy object. (Closed)

Created:
9 years, 10 months ago by Mads Ager (chromium)

Modified:
9 years, 7 months ago

Reviewers:
Rico

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

Description

Fix issues with using defineProperty on the global proxy object. Committed: http://code.google.com/p/v8/source/detail?r=6683

Patch Set 2 : Address comments. #

Created: 9 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+67 lines, -6 lines)			Patch
M	src/runtime.cc	View		3 chunks	+15 lines, -6 lines	0 comments	Download
M	test/cctest/test-api.cc	View	1	1 chunk	+16 lines, -0 lines	0 comments	Download
A	test/mjsunit/regress/regress-1112.js	View	1	1 chunk	+36 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Rico

LGTM http://codereview.chromium.org/6452004/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/6452004/diff/1/test/cctest/test-api.cc#newcode12563 test/cctest/test-api.cc:12563: " { configurable: true, defineProperty: true, value: 3 ...

9 years, 10 months ago (2011-02-08 14:23:46 UTC) #2

Mads Ager (chromium)

9 years, 10 months ago (2011-02-08 16:30:18 UTC) #3

http://codereview.chromium.org/6452004/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6452004/diff/1/test/cctest/test-api.cc#newcode...
test/cctest/test-api.cc:12563: "    { configurable: true, defineProperty: true,
value: 3 });"
On 2011/02/08 14:23:46, Rico wrote:
> a descriptor does not normally have a defineProperty value, did you mean
> enumerable or writable?

Done.

http://codereview.chromium.org/6452004/diff/1/test/mjsunit/regress/regress-11...
File test/mjsunit/regress/regress-1112.js (right):

http://codereview.chromium.org/6452004/diff/1/test/mjsunit/regress/regress-11...
test/mjsunit/regress/regress-1112.js:31: Object.defineProperty(this, 1,
{configurable: true, defineProperty: true, value: 3});
On 2011/02/08 14:23:46, Rico wrote:
> Long line + a descriptor does not have a defineProperty attribute (maybe
> enumerable in this case?)
> In addition, we do not actually check that the attribute is on the global
object
> as stated by the comment above, maybe we should guarantee this by using
> Object.getOwnPropertyDescriptor(this, '1') and asserting that the returned
> descriptor has the right values

Done.

Expand Messages | Collapse Messages