Bug 47507 - Loader cleanup: Merge CachePolicy and ResourceRequestCachePolicy
Summary: Loader cleanup: Merge CachePolicy and ResourceRequestCachePolicy
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
Depends on:
Blocks: 29947
  Show dependency treegraph
Reported: 2010-10-11 15:10 PDT by Nate Chapin
Modified: 2011-01-12 16:37 PST (History)
6 users (show)

See Also:

patch (25.11 KB, patch)
2010-10-11 15:26 PDT, Nate Chapin
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-10-11 15:10:14 PDT
It seems like these enums are almost equivalent, but not quite overlapping.  Currently, ResourceRequestCachePolicy overloads ReloadIgnoringCacheData to mean both a typical reload (revalidate all cached items) and shift-reload (remove all items from cache and fetch everything again).  If these were split out, CachePolicy would be a subset of ResourceRequestCachePolicy and could presumably be deleted.  It would also allow us to remove the overloaded definition of a reload (see, e.g., http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=69432#L1585).

Does this seem reasonable?  Will post a patch soon.
Comment 1 Nate Chapin 2010-10-11 15:26:49 PDT
Created attachment 70479 [details]

I tried to find all the places where ResourceRequestCachePolicy is used in the webkit repo and disambiguate between ReloadIgnoringCacheData and RevalidateCacheData, but I make no promises that I found al of them :)
Comment 2 Adam Barth 2010-10-11 15:31:09 PDT
> I make no promises that I found al of them :)

You can rename enum and the compiler will help you find them all.
Comment 3 Alexey Proskuryakov 2010-10-11 15:42:32 PDT
I'm not sure if these should be the same. One policy is about what is being sent in requests over the wire, and another is how WebCore handles cached data that it has.

There is a lot of confusion because CFNetwork cache behaves like a remote caching proxy, so it's logically "over the wire", although it really isn't. But there's also WebCore cache, which can have different policies. It can also encode different WebCore responses to the same network responses (e.g. "just use stale data" vs. "use it, but warn the user" - there is no difference for ResourceRequest, but there is for WebCore).
Comment 4 WebKit Review Bot 2010-10-11 17:25:55 PDT
Attachment 70479 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4353022
Comment 5 Antti Koivisto 2010-10-12 02:59:57 PDT
I agree with Alexey. CachePolicy (which should perhaps become CachedResource::CachePolicy) is about WebCore internal caching. Using ResourceRequestCachePolicy in this level seems wrong.
Comment 6 Adam Barth 2011-01-12 16:36:59 PST
Comment on attachment 70479 [details]

Sounds like we shouldn't do this change.