Summary: | Web Inspector: No headers information in network panel for downloads. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vsevolod Vlasov <vsevik> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, commit-queue, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Vsevolod Vlasov
2011-04-08 05:53:00 PDT
Created attachment 88811 [details]
Patch
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 Created attachment 88827 [details]
Patch
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 Created attachment 89277 [details]
Patch with review fixes
Fixed nits
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. (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. Created attachment 89414 [details]
Patch with fix
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. Created attachment 89568 [details]
Patch
Done.
Comment on attachment 89568 [details] Patch Clearing flags on attachment: 89568 Committed r83933: <http://trac.webkit.org/changeset/83933> All reviewed patches have been landed. Closing bug. |