RESOLVED FIXED Bug 119575
Restoring use of StackIterator instead of Interpreter::getStacktrace()
https://bugs.webkit.org/show_bug.cgi?id=119575
Summary Restoring use of StackIterator instead of Interpreter::getStacktrace()
Mark Lam
Reported 2013-08-08 08:41:35 PDT
This change set reverts r153383 (https://bugs.webkit.org/show_bug.cgi?id=119141) by adding a numberOfFrames() method to the StackIterator class. This enables us to achieve the createScriptCallStack() semantics that the inspector expects. This fixes the issue in https://bugs.webkit.org/show_bug.cgi?id=119141, and allows us to pass the "plugins" layout tests. It is questionable whether it is appropriate for the inspector to expect those semantics, but that's a question for another day. This change set also makes Interpreter::getStackTrace() private again so that "unauthorized" code is not encouraged to call it. This results in "moving" some ErrorConstructor and NativeErrorConstructor functions under the Interpreter.
Attachments
the patch (12.46 KB, patch)
2013-08-08 08:52 PDT, Mark Lam
oliver: review+
updated patch: use "goto", and ErrorConstructor changes landed separately. (6.85 KB, patch)
2013-08-08 09:52 PDT, Mark Lam
no flags
Mark Lam
Comment 1 2013-08-08 08:52:16 PDT
Created attachment 208347 [details] the patch
Oliver Hunt
Comment 2 2013-08-08 09:01:14 PDT
Comment on attachment 208347 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review > Source/JavaScriptCore/ChangeLog:19 > + (JSC::StackIterator::goToNextFrame): Renamed from gotoNextFrame. goto is a single word so the old capitalization was correct > Source/JavaScriptCore/ChangeLog:32 > + (JSC::ErrorConstructor::getCallData): > + - Don't want ErrorConstructor to call Interpreter::getStackTrace() > + directly. So, we moved the helper functions into the Interpreter > + class. Haven't read the full patch yet, but if possible it would be nice to have changes that should not effect behavior moved into a separate bug, and landed earlier if possible
Oliver Hunt
Comment 3 2013-08-08 09:04:06 PDT
Comment on attachment 208347 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review > Source/JavaScriptCore/interpreter/Interpreter.h:229 > + static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*); > + static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*); > + static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*); > + static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*); > + I like this, but it seems like this can be a separate patch, r+ for anding it separately
Mark Lam
Comment 4 2013-08-08 09:05:26 PDT
(In reply to comment #2) > (From update of attachment 208347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review > > > Source/JavaScriptCore/ChangeLog:19 > > + (JSC::StackIterator::goToNextFrame): Renamed from gotoNextFrame. > > goto is a single word so the old capitalization was correct Hmmm ... not in English AFAIK, but I'm ok with sticking with "goto" if there are no objection from anyone else. (In reply to comment #3) > (From update of attachment 208347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review > > > Source/JavaScriptCore/interpreter/Interpreter.h:229 > > + static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*); > > + static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*); > > + static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*); > > + static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*); > > + > > I like this, but it seems like this can be a separate patch, r+ for anding it separately Will land this separately.
Oliver Hunt
Comment 5 2013-08-08 09:24:03 PDT
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 208347 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review > > > > > Source/JavaScriptCore/ChangeLog:19 > > > + (JSC::StackIterator::goToNextFrame): Renamed from gotoNextFrame. > > > > goto is a single word so the old capitalization was correct > > Hmmm ... not in English AFAIK, but I'm ok with sticking with "goto" if there are no objection from anyone else. goto doesn't get flagged as wrong by spellcheck or autocorrect :D > > (In reply to comment #3) > > (From update of attachment 208347 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208347&action=review > > > > > Source/JavaScriptCore/interpreter/Interpreter.h:229 > > > + static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*); > > > + static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*); > > > + static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*); > > > + static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*); > > > + > > > > I like this, but it seems like this can be a separate patch, r+ for anding it separately > > Will land this separately.
Mark Lam
Comment 6 2013-08-08 09:52:32 PDT
Created attachment 208353 [details] updated patch: use "goto", and ErrorConstructor changes landed separately.
Mark Lam
Comment 7 2013-08-08 09:57:15 PDT
Thanks for the review. Revised patch landed in r153825: <http://trac.webkit.org/r153825>.
Note You need to log in before you can comment on or make changes to this bug.