Closed Bug 219556 Opened 21 years ago Closed 20 years ago

necko support for resumable downloads via http

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: benc, Assigned: Biesinger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

From bug 98288:

This bug is for HTTP changes in necko needed to resolve bug 98288. I moved that
bug to download manager, because all the dupes were from download manager and it
was in HTTP, which kept it from preventing dupes.
Blocks: 98288
Target Milestone: --- → Future
Summary: necko support for resumable downloads → necko support for resumable downloads via http
taking.
Assignee: darin → cbiesinger
Target Milestone: Future → mozilla1.7alpha
Attached patch patch v1 (obsolete) — Splinter Review
Comment on attachment 136505 [details] [diff] [review]
patch v1

this patch doesn't call OnStartRequest if the download can not be resumed...
should it?
Attachment #136505 - Flags: review?(darin)
Status: NEW → ASSIGNED
Comment on attachment 136505 [details] [diff] [review]
patch v1

>Index: base/public/nsIResumableEntityID.idl

>+    attribute string entityTag;

make this an ACString.


>Index: base/src/nsResumableEntityID.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/src/nsResumableEntityID.cpp,v
>retrieving revision 1.2
>diff -p -u -5 -r1.2 nsResumableEntityID.cpp
>--- base/src/nsResumableEntityID.cpp	8 Jan 2003 22:14:49 -0000	1.2
>+++ base/src/nsResumableEntityID.cpp	29 Nov 2003 19:53:47 -0000
>@@ -1,6 +1,7 @@
> /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+// vim:et ts=4 sw=4 sts=4 cin:

uber-nit: i prefer to use C style comments for the VIM control line since
the surrounding comments are all C style.  but, to do that, i've found that
i need to change the VIM control line like this:

/* vim:set et ts=4 sw=4 sts=4 cin: */

without the "set" it seems VIM tries to parse the trailing "*/"



