Bug 58139 - Web Inspector: No headers information in network panel for downloads.
Summary: Web Inspector: No headers information in network panel for downloads.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-08 05:53 PDT by Vsevolod Vlasov
Modified: 2011-04-14 19:40 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.13 KB, patch)
2011-04-08 06:15 PDT, Vsevolod Vlasov
pfeldman: review-
Details | Formatted Diff | Diff
Patch (6.31 KB, patch)
2011-04-08 08:24 PDT, Vsevolod Vlasov
pfeldman: review-
Details | Formatted Diff | Diff
Patch with review fixes (8.02 KB, patch)
2011-04-12 14:54 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch with fix (8.25 KB, patch)
2011-04-13 11:37 PDT, Vsevolod Vlasov
pfeldman: review-
Details | Formatted Diff | Diff
Patch (10.53 KB, patch)
2011-04-14 06:00 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-04-08 05:53:00 PDT
No headers information in network panel for downloads.
Comment 1 Vsevolod Vlasov 2011-04-08 06:15:30 PDT
Created attachment 88811 [details]
Patch
Comment 2 Pavel Feldman 2011-04-08 06:45:43 PDT
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
Comment 3 Vsevolod Vlasov 2011-04-08 08:24:14 PDT
Created attachment 88827 [details]
Patch
Comment 4 Pavel Feldman 2011-04-11 22:03:13 PDT
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
Comment 5 Vsevolod Vlasov 2011-04-12 14:54:05 PDT
Created attachment 89277 [details]
Patch with review fixes

Fixed nits
Comment 6 Yury Semikhatsky 2011-04-12 23:43:07 PDT
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.
Comment 7 Vsevolod Vlasov 2011-04-13 11:36:36 PDT
(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.
Comment 8 Vsevolod Vlasov 2011-04-13 11:37:47 PDT
Created attachment 89414 [details]
Patch with fix
Comment 9 Pavel Feldman 2011-04-14 03:55:52 PDT
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.
Comment 10 Vsevolod Vlasov 2011-04-14 06:00:19 PDT
Created attachment 89568 [details]
Patch

Done.
Comment 11 WebKit Commit Bot 2011-04-14 19:40:36 PDT
Comment on attachment 89568 [details]
Patch

Clearing flags on attachment: 89568

Committed r83933: <http://trac.webkit.org/changeset/83933>
Comment 12 WebKit Commit Bot 2011-04-14 19:40:41 PDT
All reviewed patches have been landed.  Closing bug.