Bug 46624 – Web Inspector: implement pausing on event listeners (back-end part)

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Podivilov]

Web Inspector: implement pausing on event listeners (back-end part)

[Pavel Podivilov]

Created attachment 68917 [details]
Proposed patch.

[Pavel Feldman]

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

[Timothy Hatcher]

Just "pauseOnNextStatement" would be better I think. A "set" prefix implies an argument.

[Pavel Feldman]

(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".

[Pavel Podivilov]

Created attachment 69336 [details]
Comments addressed.

[Pavel Feldman]

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?

[Pavel Podivilov]

(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.

[Pavel Podivilov]

Created attachment 69343 [details]
Patch.

[Ilya Tikhonovsky]

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

[Ilya Tikhonovsky]

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.

[WebKit Review Bot]

Attachment 69343 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4202028

[Pavel Feldman]

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.

[Pavel Podivilov]

Created attachment 69479 [details]
Patch.

[Yury Semikhatsky]

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.

[WebKit Review Bot]

Attachment 69479 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4142042

[Eric Seidel (no email)]

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.

[Pavel Podivilov]

Created attachment 69622 [details]
Patch.

[Yury Semikhatsky]

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.

[Ilya Tikhonovsky]

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?

[Pavel Podivilov]

(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.

[Pavel Podivilov]

(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?

[Pavel Podivilov]

Committed r69014: <http://trac.webkit.org/changeset/69014>