The default bug view has changed. See this FAQ.

nsIWebBrowserPersist should support resume

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Embedding: APIs
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: Blake Ross, Assigned: Tom Germeau)

Tracking

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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...
TestProtocols has a hackish example of using this interface. Currently only ftp
supports it.
(Reporter)

Comment 2

15 years ago
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?
(Reporter)

Updated

15 years ago
Blocks: 131780

Comment 3

15 years ago
Marking future. Resume would be nice but I will have to investigate it further.
(Reporter)

Comment 4

15 years ago
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.

Updated

15 years ago
QA Contact: mdunn → depstein

Comment 5

15 years ago
changed qa contact to Dharma. Web Browser persist is his area.
QA Contact: depstein → dsirnapalli

Comment 6

15 years ago
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.
That looks about right, from a quick glance through.

Comment 8

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

Comment 9

15 years ago
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.
Attachment #77070 - Attachment is obsolete: true
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

15 years ago
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.
Target Milestone: --- → mozilla1.1alpha
(Reporter)

Comment 12

15 years ago
How's this coming?

Comment 13

15 years ago
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

15 years ago
Setting target milestone to 'Future' since 'mozilla1.1alpha' has already passed.
Target Milestone: mozilla1.1alpha → Future

Comment 15

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

Comment 16

15 years ago
Is there a problem checking this in now ? It was more or less finished in April...
(Assignee)

Comment 17

10 years ago
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.
Attachment #265517 - Flags: review?

Comment 18

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

(Assignee)

Updated

10 years ago
Attachment #265517 - Flags: review? → review?(cbiesinger)

Comment 19

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

Updated

10 years ago
Assignee: adamlock → tom.germeau
Flags: blocking1.9?
Target Milestone: Future → mozilla1.9alpha6
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".
Attachment #265517 - Flags: review?(cbiesinger) → review+

Updated

10 years ago
Blocks: 377243
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
Attachment #265517 - Flags: superreview?(jst)
(Assignee)

Comment 22

10 years ago
Created attachment 270146 [details] [diff] [review]
append_to_file with biesi's comments applied
Attachment #265517 - Attachment is obsolete: true
Attachment #270146 - Flags: review?(cbiesinger)
Attachment #265517 - Flags: superreview?(jst)
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.

Updated

10 years ago
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Attachment #270146 - Flags: review?(cbiesinger) → review+

Updated

10 years ago
Attachment #270146 - Flags: superreview+

Updated

10 years ago
Keywords: checkin-needed

Comment 24

10 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite?

Updated

10 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.