Bug 28010 - willCacheResponse tests should be split from the main dumpResourceLoadCallbacks() dumps
Summary: willCacheResponse tests should be split from the main dumpResourceLoadCallbac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-04 19:02 PDT by Brady Eidson
Modified: 2009-08-05 14:02 PDT (History)
1 user (show)

See Also:


Attachments
v1 patch - adds layoutTestController.dumpWillCacheResponse() (21.52 KB, patch)
2009-08-05 11:26 PDT, Darin Fisher (:fishd, Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2009-08-04 19:02:56 PDT
willCacheResponse tests should be split from the main dumpResourceLoadCallbacks() dumps.

From email on webkit-dev:

On Aug 4, 2009, at 5:35 PM, Darin Fisher wrote:

There is a Mac-only willCacheResponse notification that gets recorded by several of the layout tests when layoutTestController.dumpResourceLoadCallbacks(true) is called.  However, this notification which originates at the ResourceHandle level is currently #if PLATFORM(MAC).  I noticed that the tests which depend on this notification are currently skipped on platform/win.  For Chromium, we have simply rebaselined with the willCacheResponse events excluded.  Having a separate baseline is unfortunate and not running the tests on platform/win seems even worse.

I wonder if it wouldn't be better to exclude willCacheResponse by default, and then only enable it selectively for tests that are about testing it.  That way, we can un-skip the other tests that only fail on non-Mac as a side-effect of this notification not being supported.  We could also try to support willCacheResponse on all platforms, but that seems like a lot of work for little gain.  Afterall, it is a notification that WebKit doesn't itself care about.  It is just something provided to the embedder of the WebKit Framework on Mac.

I didn't see any bugs on file about this issue.

-Darin

My reply:

willCacheResponse was lumped in with dumpResourceLoadCallbacks() solely for convenience based on the situation at the time.

As you point out, it is only interesting for embedders of mainline WebKit on Mac.  

My two proposals are to either split off a new dumpWillCacheResponse() callback or change dumpResourceLoadCallbacks() to take a second argument that is false by default.

I slightly prefer the latter.
Comment 1 Darin Fisher (:fishd, Google) 2009-08-04 23:46:42 PDT
I'm happy to help implement this, and either of your suggestions are fine by me.  Would we only enable willCacheResponse for the http/tests/misc/willCacheResponse-delegate-callback.html layout test?
Comment 2 Darin Fisher (:fishd, Google) 2009-08-05 10:27:42 PDT
dumpResourceLoadCallbacks() doesn't technically take any arguments.  I see that some tests pass a 'true' parameter to it, but the code ignores all parameters and just blindly sets m_dumpResourceLoadCallbacks to true.  So, I think a new dumpWillCacheResponse is probably better.  I'm going to go with that.
Comment 3 Darin Fisher (:fishd, Google) 2009-08-05 11:26:48 PDT
Created attachment 34153 [details]
v1 patch - adds layoutTestController.dumpWillCacheResponse()
Comment 4 Darin Fisher (:fishd, Google) 2009-08-05 13:17:18 PDT
Landed as: http://trac.webkit.org/changeset/46809
Comment 5 Brady Eidson 2009-08-05 13:19:23 PDT
So confused about Darin Adler reviewing Darin Fisher's patch and all bugzilla tells me is "darin: review+"
;)

Looks good - thanks, Darin (Fisher)!
Comment 6 Brady Eidson 2009-08-05 13:20:23 PDT
Actually...

This will break some tests that have Tiger/Windows/Leopard/SnowLeopard specific results checked in.

I'll try to clean up those as the build bots are running into them now.
Comment 7 Brady Eidson 2009-08-05 13:32:55 PDT
I landed  http://trac.webkit.org/changeset/46810 to rebaseline the Tiger-specific results for 2 of these tests, and http://trac.webkit.org/changeset/46811 to remove the Windows specific results for 4 of these tests, as they now match the cross-platform results.
Comment 8 Brady Eidson 2009-08-05 13:47:01 PDT
http://trac.webkit.org/changeset/46812 to remove Tiger-specific results on the 6 tests that no longer dump willCacheResponse().
Comment 9 Brady Eidson 2009-08-05 14:02:03 PDT
In a comedy of errors, restoring those 4 windows-specific results (didn't run diff on them correctly, they *did* have differences)

http://trac.webkit.org/changeset/46813