Bug 174059 – Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue

[Alex Christensen]

Created attachment 314338 [details]
Patch

[Alex Christensen]

This will allow me to use this code on Windows.

[Sam Weinig]

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.

[Andy Estes]

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.

[Alex Christensen]

Created attachment 314510 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r219089