[webkit-reviews] review granted: [Bug 175386] [Beacon][NetworkSession] Support CORS-preflighting on redirects : [Attachment 317734] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 175386: [Beacon][NetworkSession] Support CORS-preflighting on redirects
https://bugs.webkit.org/show_bug.cgi?id=175386

Attachment 317734: Patch

https://bugs.webkit.org/attachment.cgi?id=317734&action=review




--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 317734
  --> https://bugs.webkit.org/attachment.cgi?id=317734
Patch

The only question is whether this is fine to do so without going through CSP or
content blocker.
Given this is a replacement over synchronous XHR, and that shipping this might
allow us to remove synchronous XHR, this looks like an improvement to me.

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

> Source/WebKit/NetworkProcess/PingLoad.cpp:46
> +    ,
m_sameOriginRequest(securityOrigin().canRequest(m_parameters.request.url()))

m_isSameOriginRequest might be a better name.

> Source/WebKit/NetworkProcess/PingLoad.cpp:61
> +	   m_redirectHandler({ });

Why do we need to do that?

> Source/WebKit/NetworkProcess/PingLoad.cpp:75
> +	   loadParameters.request = request;

Aren't we creating two ResourceRequest, while we should only create one?

> Source/WebKit/NetworkProcess/PingLoad.cpp:76
> +	   m_task = NetworkDataTask::create(*networkSession, *this,
loadParameters);

And we are not moving loadParameters, which probably means that NetworkDataTask
is creating its own copy of ResourceRequest.
Some refactoring might be useful there?

> Source/WebKit/NetworkProcess/PingLoad.cpp:102
> +    // Use a unique for subsequent loads if needed.

Use a unique origin?

> Source/WebKit/NetworkProcess/PingLoad.cpp:106
> +	   m_origin = SecurityOrigin::createUnique();

You could add a check so as to not create a new origin if m_origin is not
unique.

> LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:21
> +	   response.text().then(body => {

You can just do "return response.json()" here.

> LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:25
> +    }), 1000);

Cannot we use the new polling mechanism that landed in the other patch?

> LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:40
> +	 result = JSON.parse(json);

No need for JSON.parse if using json() above.

>
LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-from-crossorigin-to-sa
meorigin.html:25
> +    }), 1000);

Ditto, or put in common some code of these two tests/merge them into one.