Return MaybeHandle from Invoke.

Return MaybeHandle from Invoke.

R=ishell@chromium.org

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

[Igor Sheludko, 2014-04-10 15:14:41]

First round of comments.

https://codereview.chromium.org/231883007/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/231883007/diff/1/src/debug.cc#newcode796
src/debug.cc:796: if (!exception.is_null()) {
I think exception can't be null here anymore.

https://codereview.chromium.org/231883007/diff/1/src/debug.cc#newcode1141
src/debug.cc:1141: ASSERT(!result.is_null());
While we are here: It's too late to ASSERT this and probably we don't have to.

https://codereview.chromium.org/231883007/diff/1/src/debug.cc#newcode1142
src/debug.cc:1142: return (*result)->IsTrue();
return result->IsTrue();

https://codereview.chromium.org/231883007/diff/1/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/231883007/diff/1/src/execution.cc#newcode187
src/execution.cc:187: result = v8::Utils::OpenHandle(*catcher.Exception());
TryCall() returns either result or exception and never null. So we will loose
the knowledge whether there was an exception or not.

[Igor Sheludko, 2014-04-10 21:34:29]

https://codereview.chromium.org/231883007/diff/20001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/231883007/diff/20001/src/debug.cc#newcode2615
src/debug.cc:2615: &caught_exception);
I think we should write here and in all other places where TryCall() is used
something like this:
  if (caught_exception) return MaybeHandle<Object>();
  return result;
Because callers of MakeJSObject() expect empty handle on exception.

[Igor Sheludko, 2014-04-10 21:44:34]

I'm sorry for sending comments in many mails.

https://codereview.chromium.org/231883007/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/231883007/diff/20001/src/factory.cc#newcode1168
src/factory.cc:1168: return Handle<Object>(isolate()->pending_exception(),
isolate());
I think we should return result here as well, because according to the
ASSERT(!isolate->has_pending_exception()); at the end of TryCall() there would
be no exception in the isolate. Please add a comment here about that.

[Igor Sheludko, 2014-04-11 09:30:02]

This is a great cleanup!

lgtm with nits:

https://codereview.chromium.org/231883007/diff/20001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/231883007/diff/20001/src/factory.h#newcode510
src/factory.h:510: MaybeHandle<FunctionTemplateInfo> ConfigureInstance(
MUST_USE_RESULT

https://codereview.chromium.org/231883007/diff/40001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/231883007/diff/40001/src/execution.cc#newcode166
src/execution.cc:166: Handle<Object>* exception_out) {
Good idea!

https://codereview.chromium.org/231883007/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/231883007/diff/40001/src/factory.cc#newcode2007
src/factory.cc:2007: if (instance_template->IsUndefined() ||
I think it would be more clear if we keep it like this (just ignore this if you
are not agree):
if (!instance_template->IsUndefined()) {
  RETURN_ON_EXCEPTION(
    isolate,
    Execution::ConfigureInstance(...),
    FunctionTemplateInfo);
}
return desc;

[Yang, 2014-04-11 09:41:51]

https://codereview.chromium.org/231883007/diff/20001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/231883007/diff/20001/src/factory.h#newcode510
src/factory.h:510: MaybeHandle<FunctionTemplateInfo> ConfigureInstance(
On 2014/04/11 09:30:03, Igor Sheludko wrote:
> MUST_USE_RESULT

Done.

https://codereview.chromium.org/231883007/diff/40001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/231883007/diff/40001/src/factory.cc#newcode2007
src/factory.cc:2007: if (instance_template->IsUndefined() ||
On 2014/04/11 09:30:03, Igor Sheludko wrote:
> I think it would be more clear if we keep it like this (just ignore this if
you
> are not agree):
> if (!instance_template->IsUndefined()) {
>   RETURN_ON_EXCEPTION(
>     isolate,
>     Execution::ConfigureInstance(...),
>     FunctionTemplateInfo);
> }
> return desc;

Good idea. Done.

[Yang, 2014-04-11 10:41:27]

Committed patchset #4 manually as r20680 (presubmit successful).