Currently, our debugger callbacks explicitly passes sourceID and position (line and column) info to the Inspector along with the DebuggerCallFrame. This info (the DebuggerCallFrame, sourceID, and position) is then stored in the Inspector's JavaScriptCallFrame. Technically, we can just get this info from the DebuggerCallFrame. There's no need to explicitly pass the sourceID and position info to the Inspector and have it stored in its JavaScriptCallFrame. For the current implementation, JSC is not getting line and column info from the stack yet. Instead the line and column number are explicitly passed in as arguments to the debug hooks. For now, we'll continue to use the debug hook line and column info. To do this, we'll store the values in the DebuggerCallFrame. At a later date, we should have the DebuggerCallFrame get the info directly from the CallFrame based on its bytecodeOffset. This refactoring is part of the work to move ScriptDebugServer functionality into JSC.
Created attachment 211403 [details] the patch. This patch has run the layout tests with no new failures.
Comment on attachment 211403 [details] the patch. Can't we simply kill debuggercallframe and rely on the stack walking instead? The removes (essentially) a duplicate type, and saves computing/storing line and column info unnecessarily.
(In reply to comment #2) > (From update of attachment 211403 [details]) > Can't we simply kill debuggercallframe and rely on the stack walking instead? The removes (essentially) a duplicate type, and saves computing/storing line and column info unnecessarily. One step at a time. I did this refactor because I found it to help organize the data in a way that allows me to move ScriptDebugServer functionality into JSC. I'm trying to stay focus on changing the Inspector to debugger interface first. I'll change the internal implementation of the debugger later after I've changed the interface to allow it. This is a stepping stone to get there.
> Can't we simply kill debuggercallframe and rely on the stack walking instead? I think DebuggerCallFrame has one good feature: it distinguishes standard data in CallFrame from data that is only present when a stack frame has been put in debugger mode. For example, I think the debugger should get a "fake" activation, and not the real thing. We could accomplish that through something like CallFrame::debuggerActivation(), and not have two types. But I think it might be valuable to have a separate type for this interface. DebuggerCallFrame implies that everything is in a debuggable state, which will diverge from normal state over time. It also clarifies who relies on debug info.
Comment on attachment 211403 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=211403&action=review r=me > Source/JavaScriptCore/interpreter/Interpreter.cpp:659 > + DebuggerCallFrame debuggerCallFrame(callFrame, line, 0, exceptionValue); Why not provide a real column number here?
(In reply to comment #5) > (From update of attachment 211403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211403&action=review > > r=me > > > Source/JavaScriptCore/interpreter/Interpreter.cpp:659 > > + DebuggerCallFrame debuggerCallFrame(callFrame, line, 0, exceptionValue); > > Why not provide a real column number here? I was just keeping parity with the pre-existing code, but it should be easy to provide a real column number there. I'll change it and run it thru the layout tests again.
Thanks for the review. Added the change to use the real column number in Interpreter::unwind(). Passes layout tests with no new failures. Landed in r155622: <http://trac.webkit.org/r155622>.