Issue 7035007: Implement get trap for proxies.

Issue 7035007: Implement get trap for proxies. (Closed)

Created:
9 years, 7 months ago by rossberg

Modified:
9 years, 7 months ago

Reviewers:
Kevin Millikin (Chromium)

CC:
v8-dev

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

Visibility:
Public.

Description

Implement get trap for proxies. TODO: reflective Object methods not handled yet. BUG= TEST= Committed: http://code.google.com/p/v8/source/detail?r=7902

Patch Set 2 : Implementing get trap for proxies. #

Total comments: 6

Patch Set 3 : Review feedback. #

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+149 lines, -50 lines)			Patch
M	src/bootstrapper.cc	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	src/builtins.h	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	src/execution.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/execution.cc	View	1 2	1 chunk	+6 lines, -1 line	0 comments	Download
M	src/handles.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/handles.cc	View		1 chunk	+11 lines, -0 lines	0 comments	Download
M	src/ic.cc	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	src/messages.js	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/mirror-debugger.js	View		1 chunk	+6 lines, -5 lines	0 comments	Download
M	src/objects.h	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	src/objects.cc	View	1 2	5 chunks	+66 lines, -24 lines	0 comments	Download
M	src/property.h	View		4 chunks	+17 lines, -9 lines	0 comments	Download
M	src/property.cc	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	src/runtime.js	View	1 2	1 chunk	+17 lines, -0 lines	0 comments	Download
M	src/v8globals.h	View		1 chunk	+6 lines, -5 lines	0 comments	Download
M	test/cctest/test-debug.cc	View		1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Kevin Millikin (Chromium)

LGTM, with small comments below. http://codereview.chromium.org/7035007/diff/1030/src/execution.cc File src/execution.cc (right): http://codereview.chromium.org/7035007/diff/1030/src/execution.cc#newcode157 src/execution.cc:157: Handle<JSFunction> func(JSFunction::cast(*callable)); Should be ...

9 years, 7 months ago (2011-05-16 15:21:41 UTC) #2

rossberg

9 years, 7 months ago (2011-05-16 15:54:37 UTC) #3

http://codereview.chromium.org/7035007/diff/1030/src/execution.cc
File src/execution.cc (right):

http://codereview.chromium.org/7035007/diff/1030/src/execution.cc#newcode157
src/execution.cc:157: Handle<JSFunction> func(JSFunction::cast(*callable));
On 2011/05/16 15:21:41, Kevin Millikin wrote:
> Should be
> 
> Handle<JSFunction> func = Handle<JSFunction>::cast(callable);
> 
> which does not create a new handle.

Done.

http://codereview.chromium.org/7035007/diff/1030/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/7035007/diff/1030/src/messages.js#newcode195
src/messages.js:195: handler_trap_missing:         ["Proxy handler ", "%0", "
has no `", "%1", "' trap"],
On 2011/05/16 15:21:41, Kevin Millikin wrote:
> The other error messages (e.g., "Cannot read property 'blah' of ...") use ' '
> and not ` '.

Done.

http://codereview.chromium.org/7035007/diff/1030/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7035007/diff/1030/src/objects.cc#newcode245
src/objects.cc:245: //    Handle<String> method =
isolate->factory()->LookupAsciiSymbol("get");
On 2011/05/16 15:21:41, Kevin Millikin wrote:
> Is the commented-out code still needed?

Done.

Expand Messages | Collapse Messages