We got bit by the TemporaryChange trap!
Created attachment 246874 [details] Patch
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.
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") ?
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?
Created attachment 246877 [details] Patch for landing Fixed the passing condition in the test as pointed out by Chris.
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.
(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.
This breaks Windows build due to missing symbol export according to EWS.
Comment on attachment 246877 [details] Patch for landing Clearing flags on attachment: 246877 Committed r180337: <http://trac.webkit.org/changeset/180337>
All reviewed patches have been landed. Closing bug.
Windows build fix landed in <http://trac.webkit.org/changeset/180338>.
Reopening to attach new patch.
Created attachment 246882 [details] Patch
Attaching a follow-up patch to clean up the layout test a bit.
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!
Comment on attachment 246882 [details] Patch Clearing flags on attachment: 246882 Committed r180339: <http://trac.webkit.org/changeset/180339>
Radar: rdar://problem/20072369
This is a regression from r177738, not r179347.