[webkit-reviews] review granted: [Bug 174059] Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue : [Attachment 314338] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 174059: Stop using dispatch_async in
ResourceHandleCFURLConnectionDelegateWithOperationQueue
https://bugs.webkit.org/show_bug.cgi?id=174059

Attachment 314338: Patch

https://bugs.webkit.org/attachment.cgi?id=314338&action=review




--- Comment #4 from Andy Estes <aestes at apple.com> ---
Comment on attachment 314338
  --> https://bugs.webkit.org/attachment.cgi?id=314338
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314338&action=review

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.cpp:113
> +	   ASSERT(context);

I don't think you need to assert this.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.cpp:149
> +	   ASSERT(context);

I don't think you need to assert this.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.cpp:203
> +    callOnMainThread([protectedThis = makeRef(*this), handle =
RefPtr<ResourceHandle>(m_handle)] () mutable {

Ditto.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.cpp:215
> +    callOnMainThread([protectedThis = makeRef(*this), handle =
RefPtr<ResourceHandle>(m_handle), error = RetainPtr<CFErrorRef>(error)] ()
mutable {

Ditto.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.cpp:233
> +	   ASSERT(context);

I don't think you need to assert this.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.cpp:292
> +	   ASSERT(context);

I don't think you need to assert this.

>
Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOpe
rationQueue.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.