| Summary: | Web Inspector: implement pausing on event listeners (back-end part) | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Pavel Podivilov
2010-09-27 08:32:55 PDT
Created attachment 68917 [details]
Proposed patch.
Comment on attachment 68917 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=68917&action=review > WebCore/bindings/js/ScriptDebugServer.cpp:@ > void ScriptDebugServer::pause() Lets be verbose here and call it "setPauseOnNextStatement". > WebCore/dom/Node.cpp:2619 > + InspectorController::instrumentWillDispatchEvent(document(), *event); Could you move timeline agent call below into instrumentWillDispatch? > WebCore/dom/Node.cpp:2712 > + InspectorController::instrumentDidDispatchEvent(document()); ditto Just "pauseOnNextStatement" would be better I think. A "set" prefix implies an argument. (In reply to comment #3) > Just "pauseOnNextStatement" would be better I think. A "set" prefix implies an argument. I meant to say "combine pause and cancelPause into one setter, with argument". Created attachment 69336 [details]
Comments addressed.
Comment on attachment 69336 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=69336&action=review > WebCore/inspector/InspectorDebuggerAgent.cpp:-319 > - if (!s_debuggerAgentOnBreakpoint) Why are you deleting this code? > WebCore/inspector/InspectorDebuggerAgent.h:70 > + void schedulePauseOnNextStatement(DebuggerEventType type, PassRefPtr<InspectorValue> data); setPauseOnNextStatement? > WebCore/inspector/InspectorInstrumentation.cpp:159 > + timelineAgentsStack().append(std::make_pair(s_callDepth, 0)); Looks like too much code... Will we need to do it for every will/did pair? (In reply to comment #6) > (From update of attachment 69336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69336&action=review > > > WebCore/inspector/InspectorDebuggerAgent.cpp:-319 > > - if (!s_debuggerAgentOnBreakpoint) > > Why are you deleting this code? This code was used to prevent accessing InspectorDebuggerAgent fields if the object was deleted. I've moved the code to didContinue method which is called only if InspectorDebuggerAgent was not deleted. > > > WebCore/inspector/InspectorDebuggerAgent.h:70 > > + void schedulePauseOnNextStatement(DebuggerEventType type, PassRefPtr<InspectorValue> data); > > setPauseOnNextStatement? schedulePauseOnNextStatement and cancelPauseOnNextStatement have different signatures, so we can't make a single method. And schedulePauseOnNextStatement alone doesn't look like a setter. > > > WebCore/inspector/InspectorInstrumentation.cpp:159 > > + timelineAgentsStack().append(std::make_pair(s_callDepth, 0)); > > Looks like too much code... Will we need to do it for every will/did pair? Yep, we don't actually need this code here. I'll move it below the frontend and timelineAgent checks. Created attachment 69343 [details]
Patch.
Comment on attachment 69343 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69343&action=review > WebCore/inspector/InspectorController.cpp:2140 > +void InspectorController::instrumentWillDispatchEvent(const Event& event) > +{ > +#if ENABLE(JAVASCRIPT_DEBUGGER) > + if (m_debuggerAgent) { > + if (!m_eventNamesMap.contains(event.type())) > + return; > + RefPtr<InspectorObject> eventData = InspectorObject::create(); > + eventData->setString("type", "EventListener"); > + eventData->setString("eventName", event.type()); > + m_debuggerAgent->schedulePauseOnNextStatement(NativeBreakpointDebuggerEventType, eventData); > + } > +#endif > +} It should be a method of debugger agent and you should call it directly from InspectorInstrumentation. > WebCore/inspector/InspectorController.cpp:2148 > + > +void InspectorController::instrumentDidDispatchEvent() > +{ > +#if ENABLE(JAVASCRIPT_DEBUGGER) > + if (m_debuggerAgent) > + m_debuggerAgent->cancelPauseOnNextStatement(); > +#endif > +} ditto > WebCore/inspector/InspectorController.h:398 > + HashMap<unsigned int, String> m_nativeBreakpoints; > + HashMap<unsigned int, String> m_eventListenerBreakpoints; > + HashMap<String, unsigned int> m_eventNamesMap; Probably these maps also should be moved to debuggerAgent > WebCore/inspector/InspectorDebuggerAgent.cpp:161 > + schedulePauseOnNextStatement(JavaScriptPauseEventType, InspectorObject::create()); It'd be better directly call ScriptDebugServer::shared().setPauseOnNextStatement(true); > WebCore/inspector/InspectorInstrumentation.cpp:162 > + inspectorController->instrumentWillDispatchEvent(event); see comments about this method in InspectorController Comment on attachment 69343 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69343&action=review >> WebCore/inspector/InspectorDebuggerAgent.cpp:161 >> + schedulePauseOnNextStatement(JavaScriptPauseEventType, InspectorObject::create()); > > It'd be better directly call ScriptDebugServer::shared().setPauseOnNextStatement(true); Please just skip this comment. Attachment 69343 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4202028 Comment on attachment 69343 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69343&action=review >> WebCore/inspector/InspectorInstrumentation.cpp:162 >> + inspectorController->instrumentWillDispatchEvent(event); > > see comments about this method in InspectorController Could you rename this method so that it would better reflect the nature of the call? > WebCore/inspector/InspectorInstrumentation.cpp:180 > + TimelineAgentsStack& stack = timelineAgentsStack(); Extract method please. Also this is very, very complex. Could we return Page* from all *::will methods and call ::did with it? > WebCore/inspector/InspectorInstrumentation.cpp:184 > + stack.removeLast(); You should check depth before this call. Also, please add assertions here with depth expectations. Created attachment 69479 [details]
Patch.
Comment on attachment 69479 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69479&action=review > WebCore/inspector/InspectorController.cpp:1689 > + m_nativeBreakpoints.set(*breakpointId, "XHR"); Please extract this into a constant. > WebCore/inspector/InspectorController.h:396 > + HashMap<String, unsigned int> m_eventNamesMap; The map name should reflect what is stored in it. Now it's unclear what the values in this map are. Please rename. > WebCore/inspector/InspectorInstrumentation.cpp:78 > + while (!stack.isEmpty() && stack.last() >= s_minCallDepth) I really don't like this mess with stack of callstack depths and s_minCallDepth. The alternative approach we discussed offline may be better. > WebCore/inspector/InspectorInstrumentation.cpp:231 > + if (!inspectorController->shouldBreakOnXMLHttpRequest(url)) The logic is getting spread over both InspectorInstrumentation and InspectorController without a clear border. I still think that we should reconsider the role of InspectorInstrumentation and make it a thin interface to IC used by the rest of WebCore. > WebCore/inspector/InspectorTimelineAgent.h:80 > + int id() { return m_id; } This is not used, please remove. Attachment 69479 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4142042 Comment on attachment 68917 [details] Proposed patch. Cleared Pavel Feldman's review+ from obsolete attachment 68917 [details] so that this bug does not appear in http://webkit.org/pending-commit. Created attachment 69622 [details]
Patch.
Comment on attachment 69622 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69622&action=review > WebCore/inspector/InspectorTimelineAgent.h:80 > + int id() { return m_id; } Not all comments were addressed. Comment on attachment 69622 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69622&action=review > WebCore/inspector/InspectorInstrumentation.cpp:180 > + if (InspectorDebuggerAgent* debuggerAgent = inspectorController->m_debuggerAgent.get()) > + debuggerAgent->cancelPauseOnNextStatement(); what happens if we get two events, one inside the other? >> WebCore/inspector/InspectorTimelineAgent.h:80 >> + int id() { return m_id; } > > Not all comments were addressed. const? > WebCore/inspector/InspectorTimelineAgent.h:162 > + int m_id; const? (In reply to comment #18) > (From update of attachment 69622 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69622&action=review > > > WebCore/inspector/InspectorTimelineAgent.h:80 > > + int id() { return m_id; } > > Not all comments were addressed. id is used as a cookie to prevent calling didXXX method of wrong timeline agent instance. (In reply to comment #19) > (From update of attachment 69622 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69622&action=review > > > WebCore/inspector/InspectorInstrumentation.cpp:180 > > + if (InspectorDebuggerAgent* debuggerAgent = inspectorController->m_debuggerAgent.get()) > > + debuggerAgent->cancelPauseOnNextStatement(); > > what happens if we get two events, one inside the other? Could you please suggest an example? Committed r69014: <http://trac.webkit.org/changeset/69014> |