Created attachment 265746[details][diff][review]
another stab
This version asks the HTTP channel whether the cache is valid.
It's a bit awkward, setting up an HTTP channel and opening it with load flags that should prevent any blocking/network IO. But I think the alternative (reworking the cache checking code to work without a running channel) would probably be worse/more error prone.

Absolutely. One of the original Google Maps guys told me they really wanted this. For example in Maps if the user zooms in, they can decide whether or not to show a zooming animation and if so, whether to use zoomed-out tiles and scale up or zoomed-in tiles and scale down. isLocallyAvailable would let them make these decisions intelligently.

Comment on attachment 265746[details][diff][review]
another stab
+nsNavigator::IsLocallyAvailable(const nsAString &aURI,
+ nsICachingChannel::LOAD_BYPASS_LOCAL_CACHE_IF_BUSY |
why this load flag?
also, I think it'd make more sense to move the LOAD_ONLY_IF_MODIFIED flag into the else-block of the offline check. like the comment says, it doesn't have an effect in the other case.
+ rv = channel->GetStatus(&status);
+ NS_ENSURE_SUCCESS(rv, rv);
+
+ stream->Close();
it seems like you should call Close even when getting the status fails
+ *aIsAvailable = (status == 0);
you should check whether status is a success code, not whether it is zero.
--
I don't really like LOAD_FORCE_VALIDITY_CHECK. It seems to be useful only for the case where you want to check validity without doing network I/O. Perhaps it would be more useful to have a LOAD_NO_NETWORK_IO flag that fails the load when it would have to hit the network?
(FORCE_VALIDITY_CHECK kind of sounds like VALIDATE_ALWAYS...)

(In reply to comment #10)
> (From update of attachment 265746[details][diff][review])
> +nsNavigator::IsLocallyAvailable(const nsAString &aURI,
> + nsICachingChannel::LOAD_BYPASS_LOCAL_CACHE_IF_BUSY |
>
> why this load flag?
I figure returning not-available is better than blocking on a network load if the requested resource is in the middle of being loaded.
> also, I think it'd make more sense to move the LOAD_ONLY_IF_MODIFIED flag into
> the else-block of the offline check. like the comment says, it doesn't have an
> effect in the other case.
hrm? The LOAD_ONLY_IF_MODIFIED is there to avoid setting up the cache pump if a cache entry is found, it applies in either case.

(In reply to comment #10)
> + rv = channel->GetStatus(&status);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + stream->Close();
>
> it seems like you should call Close even when getting the status fails
Fixed.
> + *aIsAvailable = (status == 0);
>
> you should check whether status is a success code, not whether it is zero.
Fixed this, and checked the HTTP status for a success code.
> I don't really like LOAD_FORCE_VALIDITY_CHECK. It seems to be useful only for
> the case where you want to check validity without doing network I/O. Perhaps it
> would be more useful to have a LOAD_NO_NETWORK_IO flag that fails the load when
> it would have to hit the network?
I added a LOAD_NO_NETWORK_IO flag.

So I have two issues with this patch:
1) Some of the channel impls don't follow the contract on NS_ERROR_NEEDS_NETWORK -- they need to send it to OnStopRequest, not throw it from AsyncOpen, per the documentation.
2) There need to be changes to nsWebShell so that NS_ERROR_NEEDS_NETWORK will show a reasonable error page.
3) Why use NS_ERROR_NEEDS_NETWORK when we already have NS_ERROR_DOCUMENT_NOT_CACHED?
4) How is LOAD_NO_NETWORK_IO different from LOAD_ONLY_FROM_CACHE (which lives on nsICachingChannel)?

(In reply to comment #19)
> 4) How is LOAD_NO_NETWORK_IO different from LOAD_ONLY_FROM_CACHE (which lives
> on nsICachingChannel)?
In practice, LOAD_ONLY_FROM_CACHE is a combination of "don't go to the network" and "skip validation". For the purposes of this patch we need "don't go to the network", but we want items that would be validated to fail. It would probably be cleaner to separate LOAD_ONLY_FROM_CACHE into two separate flags, but I figure that would break semantics that are relied upon elsewhere.

(In reply to comment #19)
> 2) There need to be changes to nsWebShell so that NS_ERROR_NEEDS_NETWORK will
> show a reasonable error page.
>
> 3) Why use NS_ERROR_NEEDS_NETWORK when we already have
> NS_ERROR_DOCUMENT_NOT_CACHED?
That's probably worth changing.

This caused a major security regression: bug 441751. The reason was that comment 20 was wrong. LOAD_ONLY_FROM_CACHE did NOT imply "skip validation" in all cases (LOAD_FROM_CACHE did that), and the fact that this patch actually changed it to mean that in fact broke semantics that session history depended on.
So it sounds to me like in bug 441751 we should probably unbreak LOAD_ONLY_FROM_CACHE and then consider using it for what this bug wanted and nuking LOAD_NO_NETWORK_IO, if that would work.