Web Inspector: native breakpoints aren't hit on navigation
Created attachment 72709 [details] Patch.
Comment on attachment 72709 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=72709&action=review I see two changes here: first makes front-end responsible for assigning breakpoint ids, second makes them survive commit load. Two questions: - is the first part really related to the fix, - i don't see where the breakpoint maps get into the state cookie which makes me think that it won't stop upon renderer (inspector controller) change. > WebCore/inspector/InspectorController.cpp:730 > + m_debuggerAgent->setPauseWhenAttached(); Should this be called in reuseFrontend? > WebCore/inspector/InspectorDOMAgent.cpp:782 > + m_breakpointToId.remove(breakpoint); clean up m_idToBreakpoint here as well?
(In reply to comment #2) > (From update of attachment 72709 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72709&action=review > > I see two changes here: first makes front-end responsible for assigning breakpoint ids, second makes them survive commit load. Two questions: > - is the first part really related to the fix, It is related, because without this change, closing front-end while breakpoints are restored, may result in inconsistent state. > - i don't see where the breakpoint maps get into the state cookie which makes me think that it won't stop upon renderer (inspector controller) change. Breakpoints are set by front-end while debugger is paused on the first script. > > > WebCore/inspector/InspectorController.cpp:730 > > + m_debuggerAgent->setPauseWhenAttached(); > > Should this be called in reuseFrontend? I think calling this in didCommitLoad should be enough. > > > WebCore/inspector/InspectorDOMAgent.cpp:782 > > + m_breakpointToId.remove(breakpoint); > > clean up m_idToBreakpoint here as well? Call to m_idToBreakpoint.take above removes the breakpoint from m_idToBreakpoint.
Created attachment 72829 [details] Patch. setNativeBreakpoint should return breakpointId as discussed offline.
Comment on attachment 72829 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=72829&action=review > WebCore/inspector/InspectorDOMAgent.cpp:761 > +void InspectorDOMAgent::setDOMBreakpoint(const String& breakpointId, long nodeId, long type) I like the way it used to be better. Dom agent can manage dom breakpoint handles (i.e. return then and delete by them). And if there already is a breakpoint of a type, we could return illegal handle / report error. > WebCore/inspector/InspectorDebuggerAgent.cpp:160 > + if (m_scheduledPauseEventType != type) This gets complex. Could you provide an example where we hit this guard? Setting r- until you provide explanation. > WebCore/inspector/front-end/DOMAgent.js:382 > + WebInspector.breakpointManager.setCanRestoreDOMBreakpoints(); I like the previous name more.
Created attachment 74222 [details] Comments addressed.
(In reply to comment #5) > (From update of attachment 72829 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72829&action=review > > > WebCore/inspector/InspectorDOMAgent.cpp:761 > > +void InspectorDOMAgent::setDOMBreakpoint(const String& breakpointId, long nodeId, long type) > > I like the way it used to be better. Dom agent can manage dom breakpoint handles (i.e. return then and delete by them). And if there already is a breakpoint of a type, we could return illegal handle / report error. Done. > > > WebCore/inspector/InspectorDebuggerAgent.cpp:160 > > + if (m_scheduledPauseEventType != type) > > This gets complex. Could you provide an example where we hit this guard? Setting r- until you provide explanation. The idea is to ignore schedulePause/cancelPause called from InspectorInstrumentation when "pause on attach" or "pause on next JS statement" is already scheduled to avoid unwanted pause cancellation. > > > WebCore/inspector/front-end/DOMAgent.js:382 > > + WebInspector.breakpointManager.setCanRestoreDOMBreakpoints(); > > I like the previous name more. setCanRestoreDOMBreakpoints -> documentSet.
Created attachment 75912 [details] Patch. Use backend cookie to restore sticky breakpoints on navigation instead of pausing debugger on attach.
Comment on attachment 75912 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=75912&action=review > WebCore/inspector/InspectorState.cpp:52 > + registerObject(stickyBreakpoints, (const char*)0, (const char*)0); Let's introduce a constant for (const char*)0 or better pass String() since the method accepts const String& > WebCore/inspector/front-end/BreakpointManager.js:69 > + var stickyId = this._setNativeBreakpoint("DOM", !disabled, { path: path, domEventType: domEventType }); Please extract constants for native breakpoint types. > WebCore/inspector/front-end/BreakpointManager.js:159 > + InspectorBackend.setNativeBreakpoint(data, this.stickyBreakpointRestored.bind(this, stickyId)); The callback method should be called didSetBreakpoint or didSetNativeBreakpoint. > WebCore/inspector/front-end/BreakpointManager.js:169 > + stickyBreakpointRestored: function(stickyId, backendId) Can you explain why we need two types of ids: sticky and backend ones? It seems that the front-end can assign ids for all the breakpoints when it loads them from settings/they are being added and the back-end can reuse the same ids. Also from the FE point of view stickyId is just an id for a breakpoint that is valid during current debug session only so it's not clear why it's sticky. > WebCore/inspector/front-end/BreakpointManager.js:172 > + if (!breakpoint || !breakpoint.enabled || breakpoint._backendId) { What's the scenario when you need to remove breakpoint due to the last condition(breakpoint._backendId)? > WebCore/inspector/front-end/BreakpointManager.js:261 > + if (breakpoint.type === "EventListener") Why not create these views after they have been restored on the back-end and the front-end receives stickyBreakpointRestored notification?
Created attachment 76399 [details] Patch.
Attachment 76399 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7028066
Comment on attachment 76399 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=76399&action=review > WebCore/inspector/front-end/BreakpointManager.js:36 > + this._stickyBreakpoints[projectId] = this._validateBreakpoints(breakpoints[projectId]); Why do we need this validation? No one else has access to the storage. > WebCore/inspector/front-end/BreakpointManager.js:193 > + if (breakpoint.type === WebInspector.BreakpointManager.NativeBreakpointTypes.EventListener) Why is there no branch for DOM breakpoints?
Committed r74103: <http://trac.webkit.org/changeset/74103>
(In reply to comment #12) > (From update of attachment 76399 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76399&action=review > > > WebCore/inspector/front-end/BreakpointManager.js:36 > > + this._stickyBreakpoints[projectId] = this._validateBreakpoints(breakpoints[projectId]); > > Why do we need this validation? No one else has access to the storage. Storage format may change in future. > > > WebCore/inspector/front-end/BreakpointManager.js:193 > > + if (breakpoint.type === WebInspector.BreakpointManager.NativeBreakpointTypes.EventListener) > > Why is there no branch for DOM breakpoints? DOM breakpoints are restored after pushing nodes by path to frontend.