Last Comment Bug 129921 - nsIWebBrowserPersist should support resume
: nsIWebBrowserPersist should support resume
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Embedding: APIs (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9alpha8
Assigned To: Tom Germeau
: dsirnapalli
:
Mentors:
ftp://ftp.esat.net/mirrors/ftp.mozill...
Depends on:
Blocks: 131780 377243
  Show dependency treegraph
 
Reported: 2002-03-09 23:46 PST by Blake Ross
Modified: 2007-09-09 12:08 PDT (History)
20 users (show)
sdwilsh: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress (9.25 KB, patch)
2002-04-01 12:08 PST, Adam Lock
no flags Details | Diff | Splinter Review
A better patch (10.47 KB, patch)
2002-04-02 09:45 PST, Adam Lock
no flags Details | Diff | Splinter Review
Add flag to nsIWebBrowserPersist making it possible to append to a file (1.41 KB, patch)
2007-05-21 09:24 PDT, Tom Germeau
cbiesinger: review+
Details | Diff | Splinter Review
append_to_file with biesi's comments applied (1.23 KB, patch)
2007-06-28 01:15 PDT, Tom Germeau
cbiesinger: review+
jst: superreview+
Details | Diff | Splinter Review

Description Blake Ross 2002-03-09 23:46:56 PST
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) 2002-03-17 15:26:55 PST
TestProtocols has a hackish example of using this interface. Currently only ftp
supports it.
Comment 2 Blake Ross 2002-03-19 13:08:26 PST
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 2002-03-29 11:01:22 PST
Marking future. Resume would be nice but I will have to investigate it further.
Comment 4 Blake Ross 2002-03-31 15:23:15 PST
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 2002-04-01 10:57:49 PST
changed qa contact to Dharma. Web Browser persist is his area.
Comment 6 Adam Lock 2002-04-01 12:08:32 PST
Created attachment 77070 [details] [diff] [review]
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) 2002-04-01 22:00:59 PST
That looks about right, from a quick glance through.
Comment 8 Adam Lock 2002-04-02 07:58:03 PST
Sample URL points to a file on an FTP site that supports resume.
Comment 9 Adam Lock 2002-04-02 09:45:33 PST
Created attachment 77247 [details] [diff] [review]
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) 2002-04-02 16:54:08 PST
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 2002-04-03 06:04:03 PST
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 2002-04-14 15:13:29 PDT
How's this coming?
Comment 13 Adam Lock 2002-04-16 07:40:59 PDT
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 2002-10-22 22:59:19 PDT
Setting target milestone to 'Future' since 'mozilla1.1alpha' has already passed.
Comment 15 Arne Falk 2002-11-22 05:24:54 PST
Is there a problem checking this in now ? It was more or less finished in April...
Comment 16 Arne Falk 2002-11-22 05:26:08 PST
Is there a problem checking this in now ? It was more or less finished in April...
Comment 17 Tom Germeau 2007-05-21 09:24:08 PDT
Created attachment 265517 [details] [diff] [review]
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 2007-05-21 09:29:24 PDT
Tom: You need to request a reviewer, bz, biesi or bsmedberg could probably review this for you.

Comment 19 Mark Smith (:mcs) 2007-05-31 14:00:02 PDT
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) 2007-06-26 11:43:47 PDT
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) 2007-06-26 12:19:31 PDT
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 2007-06-28 01:15:47 PDT
Created attachment 270146 [details] [diff] [review]
append_to_file with biesi's comments applied
Comment 23 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-06-28 05:11:04 PDT
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] 2007-07-13 10:31:34 PDT
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

Note You need to log in before you can comment on or make changes to this bug.