[webkit-reviews] review denied: [Bug 106123] REGRESSION(r138222?): [Mac WK1] http/tests/appcache/main-resource-redirect.html asserts in WebFrameLoaderClient::dispatchDidFinishLoading : [Attachment 181725] Fix by preventing ResourceLoader from sending callbacks

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Nate Chapin
<japhet at chromium.org>'s request for review:
Bug 106123: REGRESSION(r138222?): [Mac WK1]
http/tests/appcache/main-resource-redirect.html asserts in
WebFrameLoaderClient::dispatchDidFinishLoading
https://bugs.webkit.org/show_bug.cgi?id=106123

Attachment 181725: Fix by preventing ResourceLoader from sending callbacks
https://bugs.webkit.org/attachment.cgi?id=181725&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=181725&action=review


> Source/WebCore/loader/MainResourceLoader.cpp:185
> +	   // We need to remove our reference to the CachedResource (which will
probably cancel its underlying ResourceLoader)

Why is the cancellation just "probable"? If the load is not canceled now, will
it be canceled later, outside the temporary DoNotSendCallbacks mode?

> Source/WebCore/loader/MainResourceLoader.cpp:192
> +	   if (resourceLoader)
> +	       resourceLoader->setSendCallbackPolicy(DoNotSendCallbacks);
>	   clearResource();
> +	   resourceLoader->setSendCallbackPolicy(SendCallbacks);

It looks wrong to have a null check the first time we dereference
resourceLoader, but not the second time. If it's actually possible to not have
a resourceLoader here, please add a regression test for this case.

Also, is this expected to reset policy to original value? Can we assert that
SendCallbacks is actually the original value?

> Source/WebCore/ChangeLog:14
> +	   (WebCore::ResourceLoader::sendCallbackPolicy): Renamed from
shouldSendResourceLoadCallbacks(), returns an enum value instead of a bool.

I'm not sure why this is an improvement. A boolean gives the needed answer more
directly, as evidenced by how you had to modify the call site.