[webkit-reviews] review requested: [Bug 61205] Web Inspector: Cache XHR content in backend, do not use initialContentSet for XHRs. : [Attachment 94770] Patch with fixes

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has asked  for review:
Bug 61205: Web Inspector: Cache XHR content in backend, do not use
initialContentSet for XHRs.
https://bugs.webkit.org/show_bug.cgi?id=61205

Attachment 94770: Patch with fixes
https://bugs.webkit.org/attachment.cgi?id=94770&action=review

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
> > 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.