[webkit-reviews] review granted: [Bug 179251] Make ResourceLoader::willSendRequestInternal asynchronous : [Attachment 326022] 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 179251: Make ResourceLoader::willSendRequestInternal asynchronous
https://bugs.webkit.org/show_bug.cgi?id=179251

Attachment 326022: Patch

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




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

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

> Source/WebCore/loader/SubresourceLoader.cpp:243
> +    ResourceLoader::willSendRequestInternal(WTFMove(newRequest),
redirectResponse, [this, protectedThis = makeRef(*this), completionHandler =
WTFMove(completionHandler), redirectResponse = redirectResponse]
(ResourceRequest&& request) mutable {

You can just type "redirectResponse" to capture by value. You don't need to
type "redirectResponse = redirectResponse".

> Source/WebCore/loader/cache/CachedResource.cpp:297
> +    platformStrategies()->loaderStrategy()->loadResource(frame, *this,
WTFMove(request), m_options, [this, frame = makeRef(frame), loggingAllowed =
cachedResourceLoader.isAlwaysOnLoggingAllowed()] (RefPtr<SubresourceLoader>&&
loader) {

Is capturing |this| safe here? Do you need a CachedResourceHandle?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:93
> +    SubresourceLoader::create(frame, resource, WTFMove(request), options,
[this, completionHandler = WTFMove(completionHandler), resource =
CachedResourceHandle<CachedResource>(&resource), frame = makeRef(frame)]
(RefPtr<SubresourceLoader>&& loader) mutable {

Is it safe to capture |this| here without protecting it?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:104
> +    NetscapePlugInStreamLoader::create(frame, client, WTFMove(request),
[this, completionHandler = WTFMove(completionHandler), frame = makeRef(frame)]
(RefPtr<NetscapePlugInStreamLoader>&& loader) mutable {

Ditto.

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp:93
> +    SubresourceLoader::create(frame, resource, WTFMove(request), options,
[this, completionHandler = WTFMove(completionHandler)]
(RefPtr<WebCore::SubresourceLoader>&& loader) mutable {

Ditto.

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp:116
> +    NetscapePlugInStreamLoader::create(frame, client, WTFMove(request),
[this, completionHandler = WTFMove(completionHandler)]
(RefPtr<WebCore::NetscapePlugInStreamLoader>&& loader) mutable {

Ditto.