Bug 46086 – Web Inspector: implement pausing on XHR

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

[Pavel Podivilov]

Web Inspector: implement pausing on XHR

[Pavel Podivilov]

Created attachment 68078 [details]
Screenshot.

[Pavel Podivilov]

Created attachment 68079 [details]
Proposed patch.

[Pavel Feldman]

Comment on attachment 68079 [details]
Proposed patch.

r+ with nits. I'd actually suggest adding one more item into the native breakpoints list: "Any JavaScript statement" that is On by default. In addition to that, you change behavior of the Pause button respectively.

View in context: https://bugs.webkit.org/attachment.cgi?id=68079&action=review

> WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp:34
> +#include "InspectorController.h"

Please mind import order.

> WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp:117
> +    InspectorController::willSendXHR(xmlHttpRequest->scriptExecutionContext(), xmlHttpRequest->url());

I'd suggest calling it:

InspectorController::instrumentWillSendXmlHttpRequest

And all others will be 'instrument*'.

> WebCore/inspector/Inspector.idl:127
> +        [handler=Debug] void setPauseOnEventState(in unsigned int eventType, in boolean state);

pauseOnNextEvent ?

[Yury Semikhatsky]

Comment on attachment 68079 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=68079&action=review

>> WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp:117
>> +    InspectorController::willSendXHR(xmlHttpRequest->scriptExecutionContext(), xmlHttpRequest->url());
> 
> I'd suggest calling it:
> 
> InspectorController::instrumentWillSendXmlHttpRequest
> 
> And all others will be 'instrument*'.

It should be easy to support same functionality in JSC. There is already a bug about missing ScriptDebuggServer::breakProgram implementation in JSC which prevents this change from working in JSC but this functionality might deserve filing its own bug.

> WebCore/inspector/InspectorDebuggerAgent.h:99
> +    bool pauseOnEventState(unsigned eventType);

pauseOnEventState -> shouldPauseOnEvent

[Pavel Podivilov]

Created attachment 68246 [details]
Couple of renames.

[Pavel Podivilov]

Created attachment 68249 [details]
Couple of renames.

[Pavel Feldman]

Comment on attachment 68249 [details]
Couple of renames.

View in context: https://bugs.webkit.org/attachment.cgi?id=68249&action=review

> WebCore/inspector/InspectorController.cpp:2137
> +#if ENABLE(JAVASCRIPT_DEBUGGER)

I think you should inline willSendXHR here.

> WebCore/inspector/InspectorController.h:483
> +inline InspectorController* InspectorController::inspectorControllerForDocument(Document* document)

Can we employ static InspectorController counter here to speed up things?

> WebCore/inspector/InspectorDebuggerAgent.h:48
> +enum DebuggerEvent {

NativeBreakpointType ?

> WebCore/inspector/InspectorDebuggerAgent.h:83
> +    void willSendXHR(const String& url);

willSendXHR does not look good as a part of debugger agent.

[Joseph Pecoraro]

Comment on attachment 68249 [details]
Couple of renames.

View in context: https://bugs.webkit.org/attachment.cgi?id=68249&action=review

> WebCore/inspector/InspectorController.cpp:2140
> +#endif

Might need an #else with UNUSED_PARAM(url) here.

> WebCore/inspector/front-end/BreakpointManager.js:232
>  \ No newline at end of file

We do want newlines. =)

[Pavel Podivilov]

Created attachment 68358 [details]
Proposed patch.

[Pavel Podivilov]

Created attachment 68359 [details]
Proposed patch.

[Pavel Feldman]

Comment on attachment 68359 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=68359&action=review

> WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp:117
> +    InspectorController::instrumentWillSendXMLHttpRequest(xmlHttpRequest->scriptExecutionContext(), xmlHttpRequest->url());

Please guard this with #ifdef

> WebCore/inspector/Inspector.idl:140
> +        [handler=Controller] void setNativeBreakpoint(in unsigned int type, in Object breakpoint, out unsigned int breakpointId);

It is unclear how to specify 'breakpoint' parameter format. Why does it have url property? Should we merge 'type' into 'breakpoint' instead? Like

breakpoint: { type: "XHR", condition: { url : "foo" } }

> WebCore/inspector/InspectorController.h:492
> +    ASSERT(document);

Counter?

[Pavel Podivilov]

Created attachment 68362 [details]
Proposed patch.

[Pavel Podivilov]

(In reply to comment #11)
> (From update of attachment 68359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68359&action=review
> 
> > WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp:117
> > +    InspectorController::instrumentWillSendXMLHttpRequest(xmlHttpRequest->scriptExecutionContext(), xmlHttpRequest->url());
> 
> Please guard this with #ifdef
With disabled inspector controller, this function wouldn't be called.

> 
> > WebCore/inspector/Inspector.idl:140
> > +        [handler=Controller] void setNativeBreakpoint(in unsigned int type, in Object breakpoint, out unsigned int breakpointId);
> 
> It is unclear how to specify 'breakpoint' parameter format. Why does it have url property? Should we merge 'type' into 'breakpoint' instead? Like
> 
> breakpoint: { type: "XHR", condition: { url : "foo" } }
done

> 
> > WebCore/inspector/InspectorController.h:492
> > +    ASSERT(document);
> 
> Counter?
done

[Pavel Feldman]

Comment on attachment 68362 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=68362&action=review

> WebCore/inspector/InspectorController.cpp:156
> +unsigned InspectorController::s_inspectorControllerCount = 0;

I don't see it incrementing.

> WebCore/inspector/InspectorController.cpp:1683
> +        RefPtr<InspectorObject> condition = breakpoint->getObject("condition");

Nit: condition should be optional.

> WebCore/inspector/InspectorController.cpp:1687
> +        if (!condition->getString("url", &url))

Same here, just stop on any.

> WebCore/inspector/InspectorController.h:492
> +inline InspectorController* InspectorController::inspectorControllerForDocument(Document* document)

It is worth checking counter here.

[WebKit Commit Bot]

Comment on attachment 68362 [details]
Proposed patch.

Clearing flags on attachment: 68362

Committed r68049: <http://trac.webkit.org/changeset/68049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.