[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


Yury Semikhatsky <yurys at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #94405|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Yury Semikhatsky <yurys at chromium.org>  2011-05-23 08:05:17 PST ---
(From update of attachment 94405)
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.

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