Bug 174059 - Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue
Summary: Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-30 17:46 PDT by Alex Christensen
Modified: 2017-07-03 13:17 PDT (History)
2 users (show)

See Also:


Attachments
Patch (19.49 KB, patch)
2017-06-30 17:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2017-07-03 13:16 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-06-30 17:46:40 PDT
Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue
Comment 1 Alex Christensen 2017-06-30 17:47:19 PDT
Created attachment 314338 [details]
Patch
Comment 2 Alex Christensen 2017-06-30 17:52:33 PDT
This will allow me to use this code on Windows.
Comment 3 Sam Weinig 2017-07-01 06:57:56 PDT
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 4 Andy Estes 2017-07-03 11:22:49 PDT
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.
Comment 5 Alex Christensen 2017-07-03 13:16:55 PDT
Created attachment 314510 [details]
Patch
Comment 6 Alex Christensen 2017-07-03 13:17:53 PDT
http://trac.webkit.org/r219089