Fixes bug with v8::StackTrace for non-zero script line offsets....

Fixes bug with v8::StackTrace for non-zero script line offsets.
GetScriptLineNumber() in handles.cc already adjusted by the script line offset. The implementation of CaptureCurrentStackTrace in top.cc did not account for this.

Landed with http://codereview.chromium.org/2049004.

Committed: http://code.google.com/p/v8/source/detail?r=4623

[jaimeyap, 2010-05-06 19:33:02]

I was implementing WebKit changes to use this API and found a difference between
GetScriptLineNumber() from handles.cc and my original helper function.

This should fix this and it ammends the test case to cover it.

[Søren Thygesen Gjesse, 2010-05-07 07:57:05]

http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9002#newcode396
src/top.cc:396: int line_number = GetScriptLineNumber(Handle<Script>(script),
position);
How about adding a function GetScriptLocalLineNumber which does not take the
offset into account?

http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
Any reason for changing this?

I just looked at the code, and we have at least three ways of checking this

  if (!fun_name->IsString())
  if (fun_name->IsUndefined())
  if (fun_name->ToBoolean()->IsFalse())

I think we should be consistent, so I suggest adding a function called something
likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to JSFunction and use
that in all places where the inferred name is returned if no actual name is
found.

http://codereview.chromium.org/1985004/diff/2002/9001
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9001#newcode9691
test/cctest/test-api.cc:9691: v8::ScriptOrigin detailed_origin(origin,
line_offset);
How about adding a test with a column offset as well?

[mnaganov (inactive), 2010-05-07 11:39:51]

http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
On 2010/05/07 07:57:06, Søren Gjesse wrote:
> Any reason for changing this?
> 
> I just looked at the code, and we have at least three ways of checking this
> 
>   if (!fun_name->IsString())
>   if (fun_name->IsUndefined())
>   if (fun_name->ToBoolean()->IsFalse())
> 
> I think we should be consistent, so I suggest adding a function called
something
> likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to JSFunction and use
> that in all places where the inferred name is returned if no actual name is
> found.

I can do this as a separate change. But I'm too not quite understanding, why use
such extravagant "ToBoolean()->IsFalse()" check. Using
"shared->name()->IsUndefined()" is more straightforward.

[jaimeyap, 2010-05-07 14:49:42]

Added test case for non-zero column offset and sure enough fixed another issue.
I will be sure to be better about proper test coverage in future patches.

http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9002#newcode396
src/top.cc:396: int line_number = GetScriptLineNumber(Handle<Script>(script),
position);
On 2010/05/07 07:57:06, Søren Gjesse wrote:
> How about adding a function GetScriptLocalLineNumber which does not take the
> offset into account?

Done.

http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
On 2010/05/07 11:39:52, Michail Naganov wrote:
> On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > Any reason for changing this?
> > 
> > I just looked at the code, and we have at least three ways of checking this
> > 
> >   if (!fun_name->IsString())
> >   if (fun_name->IsUndefined())
> >   if (fun_name->ToBoolean()->IsFalse())
> > 
> > I think we should be consistent, so I suggest adding a function called
> something
> > likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to JSFunction and
use
> > that in all places where the inferred name is returned if no actual name is
> > found.
> 
> I can do this as a separate change. But I'm too not quite understanding, why
use
> such extravagant "ToBoolean()->IsFalse()" check. Using
> "shared->name()->IsUndefined()" is more straightforward.


