[Webkit-unassigned] [Bug 61205] Web Inspector: Cache XHR content in backend, do not use initialContentSet for XHRs.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61205


Vsevolod Vlasov <vsevik at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94405|0                           |1
        is obsolete|                            |
  Attachment #94770|                            |review?, commit-queue?
               Flag|                            |




--- Comment #5 from Vsevolod Vlasov <vsevik at chromium.org>  2011-05-25 05:32:53 PST ---
Created an attachment (id=94770)
 --> (https://bugs.webkit.org/attachment.cgi?id=94770&action=review)
Patch with fixes

> > LayoutTests/http/tests/inspector/network/network-xhr-async.html:25
> > +    InspectorTest.evaluateInPage("loadData()");
> 
> Please use InspectorTest.evaluateInConsole instead of these two lines.
No, that would be a different thing since I need to wait for XHR loaded callback.

> > LayoutTests/http/tests/inspector/network/network-xhr-async.html:47
> > +<p>Tests XHR network resource type and content for synchronous requests.</p>
> 
> synchronous -> asynchronous. Also please include a link to the bug.
Done.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:441
> > +            m_pageAgent->getResourceContent(errorString, resourceData->frameId, resourceData->url, optionalBase64Encode, content);
> 
> There is a similar logic on the frontend side. Shouldn't this be done by the frontend in a separate request?
No, the idea is that eventually network panel will always request content from InspectorResourceAgent and Resources Panel will request content from InspectorPageAgent.
This is to be done on the front-end as part of separation of network resources from page resources.
Meanwhile InspectorResourceAgent could need page resources content when requested from network panel. This content is requested here from InspectorPageAgent.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:469
> > +NetworkResourceData::NetworkResourceData()
> 
> identifier and isXHR initialization is missing.
This constructor was not used, removed.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:532
> > +    if (m_loadingXHRSynchronously)
> 
> Shouldn't we check that the identifier passed as parameter is the same as the id of currently loading XHR?
As discussed, no we can not and need not to do that. Since in this case XHR is requested synchronously we will not receive any other requests callbacks in between.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:552
> > +        if (m_identifierToResourceDataMap.get(resourceData->identifier)->loaderId != preservedLoaderId)
> 
> replace with if (resourceData->loaderId != preservedLoaderId) ?
Done.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:573
> > +        resourceData->content = String();
> 
> Remove this line, resourceData destructor will take care of the content string?
This particular line is fine, but the overall logic around it was flawed, thanks for spotting that.
This is now rewritten with the following approach:
 - m_identifierToResourceDataMap is used to store metadata (loaderId, frameId, url, isXHR, etc.) for all resources.
 - when resource has some content, it is added to m_resourceDataDeque as well
Thus m_identifierToResourceDataMap owns resourceData objects while m_resourceDataDeque remembers the order in which content was added only.
Now resourceData will still be available from identifierToResourceDataMap after we clear its content.

> > Source/WebCore/inspector/InspectorResourceAgent.h:78
> > +struct NetworkResourceData {
> 
> This should be moved into NetworkResourcesData as it's a part of the latter.
Done.

> > Source/WebCore/inspector/InspectorResourceAgent.h:92
> > +class NetworkResourcesData {
> 
> This class and the struct above should go into their own file.
Done.

> > Source/WebCore/inspector/front-end/NetworkManager.js:56
> > +        // FIXME: We should distinct NetworkResource (NetworkPanel resource) from ResourceRevision (ResourcesPanel/ScriptsPanel resource)
> 
> distinct -> distinguish, also please file corresponding bug and put its number next to the FIXME.
'separate' should be even better, done.

> > Source/WebCore/xml/XMLHttpRequest.cpp:664
> > +        // Inspector needs to know that a particular resource is XHR at the moment when didReceiveResponse is received.
> 
> I think you can remove this comment since it's clear enough that there are two calls before and after the synchronous load.
Done.

> > Source/WebCore/xml/XMLHttpRequest.cpp:1039
> > +void XMLHttpRequest::didReceiveAuthenticationCancellation(unsigned long identifier, const ResourceResponse& failureResponse)
> 
> Please remove the name of unused param, otherwise you'll break compilation on some platforms.
Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.