messages.js: Get better function names in stack traces.

messages.js: Get better function names in stack traces.

CallSite.getFunctionName() is able to retrieve names for functions better than
getFunction().name.  Use it in CallSite.toString().

Code by marja@chromium.org.

BUG=NONE
TEST=stack-traces.js: Added testClassNames.

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

[marja, 2012-05-16 09:04:02]

ptal

[Yang, 2012-05-16 09:22:01]

On 2012/05/16 09:04:02, marja wrote:
> ptal

LGTM. But this may break webkit layout tests if the text output is different
than expected. (You will then have to rebaseline those tests etc.)

[marja, 2012-05-16 12:22:10]

LayoutTests/platform/chromium/test_expectations.txt says:

// Tests Error.stack behavior. We aren't consistent with JSC and do not intend
to become so.
WONTFIX : fast/js/stack-trace.html = TEXT

So I'd assume this change is safe from WebKit pov.

[Yang, 2012-05-16 13:26:14]

On 2012/05/16 12:22:10, marja wrote:
> LayoutTests/platform/chromium/test_expectations.txt says:
> 
> // Tests Error.stack behavior. We aren't consistent with JSC and do not intend
> to become so.
> WONTFIX : fast/js/stack-trace.html = TEXT
> 
> So I'd assume this change is safe from WebKit pov.

Great. I'll land this.

[Yang, 2012-05-16 15:15:32]

On 2012/05/16 13:26:14, Yang wrote:
> On 2012/05/16 12:22:10, marja wrote:
> > LayoutTests/platform/chromium/test_expectations.txt says:
> > 
> > // Tests Error.stack behavior. We aren't consistent with JSC and do not
intend
> > to become so.
> > WONTFIX : fast/js/stack-trace.html = TEXT
> > 
> > So I'd assume this change is safe from WebKit pov.
> 
> Great. I'll land this.

I just ran the v8 tests and it turns out that a few tests are failing (probably
have to update some expectations). I can take this over if you want to.

[marja, 2012-05-23 12:23:33]

Hi (again),

I fixed the brokenness detected by the existing tests and added a new one
defining the case where this yields better traces than the previous code.

I'm not entirely sure that there are no corner cases left...

yangguo: plesner said he could review, I changed him to be the reviewer but
you're welcome to have a look, too.

plesner: ptal.

[Yang, 2012-05-23 12:37:09]

On 2012/05/23 12:23:33, marja wrote:
> Hi (again),
> 
> I fixed the brokenness detected by the existing tests and added a new one
> defining the case where this yields better traces than the previous code.
> 
> I'm not entirely sure that there are no corner cases left...
> 
> yangguo: plesner said he could review, I changed him to be the reviewer but
> you're welcome to have a look, too.
> 
> plesner: ptal.

That's alright with me. Tell me if you need me to land that for you.

[Christian Plesner Hansen, 2012-05-24 02:15:18]

(Replying using the account I used to use for v8 stuff)

http://codereview.chromium.org/10384196/diff/18003/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode834
src/messages.js:834: return %FunctionGetInferredName(this.fun);
I wonder if the eval case should have been reached if FunctionGetInferredName
returned null? If so it might make sense to change the logic to that and keep
the code that's removed here.

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode949
src/messages.js:949: var addSuffix = true;
I always get suffix and prefix mixed up. I do the same with left and right.
Thanks for fixing it.

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode956
src/messages.js:956: if (typeNamePrefix[0] != '[' &&
Something to consider to make this case easier to detect could be to change
getTypeName to take an optional argument that, if truthy, tells getTypeName to
return null in the cases where it otherwise just converts the object to a
string? Just a thought, it may not be a good idea.

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode961
src/messages.js:961: if (methodName && functionName.indexOf("." + methodName) !=
Is this equivalent to (methodName != this.getFunction().name)? If so that might
be a simpler way to express it.

[marja, 2012-05-24 08:38:16]

http://codereview.chromium.org/10384196/diff/18003/src/messages.js
File src/messages.js (right):

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode834
src/messages.js:834: return %FunctionGetInferredName(this.fun);
On 2012/05/24 02:15:18, Christian Plesner Hansen wrote:
> I wonder if the eval case should have been reached if FunctionGetInferredName
> returned null? If so it might make sense to change the logic to that and keep
> the code that's removed here.

Done.

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode956
src/messages.js:956: if (typeNamePrefix[0] != '[' &&
On 2012/05/24 02:15:18, Christian Plesner Hansen wrote:
> Something to consider to make this case easier to detect could be to change
> getTypeName to take an optional argument that, if truthy, tells getTypeName to
> return null in the cases where it otherwise just converts the object to a
> string? Just a thought, it may not be a good idea.

Done.

http://codereview.chromium.org/10384196/diff/18003/src/messages.js#newcode961
src/messages.js:961: if (methodName && functionName.indexOf("." + methodName) !=
On 2012/05/24 02:15:18, Christian Plesner Hansen wrote:
> Is this equivalent to (methodName != this.getFunction().name)? If so that
might
> be a simpler way to express it.

Not equivalent; kept this, but fixed indexof -> lastIndexOf.

[Christian Plesner Hansen, 2012-05-24 10:46:49]

lgtm

[marja, 2012-05-24 10:56:56]

Thanks for the review!

yangguo: can you try again to commit this?

(I ran ia32.release.check which passed. Should I have ran something more?)

[Yang, 2012-05-24 11:06:14]

On 2012/05/24 10:56:56, marja wrote:
> Thanks for the review!
> 
> yangguo: can you try again to commit this?
> 
> (I ran ia32.release.check which passed. Should I have ran something more?)

Landed as r11652.