Because it is not undefined. name and inferred name get set to the empty string
initially :(. It tripped me up too.

The empty string in javascript boolean coerces to false. In fact, any whitespace
only string boolean coerces to false.

("" == false) == true;
("\r\n" == false) == true;
("    " == false) == true;

Gross eh?

Also, I agree with Michail that we should normalize all the getters for a
function name in a separate change. This one should go in soon since it fixed
API brokenness.

[jaimeyap, 2010-05-07 14:52:32]

http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9002#newcode396
src/top.cc:396: int line_number = GetScriptLineNumber(Handle<Script>(script),
position);
On 2010/05/07 14:49:42, jaimeyap wrote:
> On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > How about adding a function GetScriptLocalLineNumber which does not take the
> > offset into account?
> 
> Done.

Clicked 'Done' and then changed my mind. See Michail's and my posts regarding
doing this in a separate patch.

[mnaganov (inactive), 2010-05-07 15:25:25]

http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
On 2010/05/07 14:49:42, jaimeyap wrote:
> On 2010/05/07 11:39:52, Michail Naganov wrote:
> > On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > > Any reason for changing this?
> > > 
> > > I just looked at the code, and we have at least three ways of checking
this
> > > 
> > >   if (!fun_name->IsString())
> > >   if (fun_name->IsUndefined())
> > >   if (fun_name->ToBoolean()->IsFalse())
> > > 
> > > I think we should be consistent, so I suggest adding a function called
> > something
> > > likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to JSFunction and
> use
> > > that in all places where the inferred name is returned if no actual name
is
> > > found.
> > 
> > I can do this as a separate change. But I'm too not quite understanding, why
> use
> > such extravagant "ToBoolean()->IsFalse()" check. Using
> > "shared->name()->IsUndefined()" is more straightforward.
> 
> 
> Because it is not undefined. name and inferred name get set to the empty
string
> initially :(. It tripped me up too.
> 
> The empty string in javascript boolean coerces to false. In fact, any
whitespace
> only string boolean coerces to false.
> 
> ("" == false) == true;
> ("\r\n" == false) == true;
> ("    " == false) == true;
> 
> Gross eh?
> 
> Also, I agree with Michail that we should normalize all the getters for a
> function name in a separate change. This one should go in soon since it fixed
> API brokenness.

Yeah, I knew that. I'm sorry, I looked into somebody's wrong code which checks
for undefined. Indeed, function's name is usually a string and it's empty if
it's anonymous. This is another point for introducing a function that returns a
"friendly" name -- there is already a piece of code that seem to do it in a
wrong way (Accessors::ScriptGetEvalFromFunctionName). I meant that converting a
string to boolean is an overkill, since parser already does a good job in
squeezing whitespaces, so function's name can't be e.g. "  " (two spaces), it
can only have zero length, if it's anonymous.

[jaimeyap, 2010-05-07 15:45:40]

http://codereview.chromium.org/1985004/diff/2002/9002
File src/top.cc (right):

http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
On 2010/05/07 15:25:27, Michail Naganov wrote:
> On 2010/05/07 14:49:42, jaimeyap wrote:
> > On 2010/05/07 11:39:52, Michail Naganov wrote:
> > > On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > > > Any reason for changing this?
> > > > 
> > > > I just looked at the code, and we have at least three ways of checking
> this
> > > > 
> > > >   if (!fun_name->IsString())
> > > >   if (fun_name->IsUndefined())
> > > >   if (fun_name->ToBoolean()->IsFalse())
> > > > 
> > > > I think we should be consistent, so I suggest adding a function called
> > > something
> > > > likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to JSFunction
and
> > use
> > > > that in all places where the inferred name is returned if no actual name
> is
> > > > found.
> > > 
> > > I can do this as a separate change. But I'm too not quite understanding,
why
> > use
> > > such extravagant "ToBoolean()->IsFalse()" check. Using
> > > "shared->name()->IsUndefined()" is more straightforward.
> > 
> > 
> > Because it is not undefined. name and inferred name get set to the empty
> string
> > initially :(. It tripped me up too.
> > 
> > The empty string in javascript boolean coerces to false. In fact, any
> whitespace
> > only string boolean coerces to false.
> > 
> > ("" == false) == true;
> > ("\r\n" == false) == true;
> > ("    " == false) == true;
> > 
> > Gross eh?
> > 
> > Also, I agree with Michail that we should normalize all the getters for a
> > function name in a separate change. This one should go in soon since it
fixed
> > API brokenness.
> 
> Yeah, I knew that. I'm sorry, I looked into somebody's wrong code which checks
> for undefined. Indeed, function's name is usually a string and it's empty if
> it's anonymous. This is another point for introducing a function that returns
a
> "friendly" name -- there is already a piece of code that seem to do it in a
> wrong way (Accessors::ScriptGetEvalFromFunctionName). I meant that converting
a
> string to boolean is an overkill, since parser already does a good job in
> squeezing whitespaces, so function's name can't be e.g. "  " (two spaces), it
> can only have zero length, if it's anonymous.

Im going to go with the principle of least surprise. If in JS I write:
var name = fun.name() || fun.inferred_name();

It is doing a proper boolean coercion. I would hate to be sensitive to a change
down the line in the default for name that boolean coerces but isn't exactly the
emtpy string :).

In any case, I think we should figure out the correct semantics for "friendly
name" when we normalize all the callers to use this shared getter (in a
subsequent patch).

[Søren Thygesen Gjesse, 2010-05-10 06:25:57]

On 2010/05/07 15:45:40, jaimeyap wrote:
> http://codereview.chromium.org/1985004/diff/2002/9002
> File src/top.cc (right):
> 
> http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
> src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
> On 2010/05/07 15:25:27, Michail Naganov wrote:
> > On 2010/05/07 14:49:42, jaimeyap wrote:
> > > On 2010/05/07 11:39:52, Michail Naganov wrote:
> > > > On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > > > > Any reason for changing this?
> > > > > 
> > > > > I just looked at the code, and we have at least three ways of checking
> > this
> > > > > 
> > > > >   if (!fun_name->IsString())
> > > > >   if (fun_name->IsUndefined())
> > > > >   if (fun_name->ToBoolean()->IsFalse())
> > > > > 
> > > > > I think we should be consistent, so I suggest adding a function called
> > > > something
> > > > > likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to JSFunction
> and
> > > use
> > > > > that in all places where the inferred name is returned if no actual
name
> > is
> > > > > found.
> > > > 
> > > > I can do this as a separate change. But I'm too not quite understanding,
> why
> > > use
> > > > such extravagant "ToBoolean()->IsFalse()" check. Using
> > > > "shared->name()->IsUndefined()" is more straightforward.
> > > 
> > > 
> > > Because it is not undefined. name and inferred name get set to the empty
> > string
> > > initially :(. It tripped me up too.
> > > 
> > > The empty string in javascript boolean coerces to false. In fact, any
> > whitespace
> > > only string boolean coerces to false.
> > > 
> > > ("" == false) == true;
> > > ("\r\n" == false) == true;
> > > ("    " == false) == true;
> > > 
> > > Gross eh?
> > > 
> > > Also, I agree with Michail that we should normalize all the getters for a
> > > function name in a separate change. This one should go in soon since it
> fixed
> > > API brokenness.
> > 
> > Yeah, I knew that. I'm sorry, I looked into somebody's wrong code which
checks
> > for undefined. Indeed, function's name is usually a string and it's empty if
> > it's anonymous. This is another point for introducing a function that
returns
> a
> > "friendly" name -- there is already a piece of code that seem to do it in a
> > wrong way (Accessors::ScriptGetEvalFromFunctionName). I meant that
converting
> a
> > string to boolean is an overkill, since parser already does a good job in
> > squeezing whitespaces, so function's name can't be e.g. "  " (two spaces),
it
> > can only have zero length, if it's anonymous.
> 
> Im going to go with the principle of least surprise. If in JS I write:
> var name = fun.name() || fun.inferred_name();
> 
> It is doing a proper boolean coercion. I would hate to be sensitive to a
change
> down the line in the default for name that boolean coerces but isn't exactly
the
> emtpy string :).
> 
> In any case, I think we should figure out the correct semantics for "friendly
> name" when we normalize all the callers to use this shared getter (in a
> subsequent patch).

