RESOLVED FIXED 48858
Web Inspector: native breakpoints aren't hit on navigation
https://bugs.webkit.org/show_bug.cgi?id=48858
Summary Web Inspector: native breakpoints aren't hit on navigation
Pavel Podivilov
Reported 2010-11-02 11:46:00 PDT
Web Inspector: native breakpoints aren't hit on navigation
Attachments
Patch. (34.92 KB, patch)
2010-11-02 11:49 PDT, Pavel Podivilov
no flags
Patch. (34.51 KB, patch)
2010-11-03 09:19 PDT, Pavel Podivilov
pfeldman: review-
Comments addressed. (40.04 KB, patch)
2010-11-18 03:12 PST, Pavel Podivilov
no flags
Patch. (44.51 KB, patch)
2010-12-08 08:52 PST, Pavel Podivilov
no flags
Patch. (45.52 KB, patch)
2010-12-13 09:58 PST, Pavel Podivilov
yurys: review+
Pavel Podivilov
Comment 1 2010-11-02 11:49:52 PDT
Pavel Feldman
Comment 2 2010-11-03 00:09:51 PDT
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?
Pavel Podivilov
Comment 3 2010-11-03 04:22:55 PDT
(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.
Pavel Podivilov
Comment 4 2010-11-03 09:19:30 PDT
Created attachment 72829 [details] Patch. setNativeBreakpoint should return breakpointId as discussed offline.
Pavel Feldman
Comment 5 2010-11-04 04:27:28 PDT
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.
Pavel Podivilov
Comment 6 2010-11-18 03:12:07 PST
Created attachment 74222 [details] Comments addressed.
Pavel Podivilov
Comment 7 2010-11-18 03:51:02 PST
(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.
Pavel Podivilov
Comment 8 2010-12-08 08:52:26 PST
Created attachment 75912 [details] Patch. Use backend cookie to restore sticky breakpoints on navigation instead of pausing debugger on attach.
Yury Semikhatsky
Comment 9 2010-12-09 03:07:02 PST
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?
Pavel Podivilov
Comment 10 2010-12-13 09:58:24 PST
Eric Seidel (no email)
Comment 11 2010-12-13 12:13:40 PST
Yury Semikhatsky
Comment 12 2010-12-14 04:25:49 PST
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?
Pavel Podivilov
Comment 13 2010-12-15 02:40:00 PST
Pavel Podivilov
Comment 14 2010-12-15 02:44:32 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.