Get information about original function from bound function

Add Function::GetBoundFunction to the API.

This adds functionality for getting original functions from bound functions.

It's of much use when information about function calls is shown on the Events timeline in DevTools: instead of referencing to v8natives.js where bound functions are created, we'll be able to show real function data (name, resource, script line and column numbers) retrieved from original functions.

BUG=None
LOG=Y
R=yangguo@chromium.org, yurys@chromium.org

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

[Alexandra Mikhaylova, 2013-12-05 16:18:16]

The very 1st version of this feature

[yurys, 2013-12-09 12:17:16]

Also please add a test for this change.

https://codereview.chromium.org/106763002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/106763002/diff/1/src/api.cc#newcode4222
src/api.cc:4222: if (func->shared()->bound()) {
We should add Function::GetBoundFunction() instead of adding additional logic
into many Function methods.

[Alexandra Mikhaylova, 2013-12-12 12:41:14]

https://codereview.chromium.org/106763002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/106763002/diff/1/src/api.cc#newcode4222
src/api.cc:4222: if (func->shared()->bound()) {
On 2013/12/09 12:17:16, yurys wrote:
> We should add Function::GetBoundFunction() instead of adding additional logic
> into many Function methods.

Done.

[yurys, 2013-12-12 12:47:15]

https://codereview.chromium.org/106763002/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/106763002/diff/20001/include/v8.h#newcode2669
include/v8.h:2669: * Returns the original function if this function is bound,
else returns this
"...if this function is bound using Function.prototype.bind..."

Also this method should return empty handle or v8::Undefined in case the
function is not bound, otherwise there is no way to tell whether the function is
bound or not.

[yurys, 2013-12-12 12:49:27]

https://codereview.chromium.org/106763002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/106763002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17764: env->GetIsolate(), "test"));
style: wrong indentation

[Alexandra Mikhaylova, 2013-12-12 15:53:01]

https://codereview.chromium.org/106763002/diff/20001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/106763002/diff/20001/include/v8.h#newcode2669
include/v8.h:2669: * Returns the original function if this function is bound,
else returns this
On 2013/12/12 12:47:15, yurys wrote:
> "...if this function is bound using Function.prototype.bind..."
> 
> Also this method should return empty handle or v8::Undefined in case the
> function is not bound, otherwise there is no way to tell whether the function
is
> bound or not.

Done.

https://codereview.chromium.org/106763002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/106763002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17764: env->GetIsolate(), "test"));
On 2013/12/12 12:49:27, yurys wrote:
> style: wrong indentation

Done.

[yurys, 2013-12-12 20:20:50]

Please provide meaningful change description and link to a bug if there is one.

https://codereview.chromium.org/106763002/diff/30001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/106763002/diff/30001/src/api.cc#newcode4317
src/api.cc:4317: i::Isolate::Current());
func->GetIsolate())

https://codereview.chromium.org/106763002/diff/30001/src/api.cc#newcode4318
src/api.cc:4318: return
Utils::ToLocal(i::Handle<i::JSFunction>::cast(original));
Is it always safe to cast original to JSFunction here?

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17853: CHECK(!g->GetBoundFunction()->IsUndefined());
CHECK(g->GetBoundFunction()->IsFunction())

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17854: CHECK_EQ(*v8::String::Utf8Value(f->GetName()),
CHECK_EQ should work well for Handles, you don't need to to cast them to
Utf8Value

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17856:
Local<v8::Function>::Cast(g->GetBoundFunction())->GetName()));
It may make sense to extract Local<v8::Function>::Cast(g->GetBoundFunction())
into a separate statement to avoid duplication.

[Alexandra Mikhaylova, 2013-12-18 10:00:27]

https://codereview.chromium.org/106763002/diff/30001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/106763002/diff/30001/src/api.cc#newcode4317
src/api.cc:4317: i::Isolate::Current());
On 2013/12/12 20:20:50, yurys wrote:
> func->GetIsolate())

Done.

https://codereview.chromium.org/106763002/diff/30001/src/api.cc#newcode4318
src/api.cc:4318: return
Utils::ToLocal(i::Handle<i::JSFunction>::cast(original));
On 2013/12/12 20:20:50, yurys wrote:
> Is it always safe to cast original to JSFunction here?

I think it is safe because here we are sure that our function is bound and this
is its original function.

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17853: CHECK(!g->GetBoundFunction()->IsUndefined());
On 2013/12/12 20:20:50, yurys wrote:
> CHECK(g->GetBoundFunction()->IsFunction())

