[webkit-reviews] review granted: [Bug 124440] Add the CFNetwork implementation of the asynchronous ResourceHandle : [Attachment 218928] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 124440: Add the CFNetwork implementation of the asynchronous ResourceHandle
https://bugs.webkit.org/show_bug.cgi?id=124440

Attachment 218928: Patch
https://bugs.webkit.org/attachment.cgi?id=218928&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=218928&action=review


r=me

> Source/WebCore/ChangeLog:9
> +	   Add a second subclass of ResourceHandleCFURLConnectionDelegate:
AsynchronousResourceHandleCFURLConnectionDelegate.
> +	   The difference is those objects handle the network callback on a
different queue.

This explanation should probably be somehow in the name
(ResourceHandleCFURLConnectionQueueDelegate?
ResourceHandleCFURLConnectionDelegateForOperationQueue?)

>
Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDel
egate.cpp:63
> +static bool connectionWasCancelled(ResourceHandle* handle)
> +{
> +    return !handle;
> +}

This function is always called as connectionWasCancelled(protector->h_handle).
It's quite a strange, because a null handle doesn't always say anything about
any connection.

Can this be made a member function, to be called as
protector->connectionWasCancelled()?

Or maybe you could just check "if (!m_handle)" everywhere?

>
Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDel
egate.cpp:67
> +void
AsynchronousResourceHandleCFURLConnectionDelegate::setupRequest(CFMutableURLReq
uestRef)
> +{
> +}

Did performance measurement show that we no longer need
CFURLRequestSetShouldStartSynchronously with NetworkProcess? This seems
surprising.

>
Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDel
egate.cpp:81
> +	   LOG(Network, "CFNet -
AsynchronousResourceHandleCFURLConnectionDelegate::willSendRequest(handle=%p)
(%s)", m_handle, m_handle->firstRequest().url().string().utf8().data());
> +
> +	   if (connectionWasCancelled(protector->m_handle)) {

This doesn't look right. We get m_handle from different places, and only null
check after use.

The situation is the same in every callback below.

>
Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDel
egate.cpp:127
> +    dispatch_semaphore_wait(m_semaphore, DISPATCH_TIME_FOREVER);

We should have a comment here (and in Mac code too) explaining why we need to
wait, even though this function doesn't return any result.

My understanding is that it's because WebKit could decide to convert the
connection to a download while handling didReceiveResponse.

>
Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDel
egate.cpp:251
> +#if PLATFORM(IOS)
> +	   if (coreProtectionSpace.authenticationScheme() ==
ProtectionSpaceAuthenticationSchemeUnknown) {
> +	       m_boolResult = false;
> +	       dispatch_semaphore_signal(m_semaphore);
> +	       return;
> +	   }
> +#endif // PLATFORM(IOS)

Can you easily find out why this is done, and why it's iOS only? A FIXME may be
in order.

>
Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDel
egate.h:41
> +    ~AsynchronousResourceHandleCFURLConnectionDelegate();

virtual

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:300
> +    if (ResourceHandleClient* client = this->client()) {
> +	   if (client->usesAsyncCallbacks())
> +	       client->shouldUseCredentialStorageAsync(this);
> +	   else
> +	       return client->shouldUseCredentialStorage(this);
> +    }

This is a pretty ugly pattern, but the best I could come up with on Mac.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:41

> +#if PLATFORM(MAC)
> +#include "WebCoreSystemInterface.h"
> +#endif // PLATFORM(MAC)
> +
> +#if PLATFORM(WIN)
> +#include <WebKitSystemInterface/WebKitSystemInterface.h>
> +#endif // PLATFORM(WIN)

Do we have a style rule requiring these comments after endif? They look
somewhat silly here.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:13
0
> +	   return 0;

nullptr?

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:14
6
> +    if (httpMessage && CFHTTPMessageGetResponseStatusCode(httpMessage) ==
307) {

Is this code necessary on iOS and Mac? Please see <rdar://problem/13625208> for
discussion.

> Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h:60

> +    ResourceRequest createResourceRequest(CFURLRequestRef, const
RetainPtr<CFURLResponseRef>&);

What is the benefit of passing "const RetainPtr<CFURLResponseRef>&" instead of
CFURLResponseRef?

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.cpp:277
> +void
SynchronousResourceHandleCFURLConnectionDelegate::continueWillSendRequest(CFURL
RequestRef)
> +{
> +}

ASSERT_NOT_REACHED

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.cpp:280
> +{

Ditto.

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.cpp:284
> +{

Ditto.

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.cpp:288
> +{

Ditto.