Bug 124440 – Add the CFNetwork implementation of the asynchronous ResourceHandle

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Add the CFNetwork implementation of the asynchronous ResourceHandle

[Benjamin Poulain]

Created attachment 217096 [details]
Patch

[WebKit Commit Bot]

Attachment 217096 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/ResourceHandleInternal.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp']" exit_code: 1
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:230:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:314:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:356:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:391:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:412:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:440:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:483:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:517:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:547:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:581:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:615:  Missing space before {  [whitespace/braces] [5]
Total errors found: 11 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 217096 [details]
Patch

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

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:236
> +        willSendRequest(conn, cfRequest, originalRedirectResponse, clientInfo);

My bad, this should be:
    handle->willSendRequest(request, redirectResponse.get());

I'll fix that before landing if everything else is okay.

[Build Bot]

Comment on attachment 217096 [details]
Patch

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

[Alexey Proskuryakov]

I'll take a look at this ASAP, I promise!

[Benjamin Poulain]

Created attachment 217112 [details]
Patch

[WebKit Commit Bot]

Attachment 217112 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/ResourceHandleInternal.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp']" exit_code: 1
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:230:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:313:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:355:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:390:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:411:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:439:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:482:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:516:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:546:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:580:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:614:  Missing space before {  [whitespace/braces] [5]
Total errors found: 11 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 217112 [details]
Patch

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

[Alexey Proskuryakov]

Comment on attachment 217112 [details]
Patch

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

Looks pretty good!

This will need to be very carefully tested on Windows - iOS and Windows both use CFNetwork directly, but they use very different versions of it. So if this patch accidentally removes a workaround to an old bug, Windows may still be affected.

I think that threading may need another look, please see comments below.

> Source/WebCore/platform/network/ResourceHandle.h:209
>      // Called in response to ResourceHandleClient::willCacheResponseAsync().

This comment applies to both versions, strange to see it between them.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:56
> +#include "Settings.h"

This is a layering violation, Settings is not a platform class. Settings::networkDataUsageTrackingEnabled should be accessed via NetworkingContext.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:88
> +static void reportNetworkDataUsage(NetworkingContext* context, CFURLConnectionRef conn)

Please don't abbr.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:157
> +static ResourceRequest createResourceRequest(CFURLRequestRef cfRequest, const RetainPtr<CFURLResponseRef>& redirectResponse, ResourceHandle& handle)

"const RetainPtr<CFURLResponseRef>&" arguments look so ugly and dangerous, I prefer .get() at call sites - and I think that this is the common WebKit style.

And the reference doesn't tell the reader that the response in non-null anyway.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:220
> +    RefPtr<ResourceHandle> handle = static_cast<ResourceHandle*>(const_cast<void*>(clientInfo));

This looks very suspicious to me. Handle is captured and used on another thread, don't we get in trouble with changing refcount from multiple threads? ResourceHandle is not ThreadSafeRefCounted.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:247
> +    if (connectionWasCancelled(handle))
> +        return 0;
> +
> +    if (handleInternal->m_requestResult.isNull())
> +        return 0;
> +
> +    cfRequest = handleInternal->m_requestResult.cfURLRequest(UpdateHTTPBody);

This all runs on a secondary thread? I don't think that is OK.

Mac uses an NSURLRequest as m_requestResult because ResourceRequest is not safe to use from a secondary thread.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:253
> +static void adjustResponseMimeTypeIfNecessary(ResourceHandle& handle, CFURLResponseRef cfResponse)

s/Mime/MIME/

Also, it looks like we now have two functions, adjustResponseMimeTypeIfNecessary and adjustMIMETypeIfNecessary(CFURLResponseRef). Can there have more distinctive names?

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:265
> +    if (_CFURLRequestCopyProtocolPropertyForKey(handle.firstRequest().cfURLRequest(DoNotUpdateHTTPBody), CFSTR("ForceHTMLMIMEType")))
>          wkSetCFURLResponseMIMEType(cfResponse, CFSTR("text/html"));

Is this actually necessary on Windows? I thought it was only necessary on Mac when external applications opened documents via AppleScript, as a workaround for some weird 3rd party application.

This code is super annoying because it creates a hidden control channel for API clients - instead of using WebKit API, they can add secret properties and control CFNetwork directly.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:326
> +    dispatch_semaphore_wait(handleInternal->m_semaphore, DISPATCH_TIME_FOREVER);

This function doesn't return anything, why do we need to wait?

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:-284
> -    // Workaround for <rdar://problem/6300990> Caching does not respect Vary HTTP header.

This looks like code that can't be deleted on Windows.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:747
> +    if (client() && client()->usesAsyncCallbacks()) {
> +        connectionClient.willSendRequest = willSendRequestAsync;

It's sightly confusing that the client structure is initialized differently in sync vs async modes.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:1233
> +#if USE(QUICK_LOOK) && USE(CFNETWORK)

We probably don't need USE(CFNETWORK) in a *CFNet.cpp file.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:1234
> +CFURLConnectionClient_V6* ResourceHandle::connectionClientCallbacks()

Hmm, this gets even more confusing with a third way to initialize the client structure.

[Benjamin Poulain]

The lifetime problem of ResourceHandle is a very good catch. I will need to revisit each function to see if there is a solution. We may have to use ThreadSafeRefCounted as you said.

Thanks a lot for having a look Alexey. I will make a complete update.

[Alexey Proskuryakov]

To be clear, it's not my suggestion to use ThreadSafeRefCounted, we should try to not have WebCore objects accessed from multiple threads.

[Benjamin Poulain]

(In reply to comment #11)
> To be clear, it's not my suggestion to use ThreadSafeRefCounted, we should try to not have WebCore objects accessed from multiple threads.

I understand. It is also my position that we should avoid it.
I am just not certain we will be able to avoid that here.

[Benjamin Poulain]

Created attachment 218259 [details]
Patch

[WebKit Commit Bot]

Attachment 218259 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/platform/network/ResourceHandle.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/ResourceHandleInternal.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:205:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:280:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:317:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:357:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:382:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:420:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:450:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:496:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:529:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:568:  Missing space before {  [whitespace/braces] [5]
Total errors found: 10 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 218259 [details]
Patch

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

[Benjamin Poulain]

Looks like I will have to do something special for Windows. Working on the two branches sucks. :(

[Brent Fulgham]

(In reply to comment #16)
> Looks like I will have to do something special for Windows. Working on the two branches sucks. :(

I'll apply the patch in the morning and resolve the build issue.

[Benjamin Poulain]

(In reply to comment #17)
> (In reply to comment #16)
> > Looks like I will have to do something special for Windows. Working on the two branches sucks. :(
> 
> I'll apply the patch in the morning and resolve the build issue.

Oh! Awesome! Thanks!

You may have to #ifdef everything. I am not sure what works and what does not from libdispatch.

[Brent Fulgham]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Looks like I will have to do something special for Windows. Working on the two branches sucks. :(
> > 
> > I'll apply the patch in the morning and resolve the build issue.
> 
> Oh! Awesome! Thanks!
> 
> You may have to #ifdef everything. I am not sure what works and what does not from libdispatch.

The problem isn't libdispatch; it's that MSVC doesn't understand the block syntax.  We only have a couple of choices:

1. Change from using a block in "willSendRequestAsync" to a function taking some kind of context value (which is what I had to do in the AVFoundationCF stuff).
2. Switch from using dispatch_async to using the WTF CallOnMainThread thing
3. #if/def it out on Windows and maybe not have the functionality?

The logic in that block is all fine on Windows, and the dispatch_async is fine, so I'd prefer if we could recast the block as a static function if you don't mind.

[Brent Fulgham]

Comment on attachment 218259 [details]
Patch

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

r- because we cannot use block syntax in the Windows code. :-(

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:205
> +    dispatch_async(dispatch_get_main_queue(), ^{

Unfortunately, this won't work on Windows.  MSVC doesn't support the block syntax, and I'm not aware of a tool we can use to convert it to something we can compile cleanly (at least, not using the Open Source toolset).

[Alexey Proskuryakov]

I think that this makes the call for a separate file with async parts even stronger.

[Benjamin Poulain]

Created attachment 218916 [details]
Patch

[WebKit Commit Bot]

Attachment 218916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:47:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:78:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:162:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:176:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:193:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:206:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:218:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:236:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:264:  Missing space before {  [whitespace/braces] [5]
Total errors found: 12 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Created attachment 218918 [details]
Patch

[WebKit Commit Bot]

Attachment 218918 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:47:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:78:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:162:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:176:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:193:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:206:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:218:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:236:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:264:  Missing space before {  [whitespace/braces] [5]
Total errors found: 12 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Created attachment 218922 [details]
Patch

[WebKit Commit Bot]

Attachment 218922 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:47:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:78:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:162:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:176:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:193:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:206:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:218:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:236:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:264:  Missing space before {  [whitespace/braces] [5]
Total errors found: 12 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 218922 [details]
Patch

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

[Benjamin Poulain]

Created attachment 218928 [details]
Patch

[WebKit Commit Bot]

Attachment 218928 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/ResourceHandle.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:47:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:78:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:162:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:176:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:193:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:206:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:218:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:236:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:264:  Missing space before {  [whitespace/braces] [5]
Total errors found: 12 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

fuuuuuucking windows!

[Alexey Proskuryakov]

Comment on attachment 218928 [details]
Patch

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/AsynchronousResourceHandleCFURLConnectionDelegate.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/AsynchronousResourceHandleCFURLConnectionDelegate.cpp:67
> +void AsynchronousResourceHandleCFURLConnectionDelegate::setupRequest(CFMutableURLRequestRef)
> +{
> +}

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

> Source/WebCore/platform/network/cf/AsynchronousResourceHandleCFURLConnectionDelegate.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/AsynchronousResourceHandleCFURLConnectionDelegate.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/AsynchronousResourceHandleCFURLConnectionDelegate.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/AsynchronousResourceHandleCFURLConnectionDelegate.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:130
> +        return 0;

nullptr?

> Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:146
> +    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/SynchronousResourceHandleCFURLConnectionDelegate.cpp:277
> +void SynchronousResourceHandleCFURLConnectionDelegate::continueWillSendRequest(CFURLRequestRef)
> +{
> +}

ASSERT_NOT_REACHED

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

Ditto.

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

Ditto.

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

Ditto.

[Benjamin Poulain]

Created attachment 219006 [details]
Patch

[WebKit Commit Bot]

Attachment 219006 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/ResourceHandle.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.h', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:47:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:78:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:149:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:163:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:178:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:196:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:210:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:224:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:243:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:271:  Missing space before {  [whitespace/braces] [5]
Total errors found: 12 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Created attachment 219010 [details]
Patch

[WebKit Commit Bot]

Attachment 219010 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/ResourceHandle.cpp', u'Source/WebCore/platform/network/ResourceHandle.h', u'Source/WebCore/platform/network/ResourceHandleClient.cpp', u'Source/WebCore/platform/network/ResourceHandleClient.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.h', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp', u'Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.h', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.cpp', u'Source/WebCore/platform/network/cf/SynchronousResourceHandleCFURLConnectionDelegate.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:78:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:135:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:149:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:163:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:178:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:196:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:210:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:224:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:243:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:271:  Missing space before {  [whitespace/braces] [5]
Total errors found: 11 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 219010 [details]
Patch

Clearing flags on attachment: 219010

Committed r160467: <http://trac.webkit.org/changeset/160467>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 219010 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        * platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp: Added.

Ugh, shouldn't it have been "CFNet" not "CFURL"?

[Benjamin Poulain]

(In reply to comment #39)
> (From update of attachment 219010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219010&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        * platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp: Added.
> 
> Ugh, shouldn't it have been "CFNet" not "CFURL"?

It is a delegate of CFURLConnection, that is why it was named like that.

[Sam Weinig]

(In reply to comment #40)
> (In reply to comment #39)
> > (From update of attachment 219010 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219010&action=review
> > 
> > > Source/WebCore/ChangeLog:20
> > > +        * platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp: Added.
> > 
> > Ugh, shouldn't it have been "CFNet" not "CFURL"?
> 
> It is a delegate of CFURLConnection, that is why it was named like that.

Also, CFNet is a terrible prefix :(.  This is plenty clear.  I vote no change.

[Alexey Proskuryakov]

I don't care which it is, but it should be consistent between ResourceHandleCFNet.{h|cpp} and its delegates.