[webkit-reviews] review denied: [Bug 61205] Web Inspector: Cache XHR content in backend, do not use initialContentSet for XHRs. : [Attachment 94405] Patch with qt build fix

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request 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 94405: Patch with qt build fix
https://bugs.webkit.org/attachment.cgi?id=94405&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94405&action=review

Looks good overall, several minor comments.

> LayoutTests/http/tests/inspector/network/network-xhr-async.html:25
> +    InspectorTest.evaluateInPage("loadData()");

Please use InspectorTest.evaluateInConsole instead of these two lines.

> 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.

> 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?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:469
> +NetworkResourceData::NetworkResourceData()

identifier and isXHR initialization is missing.

> 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?

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

replace with if (resourceData->loaderId != preservedLoaderId) ?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:573
> +	   resourceData->content = String();

Remove this line, resourceData destructor will take care of the content string?


> Source/WebCore/inspector/InspectorResourceAgent.h:78
> +struct NetworkResourceData {

This should be moved into NetworkResourcesData as it's a part of the latter.

> Source/WebCore/inspector/InspectorResourceAgent.h:92
> +class NetworkResourcesData {

This class and the struct above should go into their own file.

> 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.

> 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.

> 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.