Closed Bug 373231 Opened 17 years ago Closed 17 years ago

implement navigator.isLocallyAvailable

Categories

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

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: cajbir, Assigned: dcamp)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

Implement navigator.isLocallyAvailable as described at http://www.campd.org/stuff/Offline%20Cache.html
How does this work with cache validation? E.g. if the page was cached with If-Modified-Since, this should return false ... does it?
Assignee: chris.double → dcamp
Attached patch another stab (obsolete) — Splinter Review
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.
Attachment #257860 - Attachment is obsolete: true
Attachment #265746 - Flags: review?(cbiesinger)
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha5
The test case in this patch exposed bug 381657, adding that as a dep.
Depends on: 381657
This bug depends on the patch in bug 372970.
Depends on: 372970
Flags: blocking1.9? → blocking1.9+
can't you just check whether the given URI is present in the offline cache device?
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).
is the !offline case really useful for anything?
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.
Not happening for A5, moving to A6.
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
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.
Attached patch new version (obsolete) — Splinter Review
Attachment #265746 - Attachment is obsolete: true
Attachment #269410 - Flags: review?(cbiesinger)
Attachment #265746 - Flags: review?(cbiesinger)
(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 on attachment 269410 [details] [diff] [review]
new version

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

why not use isRequestSucceeded?
Attachment #269410 - Flags: review?(cbiesinger) → review+
the channels in extensions/finger and extensions/datetime should perhaps also check that flag.
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
use GetRequestSucceeded(), update datetime and finger.
Attachment #269410 - Attachment is obsolete: true
Attachment #270210 - Flags: superreview?(jst)
Attachment #270210 - Flags: superreview?(jst) → superreview+
committed
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Keywords: dev-doc-needed
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.
Ah, validation.  Makes sense, yeah....
Now documented in reference:

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

Please give it a look and correct any problems.
Depends on: CVE-2009-0358
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.
Depends on: 727579
No longer depends on: 727579
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.