Bug 114075

Summary: Returning NULL from willSendRequest should cancel a load from the memory cache
Product: WebKit Reporter: Andy Estes <aestes>
Component: Page LoadingAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, japhet, laszlo.gombos, webkit-bug-importer, webkit.review.bot, zarvai
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Andy Estes 2013-04-05 17:23:24 PDT
Returning NULL from willSendRequest should cancel a load from the memory cache
Comment 1 Andy Estes 2013-04-05 17:25:18 PDT
<rdar://problem/13228856>
Comment 2 Andy Estes 2013-04-05 17:48:40 PDT
Created attachment 196709 [details]
Patch
Comment 3 Darin Adler 2013-04-05 19:57:27 PDT
Comment on attachment 196709 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.h:156
> +    bool shouldContinueAfterNotifyingLoadedFromMemoryCache(CachedResource*);

I might have suggested using a bool out argument instead of return value. Then you would not have had to change the function name. But probably OK this way.

> LayoutTests/fast/loader/resources/cached-image.html:3
> +        testRunner.setWillSendRequestReturnsNull(true);

Strange that this is a function call rather than a property.

> LayoutTests/fast/loader/resources/cached-image.html:5
> +<img src="compass.jpg" onload="console.log('FAIL: image was incorrectly loaded')">

Do we really need a new image for this? Can’t we use an existing image that’s already somewhere in the test tree?
Comment 4 Andy Estes 2013-04-05 20:02:27 PDT
(In reply to comment #3)
> (From update of attachment 196709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196709&action=review
> > LayoutTests/fast/loader/resources/cached-image.html:5
> > +<img src="compass.jpg" onload="console.log('FAIL: image was incorrectly loaded')">
> 
> Do we really need a new image for this? Can’t we use an existing image that’s already somewhere in the test tree?

I was running into a case where DRT would print absolute file paths in its resource load delegate messages when the path to the resource was not a subpath of the main resource. Adding a new image (the only image in fast/loader/resources/) was my workaround. I'm going to investigate fixing this in <https://bugs.webkit.org/show_bug.cgi?id=114074>.

Thanks for the review!
Comment 5 Andy Estes 2013-04-05 20:08:02 PDT
Committed r147829: <http://trac.webkit.org/changeset/147829>
Comment 6 Zoltan Arvai 2013-04-08 02:47:25 PDT
On Qt WK1 we have different result for the test. What is your opinion about this result?

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/loader/willsendrequest-returns-null-for-memory-cache-load-expected.txt
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/loader/willsendrequest-returns-null-for-memory-cache-load-actual.txt
@@ -4,6 +4,8 @@
 resources/compass.jpg - didFinishLoading
 resources/cached-image.html - willSendRequest <NSURLRequest URL resources/cached-image.html, main document URL willsendrequest-returns-null-for-memory-cache-load.html, http method GET> redirectResponse (null)
 resources/cached-image.html - didReceiveResponse <NSURLResponse resources/cached-image.html, http status code 0>
-resources/compass.jpg - willSendRequest <NSURLRequest URL resources/compass.jpg, main document URL (null), http method GET> redirectResponse (null)
-resources/compass.jpg - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code -999, failing URL "resources/compass.jpg">
+resources/compass.jpg - willSendRequest <NSURLRequest URL resources/compass.jpg, main document URL , http method GET> redirectResponse (null)
+resources/compass.jpg - didReceiveResponse <NSURLResponse resources/compass.jpg, http status code 0>
+resources/compass.jpg - didFinishLoading
+CONSOLE MESSAGE: line 5: FAIL: image was incorrectly loaded
Comment 7 Darin Adler 2013-04-08 10:06:10 PDT
That result looks like what you’d get if the willSendRequest function did not return null. Maybe something wrong with the test harness?
Comment 8 Andy Estes 2013-04-08 14:59:18 PDT
I agree with Darin. Zoltan, does the Qt DRT implement testRunner.setWillSendRequestReturnsNull()?