WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
124440
Add the CFNetwork implementation of the asynchronous ResourceHandle
https://bugs.webkit.org/show_bug.cgi?id=124440
Summary
Add the CFNetwork implementation of the asynchronous ResourceHandle
Benjamin Poulain
Reported
2013-11-15 16:18:40 PST
Add the CFNetwork implementation of the asynchronous ResourceHandle
Attachments
Patch
(43.97 KB, patch)
2013-11-15 16:24 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(44.28 KB, patch)
2013-11-15 20:06 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(33.07 KB, patch)
2013-12-02 20:12 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(50.78 KB, patch)
2013-12-10 16:02 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(50.85 KB, patch)
2013-12-10 16:12 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(51.00 KB, patch)
2013-12-10 16:52 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(52.36 KB, patch)
2013-12-10 17:52 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(53.10 KB, patch)
2013-12-11 14:36 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Patch
(53.10 KB, patch)
2013-12-11 15:28 PST
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Benjamin Poulain
Comment 1
2013-11-15 16:24:04 PST
Created
attachment 217096
[details]
Patch
WebKit Commit Bot
Comment 2
2013-11-15 16:25:57 PST
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 3
2013-11-15 16:32:27 PST
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 4
2013-11-15 17:05:26 PST
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
Comment 5
2013-11-15 17:09:48 PST
I'll take a look at this ASAP, I promise!
Benjamin Poulain
Comment 6
2013-11-15 20:06:08 PST
Created
attachment 217112
[details]
Patch
WebKit Commit Bot
Comment 7
2013-11-15 20:09:33 PST
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 8
2013-11-15 21:49:52 PST
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 9
2013-11-16 00:08:21 PST
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
Comment 10
2013-11-16 00:28:33 PST
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
Comment 11
2013-11-16 01:14:08 PST
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
Comment 12
2013-11-16 01:17:07 PST
(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
Comment 13
2013-12-02 20:12:19 PST
Created
attachment 218259
[details]
Patch
WebKit Commit Bot
Comment 14
2013-12-02 20:15:35 PST
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 15
2013-12-02 20:42:20 PST
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
Comment 16
2013-12-02 23:29:42 PST
Looks like I will have to do something special for Windows. Working on the two branches sucks. :(
Brent Fulgham
Comment 17
2013-12-03 00:13:55 PST
(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
Comment 18
2013-12-03 00:16:08 PST
(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
Comment 19
2013-12-03 10:30:12 PST
(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 20
2013-12-03 10:31:31 PST
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
Comment 21
2013-12-03 12:19:31 PST
I think that this makes the call for a separate file with async parts even stronger.
Benjamin Poulain
Comment 22
2013-12-10 16:02:39 PST
Created
attachment 218916
[details]
Patch
WebKit Commit Bot
Comment 23
2013-12-10 16:04:02 PST
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
Comment 24
2013-12-10 16:12:48 PST
Created
attachment 218918
[details]
Patch
WebKit Commit Bot
Comment 25
2013-12-10 16:15:44 PST
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
Comment 26
2013-12-10 16:52:22 PST
Created
attachment 218922
[details]
Patch
WebKit Commit Bot
Comment 27
2013-12-10 16:53:45 PST
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 28
2013-12-10 17:35:19 PST
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
Comment 29
2013-12-10 17:52:14 PST
Created
attachment 218928
[details]
Patch
WebKit Commit Bot
Comment 30
2013-12-10 17:54:29 PST
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
Comment 31
2013-12-10 17:57:10 PST
fuuuuuucking windows!
Alexey Proskuryakov
Comment 32
2013-12-11 00:31:32 PST
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
Comment 33
2013-12-11 14:36:14 PST
Created
attachment 219006
[details]
Patch
WebKit Commit Bot
Comment 34
2013-12-11 14:52:18 PST
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
Comment 35
2013-12-11 15:28:58 PST
Created
attachment 219010
[details]
Patch
WebKit Commit Bot
Comment 36
2013-12-11 15:43:11 PST
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 37
2013-12-11 17:53:18 PST
Comment on
attachment 219010
[details]
Patch Clearing flags on attachment: 219010 Committed
r160467
: <
http://trac.webkit.org/changeset/160467
>
Benjamin Poulain
Comment 38
2013-12-11 17:53:22 PST
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 39
2013-12-25 13:00:23 PST
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
Comment 40
2013-12-25 23:00:44 PST
(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
Comment 41
2013-12-25 23:13:02 PST
(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
Comment 42
2013-12-25 23:26:27 PST
I don't care which it is, but it should be consistent between ResourceHandleCFNet.{h|cpp} and its delegates.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug