Bug 125379 – Refactor the CFURLConnectionClient to be the synchronous implementation of an abstract network delegate

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Refactor the CFURLConnectionClient to be the synchronous implementation of an abstract network delegate

[Benjamin Poulain]

Created attachment 218650 [details]
Patch

[Build Bot]

Comment on attachment 218650 [details]
Patch

Attachment 218650 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/46498007

[Benjamin Poulain]

Created attachment 218652 [details]
Patch

[Build Bot]

Comment on attachment 218652 [details]
Patch

Attachment 218652 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/46618001

[Benjamin Poulain]

Created attachment 218725 [details]
Patch

[Build Bot]

Comment on attachment 218725 [details]
Patch

Attachment 218725 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/44858218

[Benjamin Poulain]

Created attachment 218726 [details]
Patch

[Build Bot]

Comment on attachment 218726 [details]
Patch

Attachment 218726 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/47178210

[Benjamin Poulain]

Created attachment 218737 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 218737 [details]
Patch

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/SynchronousResourceHandleCFURLConnectionDelegate.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/SynchronousResourceHandleCFURLConnectionDelegate.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/SynchronousResourceHandleCFURLConnectionDelegate.h:46
> +
> +

Two empty lines.

[Benjamin Poulain]

(In reply to comment #10)
> > Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.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.

Probably historical reasons. The whole API on Windows is different here.

> > Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.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.

I'll land with the current name. It is not a big deal changing that afterwards (or hopefully getting rid of the classes eventually).

[Benjamin Poulain]

Committed r160338: <http://trac.webkit.org/changeset/160338>