Bug 80905 - Implement ApplicationCacheHost::canCacheInPageCache() for chromium.
Summary: Implement ApplicationCacheHost::canCacheInPageCache() for chromium.
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
Depends on:
Reported: 2012-03-12 16:12 PDT by Gavin Peters
Modified: 2012-03-13 13:48 PDT (History)
4 users (show)

See Also:

Patch (1.07 KB, patch)
2012-03-12 16:15 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2012-03-12 16:18 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (1.07 KB, patch)
2012-03-12 16:24 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (1.98 KB, patch)
2012-03-12 16:32 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (1.98 KB, patch)
2012-03-13 08:26 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2012-03-12 16:12:00 PDT
Implement ApplicationCacheHost::canCacheInPageCache() for chromium.
Comment 1 Gavin Peters 2012-03-12 16:15:34 PDT
Created attachment 131438 [details]
Comment 2 Gavin Peters 2012-03-12 16:18:20 PDT
Created attachment 131440 [details]
Comment 3 Gavin Peters 2012-03-12 16:21:11 PDT
Wait a second while I learn webkit-patch upload.
Comment 4 Gavin Peters 2012-03-12 16:24:48 PDT
Created attachment 131442 [details]
Comment 5 Gavin Peters 2012-03-12 16:32:44 PDT
Created attachment 131446 [details]
Comment 6 Gavin Peters 2012-03-12 16:34:31 PDT
It took me a while, but I managed to get webkit-patch to upload a real patch.

I am not turning on the PageCache with this; Settings stops it as I note in my comment.  However, this will make the statistics I'm gathering in PageCache.cpp to reevaluate that decision much better.

Note this patch is made relative to Bug 80904, so can't land until after that patch.
Comment 7 WebKit Review Bot 2012-03-12 17:09:57 PDT
Comment on attachment 131446 [details]

Attachment 131446 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11943230
Comment 8 Gavin Peters 2012-03-12 20:46:44 PDT
Comment on attachment 131446 [details]

The chromium buildbot failure is expected; given that 80904 hasn't landed yet.  I'll upload another patch to double-check EWS after any review.
Comment 9 Gavin Peters 2012-03-13 08:26:04 PDT
Created attachment 131619 [details]
Comment 10 Gavin Peters 2012-03-13 08:26:38 PDT
Bug 80904 has landed, so this upload is the same as the last one: except I expect it to pass EWS because the tree has changed.
Comment 11 Eric Seidel (no email) 2012-03-13 11:58:03 PDT
Comment on attachment 131619 [details]

I assume you've audited the callers?
Comment 12 Gavin Peters 2012-03-13 12:06:14 PDT
There are two callers: both are in PageCache.cpp.

The first is in the debug printing/histogram gathering code in the PageCache.  This change will make that family of histograms better reflect what PageCache performance would be if we used the PageCache.

The second is in PageCache::canCachePageContainingThisFrame().  That's moot because frameLoader->client()->canCachePage() will always fail, so it will always return the same result.

As well, page->settings()->usesPageCache() is usually false in chrome (modulo a command line argument that combined with the above does nothing).

If I ever want to make frameLoader->client()->canCachePage() report better stats, I'll have to either disable the command line flag to "turn on pagecache" or make page-cache work well enough not to ruin the lives of those brave canaries who have set this (currently nonfunctional) command line argument.
Comment 13 WebKit Review Bot 2012-03-13 13:48:15 PDT
Comment on attachment 131619 [details]

Clearing flags on attachment: 131619

Committed r110600: <http://trac.webkit.org/changeset/110600>
Comment 14 WebKit Review Bot 2012-03-13 13:48:28 PDT
All reviewed patches have been landed.  Closing bug.