752663 - LOAD_FROM_CACHE should be renamed

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

The LOAD_FROM_CACHE is either being used incorrectly, or behaving incorrectly.  Either way it has a terribly confusing name.

LOAD_FROM_CACHE actually seems to be logically a member of the VALIDATE_ALWAYS | VALIDATE_ONCE_PER_SESSION | VALIDATE_NEVER continuum, which is itself badly named:  VALIDATE_NEVER actually still does validation in some cases (for "no-store" entries, and SSL entries with "no-cache"), so we seem to have created LOAD_FROM_CACHE as a way to load from cache always:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2764

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2775

We should probably rename VALIDATE_NEVER to "VALIDATE_BYPASS_UNLESS_REQUIRED" and rename LOAD_FROM_CACHE "VALIDATE_BYPASS_ALL".  Before I run off and write such a patch, though, I want to have buy-in that my analysis is actually correct, and that we're willing to change the IDLs.

Meanwhile, we seem to have gotten some places in our code where we're assuming that LOAD_NO_CACHE also implies that we don't hit the network, which is not true (we'll hit the network if the item isn't in cache):  I'll file a bug for that separately.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Analysis seems correct, at first glance, though not sure about the names.

We do have a history of people mixing up LOAD_FROM_CACHE and LOAD_ONLY_FROM_CACHE, unsurprisingly.

Comment 2

[Jason Duell]

Filed bug 752675 for the current possible misuses.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from comment #0)
> The LOAD_FROM_CACHE is either being used incorrectly, or behaving
> incorrectly.  Either way it has a terribly confusing name.
> 
> LOAD_FROM_CACHE actually seems to be logically a member of the
> VALIDATE_ALWAYS | VALIDATE_ONCE_PER_SESSION | VALIDATE_NEVER continuum,
> which is itself badly named:  VALIDATE_NEVER actually still does validation
> in some cases (for "no-store" entries, and SSL entries with "no-cache"), so
> we seem to have created LOAD_FROM_CACHE as a way to load from cache always:

In Internet Explorer > Internet Options > General tab > Browsing History section > Settings > Check for newer versions of stored web pages, there are options that correspond almost one-for-one to these flags. I thought we had a similar UI, but I cannot find it. I didn't verify that this is the case, but it is something to explore. If my guess is correct, and if we don't have such a UI anymore, we may just be able to remove these VALIDATE_* flags.

> We should probably rename VALIDATE_NEVER to
> "VALIDATE_BYPASS_UNLESS_REQUIRED" and rename LOAD_FROM_CACHE
> "VALIDATE_BYPASS_ALL".

I find the "BYPASS" in the name to be confusing too, because of the use of the BYPASS_CACHE constants.

Perhaps better names would be VALIDATE_IF_STALE_NEVER | VALIDATE_IF_STALE_ONCE_PER_SESSION | VALIDATE_IF_STALE_ALWAYS.

> Meanwhile, we seem to have gotten some places in our code where we're
> assuming that LOAD_NO_CACHE also implies that we don't hit the network,
> which is not true (we'll hit the network if the item isn't in cache):  I'll
> file a bug for that separately.

I think you must mean LOAD_FROM_CACHE. I agree the LOAD_FROM_CACHE and LOAD_ONLY_FROM_CACHE names are too similar and make things too confusing, especially when you add LOAD_NO_NETWORK_IO into the mix.

Comment 4

[Boris Zbarsky [:bzbarsky]]

"We" certainly used to have such UI, and some Gecko-based apps still do, even if Firefox doesn't.

Furthermore, there are extensions that expose such UI.