Bug 28010 – willCacheResponse tests should be split from the main dumpResourceLoadCallbacks() dumps

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brady Eidson]

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.

[Darin Fisher (:fishd, Google)]

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?

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 34153 [details]
v1 patch - adds layoutTestController.dumpWillCacheResponse()

[Darin Fisher (:fishd, Google)]

Landed as: http://trac.webkit.org/changeset/46809

[Brady Eidson]

So confused about Darin Adler reviewing Darin Fisher's patch and all bugzilla tells me is "darin: review+"
;)

Looks good - thanks, Darin (Fisher)!

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

http://trac.webkit.org/changeset/46812 to remove Tiger-specific results on the 6 tests that no longer dump willCacheResponse().

[Brady Eidson]

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