129921 - nsIWebBrowserPersist should support resume

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Blake Ross]

nsIResumableChannel was added recently to facilitate cross-session resuming
(something of a replacement for the hackish
nsIRequest::Pause/nsIRequest::Resume).  Since nsIWebBrowserPersist takes care of
the downloading, I think it should offer a resume method that takes the url and
the offset in bytes as parameters.  That would really help us out in download
manager land...

Comment 1

[Bradley Baetz (:bbaetz)]

TestProtocols has a hackish example of using this interface. Currently only ftp
supports it.

Comment 2

[Blake Ross]

Adam, can you comment on this?  I think this should be really easy for you since
you know the code well.  With minimal hacking, I think I've almost hooked it up,
I'm just running into one problem that I can't solve.  I added an offset
parameter to saveURI and then called asyncOpenAt instead of asyncOpen if it was
non-zero.  Is there a chance you can get this done by 1.0?

Comment 3

[Adam Lock]

Marking future. Resume would be nice but I will have to investigate it further.

Comment 4

[Blake Ross]

This makes the download manager next to useless.  According to bbaetz and
TestProtocols.cpp, this is pretty easy to hookup. It entails calling AsyncOpenAt
on channels that implement nsIResumableChannel, rather than AsyncOpen.

Comment 5

[David Epstein]

changed qa contact to Dharma. Web Browser persist is his area.

Comment 6

[Adam Lock]

Attachment: Work in progress

Patch demonstrates how resume might be supported. Patch is still work in
progress. Still need to figure my way around the file stream interfaces to open
an output stream seeked to the right place.

Comment 7

[Bradley Baetz (:bbaetz)]

That looks about right, from a quick glance through.

Comment 8

[Adam Lock]

Sample URL points to a file on an FTP site that supports resume.

Comment 9

[Adam Lock]

Attachment: A better patch

A functioning patch. Tested it with the URL above, cancelling and resuming
several times and file downloaded correctly.

The UI needs to be updated and there are also some issues with the progress
notification. Netwerk should give progress notifications that start at 30% or
wherever we're resuming from but it starts from 0.

I don't know of an ftp site that doesn't support resume so I have only
simulated the condition by jumping into the right code in OnStartRequest to
handle an NS_ERROR_NOT_RESUMABLE error. JS will need to catch these errors
because it doesn't at the moment.

Comment 10

[Bradley Baetz (:bbaetz)]

You need to keep track of how many bytes you have - just grab the size of the
file you are resuming the download to.

You should also get the entity ID from the nsIResumableChannel, and the d/l
manager should store that info, and then pass it in to asyncOpenAt. That will
allow you to pop up an alert if the new data isn't the same as the old one.

Try downoading a directory - those aren't resumable. Ignore any assertions you
get from ftp about that - resuming directory downloads isn't something which is
supported.

Comment 11

[Adam Lock]

I shouldn't need to expose any of the entity stuff since the download manager
will be notified via OnStateChange call when I receive my OnStartRequest. It can
save the stuff for itself and popup a warning there.

Re my progress problems. If I resume 30k into a 100k file, netwerk sends me
OnProgress notifications that start from 0k and go up to 70k. This looks really
odd. It should start from 30k and go up to 100k. While I could add on the
missing 30k for myself, it requires me to assume that progress & progressmax
args in OnProgress actually represent number of bytes. They might for one
protocol but another might use them for percentage complete or some other
measure of completeness. So it would be better and safer if progress
notifications were correct when they got to me.

I will use the downloading a dir trick to test out the resume error code in
OnStartRequest.

Comment 12

[Blake Ross]

How's this coming?

Comment 13

[Adam Lock]

Patch development is on hold while 1.0 schedule is so tight. My last patch is
more or less complete except for the entity checking. After review/checkin a new
bug will have to be raised against the download manager to actually use this
functionality.

Comment 14

[Juan José Mata]

Setting target milestone to 'Future' since 'mozilla1.1alpha' has already passed.

Comment 15

[Arne Falk]

Is there a problem checking this in now ? It was more or less finished in April...

Comment 16

[Arne Falk]

Is there a problem checking this in now ? It was more or less finished in April...

Comment 17

[Tom Germeau]

Attachment: Add flag to nsIWebBrowserPersist making it possible to append to a file

Inspired by http://developer.mozilla.org/en/docs/Implementing_Download_Resuming

With this patch one can create a nsIResumableChannel, optionally handle entityID and resumeAt and ask the nsIWebBrowserPersist to append instead of overwrite.

Comment 18

[Ryan Jones-Ward [:sciguyryan]]

Tom: You need to request a reviewer, bz, biesi or bsmedberg could probably review this for you.

Comment 19

[Mark Smith [:mcs]]

I'm ok with the new patch (attachment 265517 [details] [diff] [review]) r=brade

Comment 20

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

Comment on attachment 265517 [details] [diff] [review]
Add flag to nsIWebBrowserPersist making it possible to append to a file

you don't need to change the IID when you're just adding a new constant

also, you should document that this flag will only be used when saving to a local file (as opposed to a URI). and perhaps use "target file" instead of "target stream".

Comment 21

[Srirang G Doddihal (Brahmana)]

Comment on attachment 265517 [details] [diff] [review]
Add flag to nsIWebBrowserPersist making it possible to append to a file

jst kindly review this patch as it is blocking my work. (Bug 377243)

Regards
Brahmana

Comment 22

[Tom Germeau]

Attachment: append_to_file with biesi's comments applied

Comment 23

[Jonathan Watt [:jwatt]]

Tom, if your reviewer marks your patch as r+ but asks you to make a few changes you don't need to ask them to review those changes. If they had wanted to review your changes they would have marked it r-. (You can _choose_ to re-request review if you're not sure your changes meet their request of course.) What you do need before this patch can be checked in is sr. I'm not sure if biesi could also give you sr in this simple case or if you need a second reviewer.

Comment 24

[Michael Wu [:mwu]]

Checking in embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl;
/cvsroot/mozilla/embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl,v  <--  nsIWebBrowserPersist.idl
new revision: 1.18; previous revision: 1.17
done
Checking in embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp;
/cvsroot/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp,v  <--  nsWebBrowserPersist.cpp
new revision: 1.129; previous revision: 1.128
done