The default bug view has changed. See this FAQ.

implement navigator.isLocallyAvailable

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
DOM
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: cajbir, Assigned: dcamp)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9alpha8
x86
Windows XP
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 257860 [details] [diff] [review]
First cut at implementing isLocallyAvailable. 

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)

Updated

10 years ago
Assignee: chris.double → dcamp
(Assignee)

Comment 2

10 years ago
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.
Attachment #257860 - Attachment is obsolete: true
Attachment #265746 - Flags: review?(cbiesinger)
(Assignee)

Updated

10 years ago
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha5
(Assignee)

Comment 3

10 years ago
The test case in this patch exposed bug 381657, adding that as a dep.
Depends on: 381657
(Assignee)

Comment 4

10 years ago
This bug depends on the patch in bug 372970.
Depends on: 372970

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
can't you just check whether the given URI is present in the offline cache device?
(Assignee)

Comment 6

10 years ago
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...)
(Assignee)

Comment 11

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Created attachment 269410 [details] [diff] [review]
new version
Attachment #265746 - Attachment is obsolete: true
Attachment #269410 - Flags: review?(cbiesinger)
Attachment #265746 - Flags: review?(cbiesinger)
(Assignee)

Comment 13

10 years ago
(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
(Assignee)

Comment 17

10 years ago
Created attachment 270210 [details] [diff] [review]
use GetRequestSucceeded(), update datetime and finger

use GetRequestSucceeded(), update datetime and finger.
Attachment #269410 - Attachment is obsolete: true
Attachment #270210 - Flags: superreview?(jst)

Updated

10 years ago
Attachment #270210 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 18

10 years ago
committed
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
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)?
(Assignee)

Comment 20

10 years ago
(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.

(Assignee)

Comment 21

10 years ago
(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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 441751
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.

Updated

5 years ago
Depends on: 727579

Updated

5 years ago
No longer depends on: 727579
You need to log in before you can comment on or make changes to this bug.