Bug 183010 – [Curl] Remove unnecessary copied ResourceRequest member variable.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Basuke Suzuki]

It was used when both redirect and authentication happens, but by adding const reference getter to CurlRequest, it is no longer needed.

[Basuke Suzuki]

This is a successor bug for https://bugs.webkit.org/show_bug.cgi?id=181704.

[Basuke Suzuki]

Created attachment 334406 [details]
patch

[youenn fablet]

Comment on attachment 334406 [details]
patch

Some questions below.

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

> Source/WebCore/platform/network/ResourceHandle.h:300
> +    enum class PreflightRequest {

I am not clear what PreflightRequest means in that context.
In the context of HTTP, preflight requests are used for CORS, but this does not seem related.
Would there be a better name to explicit the purpose. Is it a mechanism to do conditional requests?

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:136
> +Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest& request, PreflightRequest preflight)

Would it make sense to take a ResourceRequest&& in case CurlRequest is probably creating its own copy of the request?

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:364
> +    auto previousRequest = d->m_curlRequest->resourceRequest().isolatedCopy();

Is the isolatedCopy needed here?

[Basuke Suzuki]

Comment on attachment 334406 [details]
patch

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

Thanks for reviewing!

>> Source/WebCore/platform/network/ResourceHandle.h:300
>> +    enum class PreflightRequest {
> 
> I am not clear what PreflightRequest means in that context.
> In the context of HTTP, preflight requests are used for CORS, but this does not seem related.
> Would there be a better name to explicit the purpose. Is it a mechanism to do conditional requests?

Ah, right. I use preflight not meaning about CORS, but general meaning of preparation by parsing the headers or such info. Is `Preprocess` the better name?

>> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:136
>> +Ref<CurlRequest> ResourceHandle::createCurlRequest(ResourceRequest& request, PreflightRequest preflight)
> 
> Would it make sense to take a ResourceRequest&& in case CurlRequest is probably creating its own copy of the request?

Okay, i'll think about it.

>> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:364
>> +    auto previousRequest = d->m_curlRequest->resourceRequest().isolatedCopy();
> 
> Is the isolatedCopy needed here?

Ditto.

[youenn fablet]

> > I am not clear what PreflightRequest means in that context.
> > In the context of HTTP, preflight requests are used for CORS, but this does not seem related.
> > Would there be a better name to explicit the purpose. Is it a mechanism to do conditional requests?
> 
> Ah, right. I use preflight not meaning about CORS, but general meaning of
> preparation by parsing the headers or such info. Is `Preprocess` the better
> name?

Preprocess is very generic as well and it seems that this is related to an existing cache so I would try to come up with a name inducing the notion of cache.

If this is related to HTTP conditional request/revalidation, I would try to make the name as specific as possible.

[Basuke Suzuki]

Created attachment 335136 [details]
Patch

Rename Preflight to RequestStatus which indicates it's new or reuse. Also stop isolateCopy, but simple copy. Move is not used here because original request need to be valid in m_firstRequest.

[WebKit Commit Bot]

Comment on attachment 335136 [details]
Patch

Rejecting attachment 335136 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 335136, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
=335136
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/6831883

[Basuke Suzuki]

Created attachment 335142 [details]
changelog

[WebKit Commit Bot]

Comment on attachment 335142 [details]
changelog

Clearing flags on attachment: 335142

Committed r229347: <https://trac.webkit.org/changeset/229347>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38200401>