Web Inspector backend should get headers & cookies from network process separately from resource requests

[youenn fablet]

Web Inspector backend should get headers & cookies from network process separately from resource requests

[youenn fablet]

<rdar://problem/38877384>

[youenn fablet]

Created attachment 337446 [details]
Patch

[youenn fablet]

There is no test, I guess there should be some related to headers and inspector.
Probably when increasing the scope of response filtering, this patch will make sure inspector tests do not fail.

[EWS Watchlist]

Comment on attachment 337446 [details]
Patch

Attachment 337446 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7242791

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html

[EWS Watchlist]

Created attachment 337448 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[BJ Burg]

Comment on attachment 337446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337446&action=review

> Source/WebCore/loader/LoaderStrategy.h:82
> +    virtual ResourceResponse responseFromResourceLoaIdentifier(uint64_t /* resourceLoadIdentifier */);

There is a typo in these two methods, please fix.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:84
> +    void takeNetworkLoadInformationResponse(ResourceLoadIdentifier identifier, WebCore::ResourceResponse& response)

Is there any particular reason to use an out-parameter?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:184
> +    HashMap<ResourceLoadIdentifier, NetworkLoadInformation> m_networkLoadInformations;

The plural of information is information, so adding 's' doesn't sound good. I would rename the typedef to NetworkLoadData/NetworkLoadResponseData and call this m_responseDataForResourceLoad.

[youenn fablet]

(In reply to Brian Burg from comment #6)
> Comment on attachment 337446 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337446&action=review
> 
> > Source/WebCore/loader/LoaderStrategy.h:82
> > +    virtual ResourceResponse responseFromResourceLoaIdentifier(uint64_t /* resourceLoadIdentifier */);
> 
> There is a typo in these two methods, please fix.

OK

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:84
> > +    void takeNetworkLoadInformationResponse(ResourceLoadIdentifier identifier, WebCore::ResourceResponse& response)
> 
> Is there any particular reason to use an out-parameter?

Sync IPC, but I think I will update it so that there is no need to create a ResourceResponse and copy it.

> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:184
> > +    HashMap<ResourceLoadIdentifier, NetworkLoadInformation> m_networkLoadInformations;
> 
> The plural of information is information, so adding 's' doesn't sound good.
> I would rename the typedef to NetworkLoadData/NetworkLoadResponseData and
> call this m_responseDataForResourceLoad.

OK

[youenn fablet]

Created attachment 338314 [details]
Patch

[BJ Burg]

Comment on attachment 338314 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338314&action=review

r=me

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:426
> +    return source != ResourceResponse::Source::MemoryCache && source != ResourceResponse::Source::ServiceWorker && source != ResourceResponse::Source::ApplicationCache && source != ResourceResponse::Source::MemoryCacheAfterValidation;

Nit; a switch would be easier to read & maintain.

> Source/WebCore/loader/LoaderStrategy.h:85
> +    virtual ResourceResponse responseFromResourceLoadIdentifier(uint64_t /* resourceLoadIdentifier */);

Nit: no need to comment out the parameter names in the declaration.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:279
> +        // FIXME: All loaders should provide their origin if navigation mode is cors/no-cors/same-origin.

I don't quite understand this part. On the assumption it's going to be used for things other than Inspector, I'll let you determine the best way forward to make it more accurate.

[youenn fablet]

Thanks for the review.

(In reply to Brian Burg from comment #9)
> Comment on attachment 338314 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338314&action=review
> 
> r=me
> 
> > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:426
> > +    return source != ResourceResponse::Source::MemoryCache && source != ResourceResponse::Source::ServiceWorker && source != ResourceResponse::Source::ApplicationCache && source != ResourceResponse::Source::MemoryCacheAfterValidation;
> 
> Nit; a switch would be easier to read & maintain.

OK

> > Source/WebCore/loader/LoaderStrategy.h:85
> > +    virtual ResourceResponse responseFromResourceLoadIdentifier(uint64_t /* resourceLoadIdentifier */);
> 
> Nit: no need to comment out the parameter names in the declaration.

OK

> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:279
> > +        // FIXME: All loaders should provide their origin if navigation mode is cors/no-cors/same-origin.
> 
> I don't quite understand this part. On the assumption it's going to be used
> for things other than Inspector, I'll let you determine the best way forward
> to make it more accurate.

We are using sourceOrigin to do CORS checks in NetworkProcess.
This is currently limited to sync XHR and beacon API but will be extended to other types of loads.

[youenn fablet]

Created attachment 338338 [details]
Patch for landing

[youenn fablet]

Created attachment 338354 [details]
Win fix

[WebKit Commit Bot]

Comment on attachment 338354 [details]
Win fix

Clearing flags on attachment: 338354

Committed r230820: <https://trac.webkit.org/changeset/230820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.