373231 - implement navigator.isLocallyAvailable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[cajbir (:cajbir)]

Attachment: First cut at implementing isLocallyAvailable.

Implement navigator.isLocallyAvailable as described at http://www.campd.org/stuff/Offline%20Cache.html

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How does this work with cache validation? E.g. if the page was cached with If-Modified-Since, this should return false ... does it?

Comment 2

[Dave Camp (:dcamp)]

Attachment: 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.

Comment 3

[Dave Camp (:dcamp)]

The test case in this patch exposed bug 381657, adding that as a dep.

Comment 4

[Dave Camp (:dcamp)]

This bug depends on the patch in bug 372970.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

can't you just check whether the given URI is present in the offline cache device?

Comment 6

[Dave Camp (:dcamp)]

For the whenOffline case that'd work;  For the !offline case the idea is to check whether the request would hit the network (although that's a bit racy).

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

is the !offline case really useful for anything?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 9

[Damon Sicore (:damons)]

Not happening for A5, moving to A6.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

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...)

Comment 11

[Dave Camp (:dcamp)]

(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.

Comment 12

[Dave Camp (:dcamp)]

Attachment: new version

Comment 13

[Dave Camp (:dcamp)]

(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.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 269410 [details] [diff] [review]
new version

+    rv = httpChannel->GetResponseStatus(&httpStatus);

why not use isRequestSucceeded?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

the channels in extensions/finger and extensions/datetime should perhaps also check that flag.

Comment 16

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 17

[Dave Camp (:dcamp)]

Attachment: use GetRequestSucceeded(), update datetime and finger

use GetRequestSucceeded(), update datetime and finger.

Comment 18

[Dave Camp (:dcamp)]

committed

Comment 19

[Boris Zbarsky [:bzbarsky]]

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)?

Comment 20

[Dave Camp (:dcamp)]

(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.

Comment 21

[Dave Camp (:dcamp)]

(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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ah, validation.  Makes sense, yeah....

Comment 23

[Eric Shepherd [:sheppy]]

Now documented in reference:

http://developer.mozilla.org/en/docs/DOM:window.navigator.isLocallyAvailable

Please give it a look and correct any problems.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.