Done.

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17854: CHECK_EQ(*v8::String::Utf8Value(f->GetName()),
On 2013/12/12 20:20:50, yurys wrote:
> CHECK_EQ should work well for Handles, you don't need to to cast them to
> Utf8Value

Done.

https://codereview.chromium.org/106763002/diff/30001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17856:
Local<v8::Function>::Cast(g->GetBoundFunction())->GetName()));
On 2013/12/12 20:20:50, yurys wrote:
> It may make sense to extract Local<v8::Function>::Cast(g->GetBoundFunction())
> into a separate statement to avoid duplication.

Done.

[yurys, 2013-12-18 10:10:14]

https://codereview.chromium.org/106763002/diff/50001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/106763002/diff/50001/src/api.cc#newcode4316
src/api.cc:4316:
i::JSReceiver::cast(bound_args->get(i::JSFunction::kBoundFunctionIndex)),
Why do you need to cast to i::JSReceiver here?

https://codereview.chromium.org/106763002/diff/50001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/106763002/diff/50001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17847: "var g = f.bind();\n");
Maybe add some arguments to make sure this is not optimized out?

https://codereview.chromium.org/106763002/diff/50001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17854: Local<v8::Function> g_original =
Local<v8::Function>::Cast(
g_original -> original_function?

[Alexandra Mikhaylova, 2013-12-18 14:16:06]

https://codereview.chromium.org/106763002/diff/50001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/106763002/diff/50001/src/api.cc#newcode4316
src/api.cc:4316:
i::JSReceiver::cast(bound_args->get(i::JSFunction::kBoundFunctionIndex)),
On 2013/12/18 10:10:14, yurys wrote:
> Why do you need to cast to i::JSReceiver here?

Thanks, I think we can make it without i::JSReceiver here, I've corrected it.

https://codereview.chromium.org/106763002/diff/50001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/106763002/diff/50001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17847: "var g = f.bind();\n");
On 2013/12/18 10:10:14, yurys wrote:
> Maybe add some arguments to make sure this is not optimized out?

Done.

https://codereview.chromium.org/106763002/diff/50001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17854: Local<v8::Function> g_original =
Local<v8::Function>::Cast(
On 2013/12/18 10:10:14, yurys wrote:
> g_original -> original_function?

Done.

[yurys, 2013-12-18 15:02:32]

LGTM, you'll need OWNER's approval.

[Alexandra Mikhaylova, 2013-12-18 15:09:21]

Hi Yang,

Could you please review this issue as the OWNER.

[Yang, 2013-12-19 10:32:35]

On 2013/12/18 15:09:21, Alexandra Mikhaylova wrote:
> Hi Yang,
> 
> Could you please review this issue as the OWNER.

Why do we need this? We already have the runtime function
%BoundFunctionGetBindings, which essentially does the same. It's used in
mirror-debugger.js in ObjectMirror.GetInternalProperties. Assuming that the
Javascript debugger API is indeed used by devtools, wouldn't this be more
convenient?

[yurys, 2013-12-19 11:54:57]

On 2013/12/19 10:32:35, Yang wrote:
> On 2013/12/18 15:09:21, Alexandra Mikhaylova wrote:
> > Hi Yang,
> > 
> > Could you please review this issue as the OWNER.
> 
> Why do we need this? We already have the runtime function
> %BoundFunctionGetBindings, which essentially does the same. It's used in
> mirror-debugger.js in ObjectMirror.GetInternalProperties. Assuming that the
> Javascript debugger API is indeed used by devtools, wouldn't this be more
> convenient?

We need to have access to this information in the Blink code that doesn't have
access to V8 debugger and in particular to the ExecutionState with the mirror
objects (e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
it relies on the native APIs for accessing ScriptOrigin and call stack defined
in v8.h

[Yang, 2013-12-19 11:57:27]

On 2013/12/19 11:54:57, yurys wrote:
> On 2013/12/19 10:32:35, Yang wrote:
> > On 2013/12/18 15:09:21, Alexandra Mikhaylova wrote:
> > > Hi Yang,
> > > 
> > > Could you please review this issue as the OWNER.
> > 
> > Why do we need this? We already have the runtime function
> > %BoundFunctionGetBindings, which essentially does the same. It's used in
> > mirror-debugger.js in ObjectMirror.GetInternalProperties. Assuming that the
> > Javascript debugger API is indeed used by devtools, wouldn't this be more
> > convenient?
> 
> We need to have access to this information in the Blink code that doesn't have
> access to V8 debugger and in particular to the ExecutionState with the mirror
> objects (e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
> it relies on the native APIs for accessing ScriptOrigin and call stack defined
> in v8.h

In that case, LGTM.

[yurys, 2013-12-23 08:05:04]

Committed patchset #5 manually as r18401 (presubmit successful).