Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timob-11653: expose error stack trace #3685

Merged
merged 5 commits into from Jan 29, 2013
Merged

Conversation

hieupham007
Copy link
Contributor

}
}

} else if (jsValue->IsString()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can also handle numbers as well?

@ayeung
Copy link
Contributor

ayeung commented Jan 9, 2013

If we want to have a line number property, we can probably just parse it from the stack trace. It looks like they have a standard format that won't change much:

http://code.google.com/p/v8/wiki/JavaScriptStackTraceApi

We may want to include a file name, and other properties as well, if we end up parsing it.

}
}

} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle the case where it's something like an array, 'null', or 'undefined'? If not, we probably don't want to do an else here. We might just want to do this for cases where this jsValueToJavaString() actually works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsValueToJavaString() is merely calling jsValue->ToString() with a null check. I've tested with all cases you mentioned. If its an array, or null, null is returned for the KrollException. If it's undefined, a valid KrollException is returned with the message being undefined. I think these are acceptable results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, looks good then.

@ayeung
Copy link
Contributor

ayeung commented Jan 10, 2013

Code reviewed. Please address comments.

@hieupham007
Copy link
Contributor Author

Code updated. Ready for re-review

@@ -0,0 +1,66 @@
/**
* Appcelerator Titanium Mobile
* Copyright (c) 2012 by Appcelerator, Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 2012-2013 now =)

@ayeung
Copy link
Contributor

ayeung commented Jan 11, 2013

Code reviewed. Besides the license change, Request Accepted

@ayeung
Copy link
Contributor

ayeung commented Jan 29, 2013

Code reviewed and functionally tested. Request Accepted

ayeung pushed a commit that referenced this pull request Jan 29, 2013
timob-11653: expose error stack trace
@ayeung ayeung merged commit cc8a51c into tidev:master Jan 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants