We need to keep the cache1 IDLs internally (the appcache still uses them), but we should turn them off as soon as it looks like cache2 is going to stick, since they will no longer actually access anything but appcache entries. Better to break addons then have them "work" in a broken way.

Plan: - let fail any attempt for sessions and eviction other then of STORE_OFFLINE policy - make visitEntries throw (no control over what storage policy you want to visit) - introduce nsICacheService.visitOfflineCache, and make it only visit the offline device (~7 places in the code base to fix) - remove all netwerk/test/Test*.* working with the old cache This can be easily controlled by the use_new_backend pref and I think would mostly be just enough to make this no longer work and catch some attention.

Roughly tested patch. https://tbpl.mozilla.org/?tree=Try&rev=856279926b28

Comment on attachment 8429616 [details] [diff] [review] wip1 Michal, please check on this as well, Jason is not a deep cache1 expert - no offense :)

Here you have a whitespace ignored patch for simpler overlook.

Michal, as discussed with Jason yesterday and outlined at https://etherpad.mozilla.org/Cache2Tasks I am working on a different patch.

- nsICacheService methods throw unconditionally when cache2=on - internal methods on nsCacheService class introduced, cache2 calls on them directly - rest of the browser/etc code touching appcache migrated to the new cache api (mostly those are just wrappers, so functionality is the same) Don't worry, most of the patch are just removals :) I believe the changes are functionally identical so that there is no need for a review from module peers we touch and reviews from both of you are OK. https://tbpl.mozilla.org/?tree=Try&rev=bd66b2b457e8

Comment on attachment 8434359 [details] [diff] [review] v2 Review of attachment 8434359 [details] [diff] [review]: ----------------------------------------------------------------- The parts I looked over seem good. I don't know the API changes from old->new cache as well as Michal, so I'll leave the parts that switch those APIs to him to review. ::: browser/devtools/shared/AppCacheUtils.jsm @@ +27,5 @@ > > const { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > let { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {}); > +let { Services } = Cu.import("resource://gre/modules/Services.jsm", {});LoadContextInfo Is the "LoadContextInfo" bareword here a typo? My JS sucks but this looks wrong. ::: netwerk/cache/nsICacheService.idl @@ +24,5 @@ > interface nsICacheService : nsISupports > { > /** > + * @throws NS_ERROR_NOT_IMPLEMENTED when the cache v2 is prefered to use > + * and the storagePolicy is not STORE_OFFLINE. Looks like you don't actually check + permit STORE_OFFLINE. Instead you're making code call *Internal functions. Which is fine :) but maybe change comment to remove part about STORE_OFFLINE? ::: netwerk/cache2/OldWrappers.cpp @@ +620,5 @@ > nsCOMPtr<nsICacheSession> session; > + rv = nsCacheService::GlobalInstance()->CreateSessionInternal(clientId.get(), > + storagePolicy, > + nsICache::STREAM_BASED, > + getter_AddRefs(session)); I didn't scan code past patch context, but maybe you don't need 'serv' variable any more now that you use GlobalInstance?

(In reply to Jason Duell (:jduell) from comment #8) > Comment on attachment 8434359 [details] [diff] [review] > v2 Thanks! > > Is the "LoadContextInfo" bareword here a typo? My JS sucks but this looks > wrong. Yep, that is wrong... Try seems to be quiet. > but maybe change > comment to remove part about STORE_OFFLINE? Leftover from the first version. > > ::: netwerk/cache2/OldWrappers.cpp > @@ +620,5 @@ > > nsCOMPtr<nsICacheSession> session; > > + rv = nsCacheService::GlobalInstance()->CreateSessionInternal(clientId.get(), > > + storagePolicy, > > + nsICache::STREAM_BASED, > > + getter_AddRefs(session)); > > I didn't scan code past patch context, but maybe you don't need 'serv' > variable any more now that you use GlobalInstance? Oh, I do! We need to protect GlobalInstance() from going away e.g. during some shutdown glitch and also make sure it actually exists. This is the most simple and safest way to have GlobalInstance up and running in the block.

Backed out for xpcshell failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dbdb35eace https://tbpl.mozilla.org/php/getParsedLog.php?id=41129773&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41128719&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41129570&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41129856&tree=Mozilla-Inbound

Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/c0323d9a7ea3 with dependency on bug 938186 removed.