ommitted. Closing issue.

[Søren Thygesen Gjesse, 2010-05-10 06:26:10]

On 2010/05/10 06:25:57, Søren Gjesse wrote:
> On 2010/05/07 15:45:40, jaimeyap wrote:
> > http://codereview.chromium.org/1985004/diff/2002/9002
> > File src/top.cc (right):
> > 
> > http://codereview.chromium.org/1985004/diff/2002/9002#newcode422
> > src/top.cc:422: if (fun_name->ToBoolean()->IsFalse()) {
> > On 2010/05/07 15:25:27, Michail Naganov wrote:
> > > On 2010/05/07 14:49:42, jaimeyap wrote:
> > > > On 2010/05/07 11:39:52, Michail Naganov wrote:
> > > > > On 2010/05/07 07:57:06, Søren Gjesse wrote:
> > > > > > Any reason for changing this?
> > > > > > 
> > > > > > I just looked at the code, and we have at least three ways of
checking
> > > this
> > > > > > 
> > > > > >   if (!fun_name->IsString())
> > > > > >   if (fun_name->IsUndefined())
> > > > > >   if (fun_name->ToBoolean()->IsFalse())
> > > > > > 
> > > > > > I think we should be consistent, so I suggest adding a function
called
> > > > > something
> > > > > > likeGetNameOrInferredName/GetDisplayName/GetFriendlyName to
JSFunction
> > and
> > > > use
> > > > > > that in all places where the inferred name is returned if no actual
> name
> > > is
> > > > > > found.
> > > > > 
> > > > > I can do this as a separate change. But I'm too not quite
understanding,
> > why
> > > > use
> > > > > such extravagant "ToBoolean()->IsFalse()" check. Using
> > > > > "shared->name()->IsUndefined()" is more straightforward.
> > > > 
> > > > 
> > > > Because it is not undefined. name and inferred name get set to the empty
> > > string
> > > > initially :(. It tripped me up too.
> > > > 
> > > > The empty string in javascript boolean coerces to false. In fact, any
> > > whitespace
> > > > only string boolean coerces to false.
> > > > 
> > > > ("" == false) == true;
> > > > ("\r\n" == false) == true;
> > > > ("    " == false) == true;
> > > > 
> > > > Gross eh?
> > > > 
> > > > Also, I agree with Michail that we should normalize all the getters for
a
> > > > function name in a separate change. This one should go in soon since it
> > fixed
> > > > API brokenness.
> > > 
> > > Yeah, I knew that. I'm sorry, I looked into somebody's wrong code which
> checks
> > > for undefined. Indeed, function's name is usually a string and it's empty
if
> > > it's anonymous. This is another point for introducing a function that
> returns
> > a
> > > "friendly" name -- there is already a piece of code that seem to do it in
a
> > > wrong way (Accessors::ScriptGetEvalFromFunctionName). I meant that
> converting
> > a
> > > string to boolean is an overkill, since parser already does a good job in
> > > squeezing whitespaces, so function's name can't be e.g. "  " (two spaces),
> it
> > > can only have zero length, if it's anonymous.
> > 
> > Im going to go with the principle of least surprise. If in JS I write:
> > var name = fun.name() || fun.inferred_name();
> > 
> > It is doing a proper boolean coercion. I would hate to be sensitive to a
> change
> > down the line in the default for name that boolean coerces but isn't exactly
> the
> > emtpy string :).
> > 
> > In any case, I think we should figure out the correct semantics for
"friendly
> > name" when we normalize all the callers to use this shared getter (in a
> > subsequent patch).
> 
> ommitted. Closing issue.

omitted -> Committed