Summary: | Network Panel: .swf file is always presented as loading by XHR | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | pdeng6 <pan.deng> | ||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||||
Severity: | Normal | CC: | apavlov, ap, buildbot, japhet, keishi, loislo, pfeldman, pmuellr, rniwa, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
pdeng6
2013-03-17 21:05:55 PDT
Created attachment 193486 [details]
swf in Network panel
Created attachment 193540 [details]
Patch
Root cause is that view of Resource type is different between CachedResource and Inspector. Currently, CachedRawResource are mapped to Inspector::XHRResource, while request from plugin is also another client of CachedRawResource, so that .swf and other resource initiated from plug-in are also mapped to XHRResource. To avoid change the type view of CachedResource, I suggest to restrict RawResource => XHRResource mapping by specifying explicit XHR target. thanks Pan Comment on attachment 193540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193540&action=review > Source/WebCore/inspector/InspectorPageAgent.cpp:322 > + if (const_cast<CachedResource&>(cachedResource).resourceRequest().targetType() == ResourceRequest::TargetIsXHR) Platform specific code is not acceptable here. TargetIsXHR is supposed to be used in chromium network stack and should not be used by inspector. Distinguishing between plugins and XHRs is tricky and I don't see a straightforward solution here. (In reply to comment #5) > Distinguishing between plugins and XHRs is tricky and I don't see a straightforward solution here. I found that InspectorPageAgent::XHRResource was set in m_resourcesData during didReceiveXHRResponse, which is always happen before didReceiveResponse, then we may eliminate the plugin according to that type. (there are similar story for checking type of InspectorPageAgent::ScriptResource). I will prepare a patch for that. :) thanks Pan Created attachment 193764 [details]
Patch
Comment on attachment 193764 [details]
Patch
This way OPTIONS preflight requests will get Other type for XHRs.
Created attachment 194208 [details]
Patch
(In reply to comment #8) > (From update of attachment 193764 [details]) > This way OPTIONS preflight requests will get Other type for XHRs. thank you very much! Right, preflight of xhr will get "Other" type in that way. As preflight is transparent to loader client, I didn't find a way to know who it works for in current DocumentThreadLoader :( So I added a method to know the type of ThreadableLoaderClient is XHR or not, and if it is true, tell ResourceAgent the type is "xhr" when receive the xhr preflight. a test is also attached to make sure xhr preflight type. thanks! Pan Comment on attachment 194208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194208&action=review You might want to discuss this with ap@ or japhet@. This would probably require adding new CachedResource type. I still think implementing this would be too tricky and the value added here is negligible. > Source/WebCore/loader/ThreadableLoaderClient.h:54 > + virtual bool isXMLHttpRequestClient() { return false; } This is a layering violation. Created attachment 195476 [details]
XHR category before fix, a bunch of non-xhr living there
Created attachment 195477 [details]
XHR category after fix, only two real xhr should live there
(In reply to comment #11) > (From update of attachment 194208 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194208&action=review > > You might want to discuss this with ap@ or japhet@. This would probably require adding new CachedResource type. I still think implementing this would be too tricky and the value added here is negligible. > > > Source/WebCore/loader/ThreadableLoaderClient.h:54 > > + virtual bool isXMLHttpRequestClient() { return false; } > > This is a layering violation. I think we would better fix this, I uploaded two snapshots to access Youtube, there are a bunch of non-XHR resources are classified as XHR type. I'd like to achieve a perfect patch solve this, but seems not easy. @ap and japhet, any comments about the loader part? thanks Pan Comment on attachment 194208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194208&action=review >>> Source/WebCore/loader/ThreadableLoaderClient.h:54 >>> + virtual bool isXMLHttpRequestClient() { return false; } >> >> This is a layering violation. > > I think we would better fix this, I uploaded two snapshots to access Youtube, there are a bunch of non-XHR resources are classified as XHR type. > I'd like to achieve a perfect patch solve this, but seems not easy. > @ap and japhet, any comments about the loader part? > > thanks > Pan I'm not entirely convinced this is a layering violation. At least, it's no more of a violation than isDocumentThreadableLoaderClient() on the line above it. I also don't think that this is a layering violation. But the name should be "isXMLHttpRequest", not "isXMLHttpRequestClient". However, the design mildly worries me. Inspector should ideally have a better abstraction, and not get to low level implementation details like ThreadableLoaderClient. (In reply to comment #16) > I also don't think that this is a layering violation. But the name should be "isXMLHttpRequest", not "isXMLHttpRequestClient". > > However, the design mildly worries me. Inspector should ideally have a better abstraction, and not get to low level implementation details like ThreadableLoaderClient. thanks for your comments :) Ideally, Inspector should instrument in real client XMLHttpRequest.cpp for detailed information for this, however, pre-flight is transparent, never pass through ThreadableClient. I'm thinking another solution may be, add a virtual method didReceivePreflightResponse to ThreadableLoaderClient, default do nothing, and private override it in XMLHttpRequest, InspectorInstrumentation will be there. Although only inspector works in the override function, XMLhttpRequest's client won't aware it as it's private. your comments? thanks Pan > pre-flight is transparent, never pass through ThreadableClient
XMLHttpRequest.cpp code may run on a secondary thread, or even in a separate process in the case of shared workers. All network loads that it does have to pass through ThreadableClient.
(In reply to comment #18) > > pre-flight is transparent, never pass through ThreadableClient > > XMLHttpRequest.cpp code may run on a secondary thread, or even in a separate process in the case of shared workers. All network loads that it does have to pass through ThreadableClient. thanks for your comments. I mean when ThreadableLoader(DocumentThreadableLoader) receives a pre-flight response, it won't tell ThreadableClient that have happened. right? I will upload a patch to demonstrate. :) thanks Pan Created attachment 196055 [details]
Patch
Comment on attachment 196055 [details] Patch Attachment 196055 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17336440 New failing tests: media/video-controls-fullscreen-volume.html Created attachment 196061 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
(In reply to comment #22) > Created an attachment (id=196061) [details] > Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2 we are innocent for this failure. Closing as invalid, as this bug pertains to the old inspector UI and/or its tests. Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk. Comment on attachment 196055 [details] Patch Cleared review? from attachment 196055 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). |