Bug 121796

(In reply to comment #0) Some typos ... > 1. Extract the JSC debug hook callbacks into a JSC::DebuggerEventHandler interface. The JSC::Debugger not privately implements this interface. "not" ==> "now" > 5. Reimplement WebKit/mac's WebScriptDebugger in terms based on the new public JSC::Debugger interface (i.e. no use of the JSC debug hooks). Reimplement WebKit/mac's WebScriptDebugger based on the new JSC::Debugger interface (i.e. no use of the JSC debug hooks).

Created attachment 212373 [details] the patch.

Comment on attachment 212373 [details] the patch. Need to remove JavaScriptCallFrame.cpp from other build systems as well.

Created attachment 212375 [details] Removed JavaScriptCallFrame.cpp from other build system files.

Comment on attachment 212373 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212373&action=review > Source/JavaScriptCore/ChangeLog:16 > + 3. Move WebCore::JavaScriptCallFrame functionality into a > + Debugger::FrameVisitor class. The JavaScriptCallFrame is still > + necessary because it is published as a JS object to the Inspector. In If there's an object model published by the Web Inspector, that model belongs in WebCore's Web Inspector code, and not in JavaScriptCore. > Source/JavaScriptCore/debugger/Breakpoint.h:36 > +namespace WebCore { The WebCore namespace doesn't belong in JavaScriptCore. > Source/JavaScriptCore/debugger/Breakpoint.h:42 > +typedef enum { > + ScriptBreakpointActionTypeLog, > + ScriptBreakpointActionTypeEvaluate, > + ScriptBreakpointActionTypeSound > +} ScriptBreakpointActionType; Why is everything prefixed with "Script"? > Source/JavaScriptCore/debugger/Breakpoint.h:55 > +struct ScriptBreakpoint { Let's not double the number of objects used to represent a breakpoint. One should be enough. > Source/JavaScriptCore/debugger/Debugger.cpp:48 > + DebuggerEventHandler* m_debugger; We call client interfaces like this "*Client". So, "DebuggerClient". > Source/JavaScriptCore/debugger/Debugger.h:42 > +class DebuggerEventHandler { We don't need (or want) an abstract interface for something that's implemented by exactly one subclass. Unnecessary abstraction makes the code harder to read. > Source/JavaScriptCore/debugger/DebuggerFrameVisitor.cpp:28 > +#if ENABLE(JAVASCRIPT_DEBUGGER) We don't need an enable flag for this. > Source/JavaScriptCore/debugger/DebuggerFrameVisitor.cpp:30 > +#include "JavaScriptCallFrame.h" Files should be named after the classes they contain. So, DebuggerFrameVisitor is not a good name for this file. > Source/JavaScriptCore/debugger/DebuggerFrameVisitor.cpp:46 > +JavaScriptCallFrame::JavaScriptCallFrame(const DebuggerCallFrame& debuggerCallFrame, PassRefPtr<JavaScriptCallFrame> caller) Let's not double the number of objects used to represent the JavaScript stack.

Comment on attachment 212375 [details] Removed JavaScriptCallFrame.cpp from other build system files. Attachment 212375 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1903249

Comment on attachment 212375 [details] Removed JavaScriptCallFrame.cpp from other build system files. Attachment 212375 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2105068

Comment on attachment 212375 [details] Removed JavaScriptCallFrame.cpp from other build system files. Attachment 212375 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1903250

Comment on attachment 212373 [details] the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=212373&action=review >> Source/JavaScriptCore/ChangeLog:16 >> + necessary because it is published as a JS object to the Inspector. In > > If there's an object model published by the Web Inspector, that model belongs in WebCore's Web Inspector code, and not in JavaScriptCore. WebCore::JavaScriptCallFrame still resides in WebCore. The only differences is that it is now implemented based on JSC::Debugger::FrameVisitor. When I said "move", I meant that JSC::Debugger::FrameVisitor will now provide the functionality, and WebCore::JavaScriptCallFrame will implement it by inheriting it from JSC::Debugger::FrameVisitor. >> Source/JavaScriptCore/debugger/Breakpoint.h:36 >> +namespace JSC { > > The WebCore namespace doesn't belong in JavaScriptCore. It isn't in JavaScriptCore. Breakpoint is implemented based on ScriptBreakpoint. So, I "svn cp" the file so that we get to keep the change history. The change above modifies it so that we're using the JSC name space. I've not added any reference to WebCore in JavaScriptCore. >> Source/JavaScriptCore/debugger/Breakpoint.h:42 >> + Breakpoint() > > Why is everything prefixed with "Script"? It isn't. Look again. I'm removing the prefix, not adding it. >> Source/JavaScriptCore/debugger/Breakpoint.h:55 >> > > Let's not double the number of objects used to represent a breakpoint. One should be enough. Can you please clarify what you mean by this? In my implementation, there is only one representation of a breakpoint i.e. JSC::Breakpoint. On WebCore side, they have a ScriptBreakpoint that is used as a collection of paramteters to pass to the debugger. I could rename WebCore::ScriptBreakpoint to WebCore::ScriptBreakpointParamaters if you prefer that. >> Source/JavaScriptCore/debugger/Debugger.h:42 >> +class DebuggerEventHandler { > > We don't need (or want) an abstract interface for something that's implemented by exactly one subclass. Unnecessary abstraction makes the code harder to read. This interface is meant to be obsoleted when we implement the new debugger. I isolated it into a separate interface like this so that these APIS can be public only to the clients that uses it (i.e. old debug hook code that will eventually get replaced), while keeping it private from that the ScriptDebugServer (and its subclasses) and WebScriptDebugger classes. This allows me to use the C++ compiler to ensure that no mistakes were made where the old debug hook interfaces get accidentally leaked out and used in clients who aren't meant to use it. If you object to this approach to encapsulating the old functionality, I can just make them public in JSC::Debugger again. >> Source/JavaScriptCore/debugger/DebuggerFrameVisitor.cpp:28 >> > > We don't need an enable flag for this. Yes. I removed this enable flag, not added it. Please look again. >> Source/JavaScriptCore/debugger/DebuggerFrameVisitor.cpp:30 >> > > Files should be named after the classes they contain. So, DebuggerFrameVisitor is not a good name for this file. The class they contain is JSC::Debugger::FrameVisitor. What would you recommend as a name if not DebuggerFrameVisitor.cpp?

Comment on attachment 212375 [details] Removed JavaScriptCallFrame.cpp from other build system files. Attachment 212375 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/2110046

Comment on attachment 212375 [details] Removed JavaScriptCallFrame.cpp from other build system files. Will fix the build problems. Shouldn't have svn up'ed.

Created attachment 212383 [details] Fix build issues. The culprit were inline functions, not the svn up. This only manifests on release builds (the issue is hidden on debug builds). It's now fixe. Uploading a new patch to let the EWS bots give a go at it. Geoff's feedback has not been addressed yet.

Comment on attachment 212383 [details] Fix build issues. View in context: https://bugs.webkit.org/attachment.cgi?id=212383&action=review Based on our talk in-person, here are the refactoring steps we want: (1) Change ScriptDebugServer in WebCore to use DebuggerCallFrame instead of JavaScriptCallFrame: (1a) Add JSLocking to DebuggerCallFrame (1b) Rewrite member functions to use DebuggerCallFrame directly (1c) Rewrite ScriptDebugServer::dispatchDidPause to create a new JavaScriptCallFrame for each callback. Test to verify that the front-end doesn't depend on object identity here. (3) Move some of ScriptDebugServer into JavaScriptCore's JSC::Debugger. Split out ScriptBreakpointAction from the Breakpoint interface JavaScriptCore will expose. (4) Rewrite WebScriptDebugger in terms of new JavaScript debugger API. (5) Make VM debugger callbacks non-virtual. (6) Change DebuggerCallFrame into a ref-counted object that automatically nullifies itself after a callback. > Source/JavaScriptCore/debugger/Debugger.h:58 > +class JS_EXPORT_PRIVATE Debugger : private DebuggerEventHandler { Let's make this one class -- Debugger. Our legacy VM callbacks can be non-virtual public functions with a comment above them saying "These are the deprecated callbacks we get from the VM. Clients of Debugger should not call these functions." Non-virtual is important because it specifies that new clients, with overriding behavior, are not allowed.

Status update: (In reply to comment #13) > (From update of attachment 212383 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212383&action=review > > Based on our talk in-person, here are the refactoring steps we want: > > (1) Change ScriptDebugServer in WebCore to use DebuggerCallFrame instead of JavaScriptCallFrame: > (1a) Add JSLocking to DebuggerCallFrame > (1b) Rewrite member functions to use DebuggerCallFrame directly > (1c) Rewrite ScriptDebugServer::dispatchDidPause to create a new JavaScriptCallFrame for each callback. Test to verify that the front-end doesn't depend on object identity here. This has already been in implemented in r156936: <http://trac.webkit.org/r156936> for https://bugs.webkit.org/show_bug.cgi?id=121969. > (3) Move some of ScriptDebugServer into JavaScriptCore's JSC::Debugger. Split out ScriptBreakpointAction from the Breakpoint interface JavaScriptCore will expose. > (4) Rewrite WebScriptDebugger in terms of new JavaScript debugger API. > (5) Make VM debugger callbacks non-virtual. Will do these as part of this bug (https://bugs.webkit.org/show_bug.cgi?id=121796). > (6) Change DebuggerCallFrame into a ref-counted object that automatically nullifies itself after a callback. This has already been in implemented in r156936: <http://trac.webkit.org/r156936> for https://bugs.webkit.org/show_bug.cgi?id=121969.

Created attachment 216342 [details] candidate patch with the latest goodness. Not sure this is ready for review yet, but I want to upload it so that I can try it on the EWS bots plus have a prettier view of the diff.

Attachment 216342 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/debugger/Breakpoint.h', u'Source/JavaScriptCore/debugger/Debugger.cpp', u'Source/JavaScriptCore/debugger/Debugger.h', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.h', u'Source/JavaScriptCore/debugger/DebuggerPrimitives.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/BreakpointID.h', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.h', u'Source/WebCore/bindings/js/SourceID.h', u'Source/WebCore/bindings/js/WorkerScriptDebugServer.cpp', u'Source/WebCore/bindings/js/WorkerScriptDebugServer.h', u'Source/WebCore/inspector/InspectorDebuggerAgent.cpp', u'Source/WebCore/inspector/InspectorDebuggerAgent.h', u'Source/WebCore/inspector/ScriptDebugListener.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebScriptDebugger.h', u'Source/WebKit/mac/WebView/WebScriptDebugger.mm']" exit_code: 1 Source/JavaScriptCore/ChangeLog:23: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 216342 [details] candidate patch with the latest goodness. Attachment 216342 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/22718328

Comment on attachment 216342 [details] candidate patch with the latest goodness. I forgot to update all the other build systems with the new files. Will upload another patch shortly.

Created attachment 216344 [details] patch 2: updated build files, fixed style issue.

Created attachment 216356 [details] patch 3: fixed some issue revealed by a Windows build. I think it's ready for a review now.

Comment on attachment 216356 [details] patch 3: fixed some issue revealed by a Windows build. View in context: https://bugs.webkit.org/attachment.cgi?id=216356&action=review r=me This is a reasonable first step. I'm not sure the division of responsibilities is perfect yet. It seems odd for a JSC-level thing to be in charge of defining ReasonForPause, and implementing all the reasons you might pause. That constrains the design of the debugger UI to the set of reasons the VM builds in. A more flexible design would just build into JSC the ability to pause -- and the debugger UI layer could invent as many reasons as it liked. > Source/JavaScriptCore/debugger/DebuggerPrimitives.h:37 > +typedef intptr_t SourceID; > +static const SourceID noSourceID = 0; > + > +typedef int32_t BreakpointID; > +static const BreakpointID noBreakpointID = 0; Why intptr_t for one and int32_t for another? Seems like you actually want size_t for both.

Thanks for the review. (In reply to comment #21) > This is a reasonable first step. I'm not sure the division of responsibilities is perfect yet. It seems odd for a JSC-level thing to be in charge of defining ReasonForPause, and implementing all the reasons you might pause. That constrains the design of the debugger UI to the set of reasons the VM builds in. A more flexible design would just build into JSC the ability to pause -- and the debugger UI layer could invent as many reasons as it liked. Currently, the only real need for this is for the debugger UI layer to be able to tell that the pause is for an exception. I just added the others as a means of providing more info for now, but they may end up being unnecessary. We can tighten up or replace this design later once we have gained more experience with what this systems needs. > > Source/JavaScriptCore/debugger/DebuggerPrimitives.h:37 > > +typedef intptr_t SourceID; > > +static const SourceID noSourceID = 0; > > + > > +typedef int32_t BreakpointID; > > +static const BreakpointID noBreakpointID = 0; > > Why intptr_t for one and int32_t for another? Seems like you actually want size_t for both. Will fix.

Landed in r158937: <http://trac.webkit.org/r158937>.