Expose v8::Function::GetDisplayName to public API.

Expose v8::Function::GetDisplayName to public API.

BUG=chromium:17356
R=mstarzinger@chromium.org, yurys@chromium.org, yangguo@chromium.org, yurys

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

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

[yurys, 2013-10-16 14:57:52]

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

https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4115
src/api.cc:4115: Handle<Value> Function::GetDisplayName() const {
If you anyways return either empty handle or String, can we change return type
to Handle<String>?

https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4126
src/api.cc:4126: func->LookupRealNamedProperty(*property_name, &lookup);
Should we count displayName property declared on prototype?

[Michael Starzinger, 2013-10-16 15:15:01]

What is the difference between this new entry point and just calling
v8::Object::GetRealNamedProperty with the appropriate parameters?

[aandrey, 2013-10-16 15:17:00]

On 2013/10/16 15:15:01, Michael Starzinger wrote:
> What is the difference between this new entry point and just calling
> v8::Object::GetRealNamedProperty with the appropriate parameters?

We do not want to execute any getters. The GetRealNamedProperty executes the
getter function, does it?

[Michael Starzinger, 2013-10-16 15:19:03]

Hmpf, yep, you are right. I was misled by the "real" prefix in the function name
and the comment about the function not calling interceptors. But of course if
calls callbacks. :(

[aandrey, 2013-10-16 15:19:44]

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

https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4126
src/api.cc:4126: func->LookupRealNamedProperty(*property_name, &lookup);
On 2013/10/16 14:57:52, Yury Semikhatsky wrote:
> Should we count displayName property declared on prototype?

I think we should. And this code does look up on prototypes.

FYI, quick reference:

void JSObject::LookupRealNamedProperty(Name* name, LookupResult* result) {
  LocalLookupRealNamedProperty(name, result);
  if (result->IsFound()) return;

  LookupRealNamedPropertyInPrototypes(name, result);
}

[aandrey, 2013-10-16 15:25:19]

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

https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4115
src/api.cc:4115: Handle<Value> Function::GetDisplayName() const {
On 2013/10/16 14:57:52, Yury Semikhatsky wrote:
> If you anyways return either empty handle or String, can we change return type
> to Handle<String>?

I guess we can, but there are 2 similar methods defined already:

Handle<Value> GetName() const;
Handle<Value> GetInferredName() const;

I think all these should be consistent in return value. Should we change all of
them? I guess it could be painful to change the public API?

[yurys, 2013-10-17 07:40:18]

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

https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4115
src/api.cc:4115: Handle<Value> Function::GetDisplayName() const {
On 2013/10/16 15:25:19, aandrey wrote:
> On 2013/10/16 14:57:52, Yury Semikhatsky wrote:
> > If you anyways return either empty handle or String, can we change return
type
> > to Handle<String>?
> 
> I guess we can, but there are 2 similar methods defined already:
> 
> Handle<Value> GetName() const;
> Handle<Value> GetInferredName() const;
> 
> I think all these should be consistent in return value. Should we change all
of
> them? I guess it could be painful to change the public API?
That's because they will return Undefined when there is no {inferred}name while
you return an empty handle in that case. So to be consistent with them you
should also return Undefined in case there is no displayName.

[aandrey, 2013-10-17 08:03:34]

On 2013/10/17 07:40:18, Yury Semikhatsky wrote:
> https://codereview.chromium.org/26709011/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4115
> src/api.cc:4115: Handle<Value> Function::GetDisplayName() const {
> On 2013/10/16 15:25:19, aandrey wrote:
> > On 2013/10/16 14:57:52, Yury Semikhatsky wrote:
> > > If you anyways return either empty handle or String, can we change return
> type
> > > to Handle<String>?
> > 
> > I guess we can, but there are 2 similar methods defined already:
> > 
> > Handle<Value> GetName() const;
> > Handle<Value> GetInferredName() const;
> > 
> > I think all these should be consistent in return value. Should we change all
> of
> > them? I guess it could be painful to change the public API?
> That's because they will return Undefined when there is no {inferred}name
while
> you return an empty handle in that case. So to be consistent with them you
> should also return Undefined in case there is no displayName.

I thought the Local<v8::Value>() does represent the JS's "undefined".
If not, what does it stand for?

And how to return an Undefined? just "return Undefined();" instead of "return
Local<v8::Value>();" ?

[yurys, 2013-10-17 09:59:05]

On 2013/10/17 08:03:34, aandrey wrote:
> On 2013/10/17 07:40:18, Yury Semikhatsky wrote:
> > https://codereview.chromium.org/26709011/diff/1/src/api.cc
> > File src/api.cc (right):
> > 
> > https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4115
> > src/api.cc:4115: Handle<Value> Function::GetDisplayName() const {
> > On 2013/10/16 15:25:19, aandrey wrote:
> > > On 2013/10/16 14:57:52, Yury Semikhatsky wrote:
> > > > If you anyways return either empty handle or String, can we change
return
> > type
> > > > to Handle<String>?
> > > 
> > > I guess we can, but there are 2 similar methods defined already:
> > > 
> > > Handle<Value> GetName() const;
> > > Handle<Value> GetInferredName() const;
> > > 
> > > I think all these should be consistent in return value. Should we change
all
> > of
> > > them? I guess it could be painful to change the public API?
> > That's because they will return Undefined when there is no {inferred}name
> while
> > you return an empty handle in that case. So to be consistent with them you
> > should also return Undefined in case there is no displayName.
> 
> I thought the Local<v8::Value>() does represent the JS's "undefined".
> If not, what does it stand for?
> 
> And how to return an Undefined? just "return Undefined();" instead of "return
> Local<v8::Value>();" ?

Empty handle is a handle that doesn't point to any object which is different
from a handle to "undefined" or "null" value. You can create a handle to
"undefined" e.g. the following way: 

ToApiHandle<Primitive>(isolate->factory()->undefined_value());

[aandrey, 2013-10-17 10:55:07]

On 2013/10/17 09:59:05, Yury Semikhatsky wrote:
> On 2013/10/17 08:03:34, aandrey wrote:
> > On 2013/10/17 07:40:18, Yury Semikhatsky wrote:
> > > https://codereview.chromium.org/26709011/diff/1/src/api.cc
> > > File src/api.cc (right):
> > > 
> > > https://codereview.chromium.org/26709011/diff/1/src/api.cc#newcode4115
> > > src/api.cc:4115: Handle<Value> Function::GetDisplayName() const {
> > > On 2013/10/16 15:25:19, aandrey wrote:
> > > > On 2013/10/16 14:57:52, Yury Semikhatsky wrote:
> > > > > If you anyways return either empty handle or String, can we change
> return
> > > type
> > > > > to Handle<String>?
> > > > 
> > > > I guess we can, but there are 2 similar methods defined already:
> > > > 
> > > > Handle<Value> GetName() const;
> > > > Handle<Value> GetInferredName() const;
> > > > 
> > > > I think all these should be consistent in return value. Should we change
> all
> > > of
> > > > them? I guess it could be painful to change the public API?
> > > That's because they will return Undefined when there is no {inferred}name
> > while
> > > you return an empty handle in that case. So to be consistent with them you
> > > should also return Undefined in case there is no displayName.
> > 
> > I thought the Local<v8::Value>() does represent the JS's "undefined".
> > If not, what does it stand for?
> > 
> > And how to return an Undefined? just "return Undefined();" instead of
"return
> > Local<v8::Value>();" ?
> 
> Empty handle is a handle that doesn't point to any object which is different
> from a handle to "undefined" or "null" value. You can create a handle to
> "undefined" e.g. the following way: 
> 
> ToApiHandle<Primitive>(isolate->factory()->undefined_value());

Done. PTAL.

[yurys, 2013-10-17 11:51:33]

lgtm

[aandrey, 2013-10-17 12:13:30]

need lg from v8 owners

[aandrey, 2013-10-21 15:27:11]

ping??

[Michael Starzinger, 2013-10-22 14:37:50]

LGTM.

[yurys, 2013-10-22 14:57:27]

Committed patchset #2 manually as r17324 (presubmit successful).

[yurys, 2013-10-23 12:38:14]

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

[arv (Not doing code reviews), 2013-11-04 23:37:20]

FYI, TC39 opposed adding displayName. Instead ES6 makes Function name writable.
That should be in the latest spec draft.

[aandrey, 2013-11-05 09:59:20]

On 2013/11/04 23:37:20, arv wrote:
> FYI, TC39 opposed adding displayName. Instead ES6 makes Function name
writable.
> That should be in the latest spec draft.

Thanks for the pointer!

I guess, we should implement displayName anyway for now, in order to match FF
debugger's behavior.
We can remove it later, after ES6 is implemented and most libraries migrate from
displayName to setting function names directly via this API.

wdyt?

[yurys, 2013-11-05 10:22:38]

On 2013/11/05 09:59:20, aandrey wrote:
> On 2013/11/04 23:37:20, arv wrote:
> > FYI, TC39 opposed adding displayName. Instead ES6 makes Function name
> writable.
> > That should be in the latest spec draft.
> 
> Thanks for the pointer!
> 
> I guess, we should implement displayName anyway for now, in order to match FF
> debugger's behavior.
> We can remove it later, after ES6 is implemented and most libraries migrate
from
> displayName to setting function names directly via this API.
> 
> wdyt?

I agree with Andrey, this is a de-facto standard supported by FF and one of the
most requested features of DevTools debugger (59 stars at
https://code.google.com/p/chromium/issues/detail?id=17356). If we need to we can
drop this when writable function name is supported.