>+    if (!thisEmpty && !otherEmpty) {
>+        if (mEntityTag.Equals(entityTag)) {
>+            // Equal ETag -> Entities are equal, unless this is a weak tag
>+            // if it is a weak tag, we also compare size and time, see below
>+            if (!StringBeginsWith(mEntityTag, NS_LITERAL_CSTRING("W/"))) {
>+                *ret = PR_TRUE;
>+                return NS_OK;
>+            }

can you explain this comment further?  i don't think it is correct
to examine the syntax of the ETag.


>Index: protocol/http/src/nsHttpChannel.cpp

i still need to review the rest of this...
Attachment #136505 - Flags: review?(darin) → review-
>without the "set" it seems VIM tries to parse the trailing "*/"

ah thanks. the parsing of the */ was the reason I used a C++-style comment, I
didn't know that I could avoid it with set.

>can you explain this comment further?  i don't think it is correct
>to examine the syntax of the ETag.

ETags starting with W/ are weak entity tags, which according to rfc2616:
"MAY be shared by
   two entities of a resource only if the entities are equivalent and
   could be substituted for each other with no significant change in
   semantics. A weak entity tag can only be used for weak comparison."

I don't think that a "semantic" equivalence should suffice for resuming.
(strong ETags can only be shared by two entities if they are "equivalent by
octet equality")
Blocks: 227057
well... we don't make such a distinction when issuing If-Range requests to
complete a partially downloaded document.  maybe we should be including
If-Unmodified-Since with range requests that only include a weak ETag.  hmm...

>+            // Equal ETag -> Entities are equal, unless this is a weak tag
>+            // if it is a weak tag, we also compare size and time, see below

hmm... why not always check size and time?  why skip this for strong ETags?
>hmm... why not always check size and time?  why skip this for strong ETags?

well, I was thinking that two entities can only have the same strong ETag if
they are identical; so that a matching strong ETag means that they are
definitely identical and date/size need not be checked.

I can change that if you want, of course. hm... maybe a check if at least the
size matches would also be good (different size -> can't be identical)
(I'd rather not check the date if a strong etag matches: if the last-mod date
was changed, the file contents may still be unchanged, in which case the server
can continue sending the same etag but different last-modified)
that's an extreme edge case you're talking about there.  i was just thinking
that you could boil things down to simply:

  equals = (entity == other.entity &&
            lastModified == other.lastModified &&
            size == other.size);

i also don't understand why you allow one or either of the lastmodified values
to be LL_MaxInt.  it seems that you are allowing one or the other to be
undefined, and you are for some reason treating defined and undefined as
equivalent.  why?

same thing seems to be happening for the file size comparisons.  i think you can
assume that the same values will be set for the same document, so defined and
undefined should be treated as not equal to one another.
note to self: POST requests should probably not be resumable and GetEntityID
should return null, otherwise the postdata must be stored...

darin: hm... probably "no good reason"... I was thinking that the caller of
AsyncOpenAt might not have all of ETag,LastModified,Size; however the server
might send them so the entityid should be allowed to match also then. but my use
case for that was mostly "TestProtocols -resume 123 -etag abc url"

probably your suggestion is indeed better. I'll make that change after you
finish reviewing.
Attached patch patch v2 (obsolete) — Splinter Review
so I decided to make a new patch even though you are not done with your review

this addresses the comments previously in the bug.
Especially it implements NS_ERROR_NOT_RESUMABLE in a (imo) better way: It calls
Cancel(NS_ERROR_NOT_RESUMABLE) and then calls CallOnStartRequest(), so that
also OSR is sent for non-resumable requests.
Attachment #136505 - Attachment is obsolete: true
Comment on attachment 136540 [details] [diff] [review]
patch v2

I'll change the following:
+static const char* sWeekdays[] = {
+static const char* sMonths[] = {

to:
+static const char sWeekdays[][4] = {
+static const char* sMonths[][4] = {
to save size & be faster (per drepper)
Comment on attachment 136540 [details] [diff] [review]
patch v2

>Index: base/public/nsIResumableEntityID.idl

>     /** Entity, may be empty. This is meant to hold the E-Tag for http */
>+    attribute ACString entityTag;

"The entity tag is a strong identifier specified by the protocol.  For HTTP,
 this attribute corresponds to the value of the ETag response header.  This
 attribute may be empty."


>Index: base/src/nsResumableEntityID.cpp

> NS_IMETHODIMP
> nsResumableEntityID::GetSize(PRUint32 *aSize) {
>+    if (mSize == PR_UINT32_MAX)
>+        return NS_ERROR_NOT_AVAILABLE;
>     *aSize = mSize;
>     return NS_OK;
> }

why do we want to do this?  might it be better to simply state
in the interface that PR_UINT32_MAX is a reserved value meaning
unspecified?


> NS_IMETHODIMP
> nsResumableEntityID::GetLastModified(PRTime *aLastModified) {
>+    if (LL_EQ(mLastModified, LL_MaxInt()))
>+        return NS_ERROR_NOT_AVAILABLE;
>     *aLastModified = mLastModified;
>     return NS_OK;
> }

hmm... maybe use time zero to indicate unspecified last modified
time?  either way, my same question applies to this function...


>+NS_IMETHODIMP
> nsResumableEntityID::Equals(nsIResumableEntityID *other, PRBool *ret) {
>     PRUint32 size;
>     PRInt64 lastMod;
>+    nsCAutoString entityTag;
> 
>     nsresult rv = other->GetSize(&size);
>+    if (NS_FAILED(rv))
>+        size = PR_UINT32_MAX;
> 
>     rv = other->GetLastModified(&lastMod);
>+    if (NS_FAILED(rv))
>+        lastMod = LL_MaxInt();
>+
>+    rv = other->GetEntityTag(entityTag);
>+    if (NS_FAILED(rv))
>+        entityTag.Truncate();

hmm... this is such bloaty code.  it'd be nice if we
could just case |other| to |nsResumableEntityID*| and
then proceed directly.


>Index: protocol/http/src/nsHttpChannel.cpp

>+/** Weekdays, as stored in PRExplodedTime, for use by If-Unmodified-Since
>+ * requests
>+ */
>+static const char* sWeekdays[] = {
>+    "Sun",
>+    "Mon",
>+    "Tue",
>+    "Wed",
>+    "Thu",
>+    "Fri",
>+    "Sat"
>+};
>+
>+static const char* sMonths[] = {
>+    "Jan",
>+    "Feb",
>+    "Mar",
>+    "Apr",
>+    "May",
>+    "Jun",
>+    "Jul",
>+    "Aug",
>+    "Sep",
>+    "Oct",
>+    "Nov",
>+    "Dec"
>+};

>+                char datebuf[32];
>+                PR_snprintf(datebuf, sizeof(datebuf),
>+                            "%s, %02d %s %04d %02d:%02d:%02d GMT",
>+                            sWeekdays[exploded.tm_wday],
>+                            exploded.tm_mday,
>+                            sMonths[exploded.tm_month],
>+                            exploded.tm_year,
>+                            exploded.tm_hour,
>+                            exploded.tm_min,
>+                            exploded.tm_sec);

i understand why you are using this method of generating a date.
PR_FormatTime can only be used if we are in the right locale.
so, maybe the code for this date formating should be put someplace
more generic.  perhaps a helper function in xpcom/ds/nsTime.h, or
maybe it could just be moved into nsHttp.{h,cpp} if we think this
is HTTP specific.  <= i'd be fine with this as a first step.


>+    if (mResuming) {
>+        char buf[32];
>+        PR_snprintf(buf, sizeof(buf), "bytes=%u-", mStartPos);
>+        mRequestHead.SetHeader(nsHttp::Range, nsDependentCString(buf));
>+
>+        if (mEntityID) {
>+            // Also, we want an error if this resource changed in the meantime
>+            nsCAutoString entityTag;
>+            rv = mEntityID->GetEntityTag(entityTag);
>+            if (NS_SUCCEEDED(rv) && !entityTag.IsEmpty()) {
>+                mRequestHead.SetHeader(nsHttp::If_Match, entityTag);
>+            }
>+
>+            PRTime lastMod;
>+            rv = mEntityID->GetLastModified(&lastMod);
>+            if (NS_SUCCEEDED(rv)) {
>+                PRExplodedTime exploded;
>+                PR_ExplodeTime(lastMod, PR_GMTParameters, &exploded);
>+
>+                char datebuf[32];
>+                PR_snprintf(datebuf, sizeof(datebuf),
>+                            "%s, %02d %s %04d %02d:%02d:%02d GMT",
>+                            sWeekdays[exploded.tm_wday],
>+                            exploded.tm_mday,
>+                            sMonths[exploded.tm_month],
>+                            exploded.tm_year,
>+                            exploded.tm_hour,
>+                            exploded.tm_min,
>+                            exploded.tm_sec);
>+                mRequestHead.SetHeader(nsHttp::If_Unmodified_Since,
>+                                       nsDependentCString(datebuf));
>+
>+            }
>+        }
>+    }

some more thoughts... why aren't you using the HTTP/1.1 header If-Range?  we
should really only support resuming for HTTP/1.1 servers that explicitly accept
byte range requests.  using the If-Range request seems best.  that's what we do
for the byte range requests used to complete a partial cache entry.  also, if
you use If-Range then you either put your ETag in the If-Range header or if you
don't have an ETag, then you put the value of the Last-Modified header there. 
That way, you only send the better of the two validators.

furthermore, this business of converting from a Last-modified header string to
a PRTime and then back to a Last-modified header could be avoided if
nsResumableEntityID simply stored the Last-modified value as a string.	There
is an issue with that:

some servers send non-standard date strings in Last-Modified headers and will
understand if you give them the same string in a If-Modified-Since header.  we
purposefully avoid converting from string to PRTime back to string to deal with
this problem.  we should if possible do the same here.

note: we have to anyways serialize the resumable entity id to disk, so what's
the harm in keeping it as a string?  perhaps our GUI will want to inspect the
last-modified value?


>+    case 412:
>+        if (mResuming) {
>+            Cancel(NS_ERROR_NOT_RESUMABLE);
>+            rv = CallOnStartRequest();
>+            break;
>+        }
>+        // fall through
>     default:
>         CloseCacheEntry(NS_ERROR_ABORT);
>         rv = ProcessNormal();
>         break;
>     }

hmm... i'm not sure i like this special handling for 412 responses.  why not
show the error page?  the consumer can already check the response status using
nsIHttpChannel.responseStatus.	that's how all other error pages are handled,
and i think our download manager code / webbrowserpersist code inspects that
value.

then again, i see that you are doing this to support the NS_ERROR_NOT_RESUMABLE
API.  /sigh/  i want to make changes to
nsIResumableChannel anyways... it makes little sense to have an AsyncOpenAt
method when we don't have an OpenAt method.  the
startPos and entityID stuff should really be configured on nsIResumableChannel
prior to the normal AsyncOpen/Open calls, with those parameters changing the
behavior of AsyncOpen/Open.  that would be way more consistent with the rest of
the derivative channel interfaces (see for example nsICachingChannel).

maybe those API changes can be put off to another bug.	in that case, i'm okay
with your handling of 412 here.


> nsresult
> nsHttpChannel::OpenCacheEntry(PRBool offline, PRBool *delayed)
> {
>+    // XXX For now, no cache when resuming
>+    if (mResuming)
>+        return NS_OK;
>+
>     nsresult rv;
> 
>     *delayed = PR_FALSE;

maybe push this early return after we assign the default value
for the out-param.  yes, i know .. it looks like the caller 
pre-assigns delayed to FALSE.


>+NS_IMETHODIMP
>+nsHttpChannel::GetEntityID(nsIResumableEntityID** aEntityID)
>+{
>+    // Don't return an entity ID for HTTP/1.0 servers
>+    if (mResponseHead && (mResponseHead->Version() != NS_HTTP_VERSION_1_1)) {
>+        *aEntityID = nsnull;
>+        return NS_OK;
>+    }

ah... so you are assuming that the server cannot be HTTP/1.0!  (please
test for version < NS_HTTP_VERSION_1_1)

so you can use If-Range!


>+    nsresult rv = CallCreateInstance(NS_RESUMABLEENTITYID_CONTRACTID, aEntityID);
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    if (mResponseHead) {
>+        const char* etag = mResponseHead->PeekHeader(nsHttp::ETag);
>+        if (etag)
>+            (*aEntityID)->SetEntityTag(nsDependentCString(etag));
>+        (*aEntityID)->SetSize(mResponseHead->LengthTotal());
>+        PRTime time;
>+        rv = mResponseHead->GetLastModifiedPRTime(&time);
>+        if (NS_SUCCEEDED(rv))
>+            (*aEntityID)->SetLastModified(time);
>+    }

i'm not a fan of this (*aEntityID) stuff... just use an extra temporary
variable.  it doesn't cost you anything.

why is this code not using NS_NewResumableEntityID?


i have not carefully reviewed the changes in nsHttpResponseHead b/c
i think that some of what i've said here may influence that code.
(In reply to comment #13)
> why do we want to do this?  might it be better to simply state
> in the interface that PR_UINT32_MAX is a reserved value meaning
> unspecified?

What would be the advantage?

> hmm... maybe use time zero to indicate unspecified last modified
> time?  either way, my same question applies to this function...

as above, I don't think that that improves anything... and it forces the caller
to think about how an unspecified time is represented, there are at least two
reasonable possibilities (LL_MaxInt and 0).

> hmm... this is such bloaty code.  it'd be nice if we
> could just case |other| to |nsResumableEntityID*| and
> then proceed directly.

I don't think I can assume that... nothing prevents someone from implementing
nsIResumableEntityID himself, well except common sense of course.

> i understand why you are using this method of generating a date.
> PR_FormatTime can only be used if we are in the right locale.

exactly.

> so, maybe the code for this date formating should be put someplace
> more generic.  perhaps a helper function in xpcom/ds/nsTime.h, or
> maybe it could just be moved into nsHttp.{h,cpp} if we think this
> is HTTP specific.  <= i'd be fine with this as a first step.

ok, I'll move it to nsHttp.h. actually see below.

> some more thoughts... why aren't you using the HTTP/1.1 header If-Range?

If-Range sends me the complete file when it can't send me the partial file. I
don't want that, I really want the request to fail if I can't get the partial
file... (required by the nsIResumableChannel API and it's also the sensible
thing to do. imo).

> we
> should really only support resuming for HTTP/1.1 servers that explicitly accept
> byte range requests. 

the patch only supports resuming from HTTP/1.1 servers... as those may ignore a
range header, I should probably send a failure code when I get 200 OK instead of
206 Partial Content...

> using the If-Range request seems best.  that's what we do
> for the byte range requests used to complete a partial cache entry.  also, if
> you use If-Range then you either put your ETag in the If-Range header or if you
> don't have an ETag, then you put the value of the Last-Modified header there. 
> That way, you only send the better of the two validators.

yeah, but while that makes sense for  the "complete partial cache entry" use
case, it doesn't make sense for the use case where I want the partial data and
an error if that's not possible.

> furthermore, this business of converting from a Last-modified header string to
> a PRTime and then back to a Last-modified header could be avoided if
> nsResumableEntityID simply stored the Last-modified value as a string.

Hm, that's a good point. And that would allow me to drop the date conversion
function. Maybe it simplifies the FTP resuming stuff as well.

>	There
> is an issue with that:

that seems to be an advantage of storing the string, not an issue with it?

> note: we have to anyways serialize the resumable entity id to disk, so what's
> the harm in keeping it as a string?  perhaps our GUI will want to inspect the
> last-modified value?

hmm. it already does, but not via nsIResumableEntityID (page info)... looks like
it just gets the HTTP header.

> then again, i see that you are doing this to support the NS_ERROR_NOT_RESUMABLE
> API. 

indeed.

> /sigh/  i want to make changes to
> nsIResumableChannel anyways... it makes little sense to have an AsyncOpenAt
> method when we don't have an OpenAt method.  the
> startPos and entityID stuff should really be configured on nsIResumableChannel
> prior to the normal AsyncOpen/Open calls, with those parameters changing the
> behavior of AsyncOpen/Open.  that would be way more consistent with the rest of
> the derivative channel interfaces (see for example nsICachingChannel).

But that would not affect the NS_ERROR_NOT_RESUMABLE part, or not necessarily. I
like it that no matter whether this is HTTP or FTP users can easily find out
whether resuming is supported. it seems wrong to me to force users to know about
HTTP response codes.

> maybe those API changes can be put off to another bug.	in that case, i'm okay
> with your handling of 412 here.

bug 227057 comment 3 indicates that that bug could be used for those api changes.

> maybe push this early return after we assign the default value
> for the out-param.  yes, i know .. it looks like the caller 
> pre-assigns delayed to FALSE.

good idea, will do.

> ah... so you are assuming that the server cannot be HTTP/1.0!  (please
> test for version < NS_HTTP_VERSION_1_1)

Nah, I'm assuming HTTP/1.0 doesn't support resuming ;)

> so you can use If-Range!

see above...

> why is this code not using NS_NewResumableEntityID?

Hmm, I was not aware of that function. It also needs updating for the new
entityTag attribute...


I'll make a new patch
Attachment #136540 - Flags: review?(darin) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #136540 - Attachment is obsolete: true
>> some more thoughts... why aren't you using the HTTP/1.1 header If-Range?
>
>If-Range sends me the complete file when it can't send me the partial file. I
>don't want that, I really want the request to fail if I can't get the partial
>file... (required by the nsIResumableChannel API and it's also the sensible
>thing to do. imo).

is it really sensible though?  i mean, won't a downloader want to download the
whole thing if it cannot resume?  that said, i see your point.  i was just
hoping to simpify this patch some.

i'd like to see nsIResumableEntityID go away.  it isn't needed.  all we need is
something like S/GetResumableParams on a nsIResumableChannel interface.  then we
could avoid a lot of the XPCOM baggage.  i understand that is just a tangent...
the current interfaces are OK, and it's reasonable to just implement them here.
 i'd just really like to see them change ;-)

>I like it that no matter whether this is HTTP or FTP users can easily find out
>whether resuming is supported. it seems wrong to me to force users to know
>about HTTP response codes.

great point!  how about if nsIResumableChannel::SetResumableParams included a
boolean to control whether we fail or provide the full file, if we cannot give
the listener just the partial file.


note: i haven't looked at your new patch yet.
(In reply to comment #16)
> is it really sensible though?  i mean, won't a downloader want to download the
> whole thing if it cannot resume?  that said, i see your point.  i was just
> hoping to simpify this patch some.

Hmm, consider a download manager. user resumes a download after restarting
mozilla, to find out that the file is not resumable. would he want to overwrite
the partial file? What if the partial file is already somewhat useful and almost
fully downloaded? (consider a video file)

This would probably be handled by showing a dialog box _at the point when it is
known that resuming is unsupported_.

> i'd like to see nsIResumableEntityID go away.  it isn't needed.  all we need is
> something like S/GetResumableParams on a nsIResumableChannel interface.  then we
> could avoid a lot of the XPCOM baggage.  i understand that is just a tangent...

Interesting, I was thinking the same the other day :)

Now that 1.7beta is effectively missed there is much time :) Should the api
changes done here or in the bug mentioned a few comments up? I don't really care
much at this point...

> great point!  how about if nsIResumableChannel::SetResumableParams included a
> boolean to control whether we fail or provide the full file, if we cannot give
> the listener just the partial file.

hmm, yes, that would be helpful if callers want that behaviour... is it worth
the extra code? If we do this, the status code on the channel should probably be
a different successcode during OnStartRequest.

Also note that not all protocols support such behaviour... FTP would have to
emulate it, I'm pretty sure. hm...
ok, so maybe the failover to downloading the full file is overkill.  again, i
was only suggesting it in the hopes of avoiding the extra code.

yeah, let's come up with a better API for range requests.  (btw,
nsIResumableChannel is not the best name either.)

nsIByteRangeRequest is an interesting interface.  currently, it is only used by
the multipart mixed stream converter.  i wonder if we want to play with that some.
Comment on attachment 143154 [details] [diff] [review]
patch v3

>Index: base/src/nsResumableEntityID.cpp

>+    *ret = mEntityTag.Equals(entityTag) && lastMod.Equals(mLastModified) &&
>+           (mSize == size);

add a comment that we assume the last-modified string value would
have been generated by the server in exactly the same way (same
timezone, same format, etc.)


>Index: protocol/ftp/src/nsFtpConnectionThread.cpp

>-            const char* date = mResponseMsg.get();
>-            PRExplodedTime exp;
>-            exp.tm_year = (date[0]-'0')*1000 + (date[1]-'0')*100 +
>-                (date[2]-'0')*10 + (date[3]-'0');
>-            exp.tm_month = (date[4]-'0')*10 + (date[5]-'0');
>-            exp.tm_mday = (date[6]-'0')*10 + (date[7]-'0');
>-            exp.tm_hour = (date[8]-'0')*10 + (date[9]-'0');
>-            exp.tm_min = (date[10]-'0')*10 + (date[11]-'0');
>-            exp.tm_sec = (date[12]-'0')*10 + (date[13]-'0');
>-            exp.tm_usec = 0;
>-            exp.tm_wday = 0;
>-            exp.tm_yday = 0;
>-            exp.tm_params.tp_gmt_offset = 0;
>-            exp.tm_params.tp_dst_offset = 0;
>-            mModTime = PR_ImplodeTime(&exp);
>+            mModTime = mResponseMsg;

nice!!


>Index: protocol/http/src/nsHttpChannel.cpp

>- *   Darin Fisher <darin@netscape.com> (original author)
>+ *   Darin Fisher <darin@meer.net> (original author)

thanks!


>+        // Don't allow resuming when cache must be used
>+        if (mResuming && (mLoadFlags & LOAD_ONLY_FROM_CACHE)) {
>+            LOG(("Resuming from cache is not supported yet"));
>+            return NS_ERROR_NOT_IMPLEMENTED;
>+        }

if we are resuming, then we should probably also set the INHIBIT_CACHING
load flag.  i.e., we don't want to write to the cache if we are downloading
a partial document.

whenever LOAD_ONLY_FROM_CACHE is specified, we are supposed to fail the
channel with NS_ERROR_DOCUMENT_NOT_CACHED.  maybe you should do that here
instead of returning NS_ERROR_NOT_IMPLEMENTED.


> nsHttpChannel::OpenCacheEntry(PRBool offline, PRBool *delayed)
...
>+    // XXX For now, no cache when resuming
>+    if (mResuming)
>+        return NS_OK;

setting INHIBIT_CACHING when mResuming is true avoids having to
add this code.	(make sure you update your tree... i only just
added support for INHIBIT_CACHING earlier this week!)


> class nsHttpChannel : public nsIHttpChannel
...
>                     , public nsITransportEventSink
>+		    , public nsIResumableChannel
> {

nit: fix alignment.


>Index: protocol/http/src/nsHttpResponseHead.h

>+    /**
>+     * Total length of the file, i.e. not only the length of the requested part
>+     */
>+    PRInt32               LengthTotal();

i'm not very happy with this name.  i'd like something more descriptive
if possible.  how about "TotalEntitySize()" ... "entity" helps tie this
to the full document.  i'd also add to the comment that this is the 
size of the full document whereas ContentLength only reports the length
of the response body.  highlight the fact that for a range request the
length of the response body may be less than the size of the full 
document (or "total entity size").  some text like that would be nice :)


r=darin with those nits picked.
Attachment #143154 - Flags: review?(darin) → review+
let's make the rest of the proposed API changes in a separate bug.
(In reply to comment #19)
> setting INHIBIT_CACHING when mResuming is true avoids having to
> add this code.	(make sure you update your tree... i only just
> added support for INHIBIT_CACHING earlier this week!)

It turns out that I can remove this code anyway, as a few lines down there's a
check if the Range header is set, and this function just returns in that case.

about to attach new patch.
Attachment #143154 - Attachment is obsolete: true
Attachment #143764 - Flags: superreview?(bzbarsky)
biesi, I'll try to get to this tomorrow, but if that doesn't happen it'll be
three weeks or so before I'm maybe in a position to do reviews again....
Target Milestone: mozilla1.7alpha → mozilla1.8alpha
Comment on attachment 143764 [details] [diff] [review]
patch v4

>Index: base/public/nsIResumableEntityID.idl

>     /** Size of the entity, -1 if unknown */
>     attribute unsigned long size;

>Index: base/src/nsResumableEntityID.cpp

> nsResumableEntityID::GetSize(PRUint32 *aSize) {
>+    if (mSize == PR_UINT32_MAX)
>+        return NS_ERROR_NOT_AVAILABLE;

The idl should probably reflect that....

Other than that, looks excellent.  sr=bzbarsky
Attachment #143764 - Flags: superreview?(bzbarsky) → superreview+
thanks, checked in for 1.8alpha with changes from comment 24 made.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: