[webkit-reviews] review granted: [Bug 125379] Refactor the CFURLConnectionClient to be the synchronous implementation of an abstract network delegate : [Attachment 218737] 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 125379: Refactor the CFURLConnectionClient to be the synchronous
implementation of an abstract network delegate
https://bugs.webkit.org/show_bug.cgi?id=125379

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

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


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

> +}
>
+ResourceHandleCFURLConnectionDelegate::~ResourceHandleCFURLConnectionDelegate(
)

Missing empty line here.

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.cpp:239
> +#if PLATFORM(WIN)
> +    if (m_handle->client() &&
!m_handle->client()->shouldCacheResponse(m_handle, cachedResponse))
> +	   return 0;
> +#else
> +    CFCachedURLResponseRef newResponse =
m_handle->client()->willCacheResponse(m_handle, cachedResponse);
> +    if (newResponse != cachedResponse)
> +	   return newResponse;
> +#endif

Why are these different?

Strange that we only null check on Windows too.

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.h:35
> +class SynchronousResourceHandleCFURLConnectionDelegate FINAL : public
ResourceHandleCFURLConnectionDelegate {

Thinking about the naming more, I'm still not a fan. This sounds as if it was
about synchronous and asynchronous requests, which it is not. Also, both sync
and (future) async delegates actually use the same API, the only difference is
that one of these is meant to be used on a secondary thread, and has built-in
thread hopping.

But I don't have a better suggestion.

>
Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDele
gate.h:46
> +
> +

Two empty lines.