Object::GetElements() and friends maybehandlification.

Object::GetElements() and friends maybehandlification.

R=yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20644

[Igor Sheludko, 2014-04-09 16:12:08]

PTAL. I added some notes here.

https://codereview.chromium.org/231103002/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/elements.cc#newcode1815
src/elements.cc:1815: true);
I'm returning true in case of exception.

https://codereview.chromium.org/231103002/diff/1/src/handles.cc
File src/handles.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/handles.cc#newcode471
src/handles.cc:471: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate,
FixedArray);
Looks like we do exactly the same here as in the macro + we should return on
exception.

https://codereview.chromium.org/231103002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/runtime.cc#newcode11704
src/runtime.cc:11704: for (int i = 0; i < keys->length(); i++) {
What about adding a HandleScope here?

[Yang, 2014-04-10 07:05:22]

LGTM. Very nice!

Got some comments.

https://codereview.chromium.org/231103002/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/elements.cc#newcode1815
src/elements.cc:1815: true);
On 2014/04/09 16:12:09, Igor Sheludko wrote:
> I'm returning true in case of exception.

I think we usually return false in those cases, see for example
JSProxy::HasPropertyWithHandler.

https://codereview.chromium.org/231103002/diff/1/src/handles.cc
File src/handles.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/handles.cc#newcode486
src/handles.cc:486: if (!result.IsEmpty())
please add curly braces.

https://codereview.chromium.org/231103002/diff/1/src/handles.cc#newcode520
src/handles.cc:520: if (!result.IsEmpty())
add curly braces please.

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

https://codereview.chromium.org/231103002/diff/1/src/objects.cc#newcode7992
src/objects.cc:7992: Handle<FixedArrayBase>::cast(second)).ToHandleChecked();
I think we should return a MaybeHandle on exception here.

https://codereview.chromium.org/231103002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/runtime.cc#newcode11704
src/runtime.cc:11704: for (int i = 0; i < keys->length(); i++) {
On 2014/04/09 16:12:09, Igor Sheludko wrote:
> What about adding a HandleScope here?

Yup. Let's add a handle scope here.

[Igor Sheludko, 2014-04-10 08:27:54]

https://codereview.chromium.org/231103002/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/elements.cc#newcode1815
src/elements.cc:1815: true);
On 2014/04/10 07:05:22, Yang wrote:
> On 2014/04/09 16:12:09, Igor Sheludko wrote:
> > I'm returning true in case of exception.
> 
> I think we usually return false in those cases, see for example
> JSProxy::HasPropertyWithHandler.

Done.

https://codereview.chromium.org/231103002/diff/1/src/handles.cc
File src/handles.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/handles.cc#newcode486
src/handles.cc:486: if (!result.IsEmpty())
On 2014/04/10 07:05:22, Yang wrote:
> please add curly braces.

Done.

https://codereview.chromium.org/231103002/diff/1/src/handles.cc#newcode520
src/handles.cc:520: if (!result.IsEmpty())
On 2014/04/10 07:05:22, Yang wrote:
> add curly braces please.

Done.

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

https://codereview.chromium.org/231103002/diff/1/src/objects.cc#newcode7992
src/objects.cc:7992: Handle<FixedArrayBase>::cast(second)).ToHandleChecked();
On 2014/04/10 07:05:22, Yang wrote:
> I think we should return a MaybeHandle on exception here.

Done.

https://codereview.chromium.org/231103002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/231103002/diff/1/src/runtime.cc#newcode11704
src/runtime.cc:11704: for (int i = 0; i < keys->length(); i++) {
On 2014/04/10 07:05:22, Yang wrote:
> On 2014/04/09 16:12:09, Igor Sheludko wrote:
> > What about adding a HandleScope here?
> 
> Yup. Let's add a handle scope here.

Done.

[Igor Sheludko, 2014-04-10 09:20:30]

Committed patchset #3 manually as r20644 (presubmit successful).