RESOLVED FIXED 141788
REGRESSION(r177738): Clearing the PageCache no longer clears the PageCache. 
https://bugs.webkit.org/show_bug.cgi?id=141788
Summary REGRESSION(r177738): Clearing the PageCache no longer clears the PageCache.
Andreas Kling
Reported 2015-02-18 18:54:06 PST
We got bit by the TemporaryChange trap!
Attachments
Patch (7.41 KB, patch)
2015-02-18 18:58 PST, Andreas Kling 		andersca: review+
DetailsFormatted DiffDiff
Patch for landing (7.44 KB, patch)
2015-02-18 21:16 PST, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Patch (4.31 KB, patch)
2015-02-18 22:37 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2015-02-18 18:58:45 PST
Created attachment 246874 [details] Patch
Chris Dumez
Comment 2 2015-02-18 19:08:47 PST
Comment on attachment 246874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246874&action=review > LayoutTests/fast/history/page-cache-clearing.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> <!DOCTYPE html> ? > LayoutTests/fast/history/page-cache-clearing.html:29 > + window.testRunner.notifyDone(); This is redundant, I believe finishJSTest() already does that. > LayoutTests/fast/history/page-cache-clearing.html:38 > + window.testRunner.notifyDone(); This is redundant, I believe finishJSTest() already does that. > LayoutTests/fast/history/page-cache-clearing.html:47 > + window.testRunner.waitUntilDone(); window.jsTestIsAsync = true should already take care of this.
Chris Dumez
Comment 3 2015-02-18 19:11:07 PST
Comment on attachment 246874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246874&action=review > LayoutTests/fast/history/page-cache-clearing.html:8 > +<div id="console"></div> This should not be needed. > LayoutTests/fast/history/page-cache-clearing.html:24 > + if (cachedPageCountBefore != 0 || cachedPageCountAfter == 0) Do you mean && ? > LayoutTests/fast/history/page-cache-clearing.html:36 > + debug("FAIL - Page did not enter the page cache."); testFailed("Page did not enter the page cache") ?
Chris Dumez
Comment 4 2015-02-18 19:15:01 PST
Comment on attachment 246874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246874&action=review > LayoutTests/fast/history/page-cache-clearing.html:46 > + window.testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1); This doesn't really need to be done in the onload handler. > LayoutTests/fast/history/page-cache-clearing.html:51 > + setTimeout(function() { Do we really need this setTimeout. Cannot we simply set href in the onload handler?
Andreas Kling
Comment 5 2015-02-18 21:16:52 PST
Created attachment 246877 [details] Patch for landing Fixed the passing condition in the test as pointed out by Chris.
Chris Dumez
Comment 6 2015-02-18 21:20:43 PST
Comment on attachment 246877 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=246877&action=review > LayoutTests/fast/history/page-cache-clearing.html:19 > + debug("Cached page count before clearing: " + cachedPageCountBefore); Isn't printing the pageCount here dangerous? How do we know there is always the same amount of pages in the page cache when running this test? > LayoutTests/fast/history/page-cache-clearing.html:29 > + window.testRunner.notifyDone(); still redundant. > LayoutTests/fast/history/page-cache-clearing.html:38 > + window.testRunner.notifyDone(); still redundant. > LayoutTests/fast/history/page-cache-clearing.html:47 > + window.testRunner.waitUntilDone(); still redundant.
Andreas Kling
Comment 7 2015-02-18 21:28:12 PST
(In reply to comment #6) > Comment on attachment 246877 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246877&action=review > > > LayoutTests/fast/history/page-cache-clearing.html:19 > > + debug("Cached page count before clearing: " + cachedPageCountBefore); > > Isn't printing the pageCount here dangerous? How do we know there is always > the same amount of pages in the page cache when running this test? AFAIK the page cache is disabled between tests, as part of the "reset everything between tests so conditions are consistent" mechanism. Sent from my iThing.
Chris Dumez
Comment 8 2015-02-18 21:37:34 PST
This breaks Windows build due to missing symbol export according to EWS.
WebKit Commit Bot
Comment 9 2015-02-18 22:07:56 PST
Comment on attachment 246877 [details] Patch for landing Clearing flags on attachment: 246877 Committed r180337: <http://trac.webkit.org/changeset/180337>
WebKit Commit Bot
Comment 10 2015-02-18 22:08:00 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 11 2015-02-18 22:18:41 PST
Windows build fix landed in <http://trac.webkit.org/changeset/180338>.
Chris Dumez
Comment 12 2015-02-18 22:37:00 PST
Reopening to attach new patch.
Chris Dumez
Comment 13 2015-02-18 22:37:02 PST
Created attachment 246882 [details] Patch
Chris Dumez
Comment 14 2015-02-18 22:38:00 PST
Attaching a follow-up patch to clean up the layout test a bit.
Andreas Kling
Comment 15 2015-02-18 22:54:22 PST
Comment on attachment 246882 [details] Patch r=me Generally speaking I like tests to be a bit idiosyncratic since that increases our general code coverage. That said I don't mind these changes. And thanks for fixing the windows build!
WebKit Commit Bot
Comment 16 2015-02-18 23:37:22 PST
Comment on attachment 246882 [details] Patch Clearing flags on attachment: 246882 Committed r180339: <http://trac.webkit.org/changeset/180339>
WebKit Commit Bot
Comment 17 2015-02-18 23:37:28 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 18 2015-03-06 10:40:41 PST
Radar: rdar://problem/20072369
Chris Dumez
Comment 19 2015-03-06 10:47:27 PST
This is a regression from r177738, not r179347.
Note You need to log in before you can comment on or make changes to this bug.