Add the CFNetwork implementation of the asynchronous ResourceHandle
Created attachment 217096 [details] Patch
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.
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.
Comment on attachment 217096 [details] Patch Attachment 217096 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/24258057
I'll take a look at this ASAP, I promise!
Created attachment 217112 [details] Patch
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.
Comment on attachment 217112 [details] Patch Attachment 217112 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/24718051
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.
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.
To be clear, it's not my suggestion to use ThreadSafeRefCounted, we should try to not have WebCore objects accessed from multiple threads.
(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.
Created attachment 218259 [details] Patch
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.
Comment on attachment 218259 [details] Patch Attachment 218259 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/39358206
Looks like I will have to do something special for Windows. Working on the two branches sucks. :(
(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.
(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.
(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.
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).
I think that this makes the call for a separate file with async parts even stronger.
Created attachment 218916 [details] Patch
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.
Created attachment 218918 [details] Patch
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.
Created attachment 218922 [details] Patch
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.
Comment on attachment 218922 [details] Patch Attachment 218922 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/47218371
Created attachment 218928 [details] Patch
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.
fuuuuuucking windows!
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.
Created attachment 219006 [details] Patch
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.
Created attachment 219010 [details] Patch
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.
Comment on attachment 219010 [details] Patch Clearing flags on attachment: 219010 Committed r160467: <http://trac.webkit.org/changeset/160467>
All reviewed patches have been landed. Closing bug.
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"?
(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.
(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.
I don't care which it is, but it should be consistent between ResourceHandleCFNet.{h|cpp} and its delegates.