Summary: | Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, sam | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Alex Christensen
2017-06-30 17:46:40 PDT
Created attachment 314338 [details]
Patch
This will allow me to use this code on Windows. Comment on attachment 314338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314338&action=review > Source/WebCore/ChangeLog:9 > + Use dispatch_async_f and callOnMainThread instead. > + No change in behavior. This is missing a reason why. Comment on attachment 314338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314338&action=review > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:110 > + struct ProtectedParameters { > + Ref<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protectedThis; > + RefPtr<ResourceHandle> handle; > + RetainPtr<CFURLRequestRef> cfRequest; > + RetainPtr<CFURLResponseRef> originalRedirectResponse; > + }; This is fine, but I wonder if using tuples and std::tie would be better than a struct. Maybe that's not supported right now in MSVC? > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:113 > + ASSERT(context); I don't think you need to assert this. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:116 > + auto& parameters = *reinterpret_cast<ProtectedParameters*>(context); > + auto& protectedThis = parameters.protectedThis; > + auto& handle = parameters.handle; It's not clear why you chose to unpack these three parameters but not the others. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:146 > + struct ProtectedParameters { > + Ref<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protectedThis; > + RefPtr<ResourceHandle> handle; > + RetainPtr<CFURLConnectionRef> connection; > + RetainPtr<CFURLResponseRef> cfResponse; > + }; This is fine, but I wonder if using tuples and std::tie would be better than a struct. Maybe that's not supported right now in MSVC? > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:149 > + ASSERT(context); I don't think you need to assert this. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:152 > + auto& parameters = *reinterpret_cast<ProtectedParameters*>(context); > + auto& protectedThis = parameters.protectedThis; > + auto& handle = parameters.handle; It's not clear why you chose to unpack these three parameters but not the others. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:192 > + callOnMainThread([protectedThis = makeRef(*this), handle = RefPtr<ResourceHandle>(m_handle), data = RetainPtr<CFDataRef>(data), originalLength = originalLength] () mutable { Do you need to create a second reference to m_handle if you already have protectedThis? > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:203 > + callOnMainThread([protectedThis = makeRef(*this), handle = RefPtr<ResourceHandle>(m_handle)] () mutable { Ditto. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:215 > + callOnMainThread([protectedThis = makeRef(*this), handle = RefPtr<ResourceHandle>(m_handle), error = RetainPtr<CFErrorRef>(error)] () mutable { Ditto. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:230 > + struct ProtectedParameters { > + Ref<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protectedThis; > + RefPtr<ResourceHandle> handle; > + RetainPtr<CFCachedURLResponseRef> cachedResponse; > + }; This is fine, but I wonder if using tuples and std::tie would be better than a struct. Maybe that's not supported right now in MSVC? > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:233 > + ASSERT(context); I don't think you need to assert this. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:236 > + auto& parameters = *reinterpret_cast<ProtectedParameters*>(context); > + auto& protectedThis = parameters.protectedThis; > + auto& handle = parameters.handle; It's not clear why you chose to unpack these three parameters but not the others. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:267 > + callOnMainThread([protectedThis = makeRef(*this), handle = RefPtr<ResourceHandle>(m_handle), totalBytesWritten, totalBytesExpectedToWrite] () mutable { Do you need to create a second reference to m_handle if you already have protectedThis? > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:289 > + struct ProtectedParameters { > + Ref<ResourceHandleCFURLConnectionDelegateWithOperationQueue> protectedThis; > + RefPtr<ResourceHandle> handle; > + RetainPtr<CFURLProtectionSpaceRef> protectionSpace; > + }; This is fine, but I wonder if using tuples and std::tie would be better than a struct. Maybe that's not supported right now in MSVC? > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:292 > + ASSERT(context); I don't think you need to assert this. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp:295 > + auto& parameters = *reinterpret_cast<ProtectedParameters*>(context); > + auto& protectedThis = parameters.protectedThis; > + auto& handle = parameters.handle; It's not clear why you chose to unpack these three parameters but not the others. Created attachment 314510 [details]
Patch
|