WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch.
(34.51 KB, patch)
2010-11-03 09:19 PDT
,
Pavel Podivilov
pfeldman
: review-
Details
Formatted Diff
Diff
Comments addressed.
(40.04 KB, patch)
2010-11-18 03:12 PST
,
Pavel Podivilov
no flags
Details
Formatted Diff
Diff
Patch.
(44.51 KB, patch)
2010-12-08 08:52 PST
,
Pavel Podivilov
no flags
Details
Formatted Diff
Diff
Patch.
(45.52 KB, patch)
2010-12-13 09:58 PST
,
Pavel Podivilov
yurys
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Podivilov
Comment 1
2010-11-02 11:49:52 PDT
Created
attachment 72709
[details]
Patch.
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
Created
attachment 76399
[details]
Patch.
Eric Seidel (no email)
Comment 11
2010-12-13 12:13:40 PST
Attachment 76399
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/7028066
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
Committed
r74103
: <
http://trac.webkit.org/changeset/74103
>
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.
Top of Page
Format For Printing
XML
Clone This Bug