Bug 58139 – Web Inspector: No headers information in network panel for downloads.

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

[Vsevolod Vlasov]

No headers information in network panel for downloads.

[Vsevolod Vlasov]

Created attachment 88811 [details]
Patch

[Pavel Feldman]

Comment on attachment 88811 [details]
Patch

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

> Source/WebCore/loader/MainResourceLoader.cpp:260
> +        frameLoader()->notifier()->dispatchDidReceiveResponse(documentLoader(), identifier(), r);

InspectorInstrumentation::...

> Source/WebCore/loader/MainResourceLoader.cpp:273
> +        frameLoader()->notifier()->dispatchDidReceiveResponse(documentLoader(), identifier(), r);

ditto

[Vsevolod Vlasov]

Created attachment 88827 [details]
Patch

[Pavel Feldman]

Comment on attachment 88827 [details]
Patch

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

A couple of nits, otherwise looks good.

> LayoutTests/http/tests/inspector/network/download-expected.txt:1
> +http://127.0.0.1:8000/inspector/network/resources/download.zzz - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/inspector/network/resources/download.zzz, main document URL http://127.0.0.1:8000/inspector/network/download.html, http method GET> redirectResponse (null)

This looks platform specific.

> Source/WebCore/loader/MainResourceLoader.cpp:261
> +        InspectorInstrumentation::didReceiveResourceResponseButCanceled(m_frame.get(), documentLoader(), identifier(), r);

You should provide as much context into the signal as possible, in this case I would call it "continueWithPolicyDownload" or similar.

> Source/WebCore/loader/MainResourceLoader.cpp:274
> +        InspectorInstrumentation::didReceiveResourceResponseButCanceled(m_frame.get(), documentLoader(), identifier(), r);

ditto

[Vsevolod Vlasov]

Created attachment 89277 [details]
Patch with review fixes

Fixed nits

[Yury Semikhatsky]

Comment on attachment 89277 [details]
Patch with review fixes

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

> Source/WebCore/loader/MainResourceLoader.cpp:261
> +        InspectorInstrumentation::continueWithPolicyDownload(m_frame.get(), documentLoader(), identifier(), r);

Shouldn't we move this call a few lines down, closer to frameLoader()->client()->download call? Otherwise we call continueWithPolicyDownload while the loader reports an error.

[Vsevolod Vlasov]

(In reply to comment #6)
> (From update of attachment 89277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89277&action=review
> 
> > Source/WebCore/loader/MainResourceLoader.cpp:261
> > +        InspectorInstrumentation::continueWithPolicyDownload(m_frame.get(), documentLoader(), identifier(), r);
> 
> Shouldn't we move this call a few lines down, closer to frameLoader()->client()->download call? Otherwise we call continueWithPolicyDownload while the loader reports an error.

That's right, I have checked that it's enough to call it 3 lines later to correctly show downloads in inspector.

FYI: PolicyDownload is used for downloads in Safari.
In Chromium we use PolicyIgnore.
That's the reason to make two calls to InspectorInstrumentation.

[Vsevolod Vlasov]

Created attachment 89414 [details]
Patch with fix

[Pavel Feldman]

Comment on attachment 89414 [details]
Patch with fix

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

> Source/WebCore/inspector/InspectorInstrumentation.h:680
> +    InspectorInstrumentation::didReceiveResourceResponseButCanceled(frame, loader, identifier, r);

Instrumentation header should have no logic and bring all signals to the implementation as is.

[Vsevolod Vlasov]

Created attachment 89568 [details]
Patch

Done.

[WebKit Commit Bot]

Comment on attachment 89568 [details]
Patch

Clearing flags on attachment: 89568

Committed r83933: <http://trac.webkit.org/changeset/83933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.