Closed Bug 377243 Opened 17 years ago Closed 17 years ago

[SoC] Implement Download Resume

Categories

(Toolkit :: Downloads API, defect)

Product:
Toolkit
Component:
Downloads API
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha8

People

(Reporter: chofmann, Assigned: om.brahmana)

References

Details

Attachments

(2 files, 15 obsolete files)

test database
7.00 KB, application/octet-stream
Details
v1.2
21.19 KB, patch
Details | Diff | Splinter Review 
Tracking bug for progress on summer of code project
Just wanted to point out that bug 230870 is the "actual" bug on this. Jeff Walden might be interested in talking to whoever's SoC project this is (since he's the assignee of bug 230870).
I'll follow along, and I can give advice/suggestions if needed, but I'm unlikely to follow too closely, given all the other things that occupy my attention (as evidenced by the date/contents of my last comment in that bug).
Summary: Implement Download Resume → [SoC] Implement Download Resume
OS: Windows XP → All
Hardware: PC → All
Assignee: dmose → om.brahmana
Hello,

I am the student who has taken up this as my SoC project. I am sorry for delayed entry in this discussion. I was discussing few things with my mentor so that I can start of with a proper discussion.

Regards
Brahmana
(In reply to comment #2)
> I'll follow along, and I can give advice/suggestions if needed,

That would be very  much needed and also very nice. :)

> but I'm
> unlikely to follow too closely, given all the other things that occupy my
> attention (as evidenced by the date/contents of my last comment in that bug).
> 

Thank you. :)
Hello,

I have the initial design proposal. It was little big to be posted here as a comment. So I have it on my wiki. Here is the link http://brahmana.wikispaces.com/Design+proposal+1 

Till now I was studying about XPCOM and XPConnect. I also looked at the current XUL definition of the DM window and related JS. I am yet to probe into the concerned interfaces like nsIResumableChannel, nsIFile and others. This would be my next step. (at mxr.mozilla.org)

I have the read your wiki and all looks reasonably good.

However one (important) question as it wasn't clear - if the user closes
Firefox while a download is running and selects Clear Private Data - will that
wipe away the partial download and all associated metadata?
One note:
downloads.rdf already contains a "DownloadState" entry for each download - which indicates that it is: in progress, finished, canceled, paused, failed
so one or both of your flags should probably just use this existing flag, adding more states as needed.

Also:
If there are active downloads and the user closes the browser, I would suggest only alerting and interrupting the user if one of the active downloads doesn't have server support for resuming (assuming this can be determined)  




(In reply to comment #6)
> I have the read your wiki and all looks reasonably good.
> 
> However one (important) question as it wasn't clear - if the user closes
> Firefox while a download is running and selects Clear Private Data

I am not aware of a way of doing this before starting a new session. Can you please elaborate on that? Because when the user just starts browser again he will be asked whether to restore or start a new session. And he will have to go through this step before clearing the private data. Is it not so?

> - will that
> wipe away the partial download and all associated metadata?
> 

Thank you for bringing this in. I had forgotten about the download history being cleared with ctrl+shift+del.

AFAIK when Download History is cleared the contents of the contents of dead downloads are erased. I tried it. Clearing download history when there was an active download. The active download was not removed. Then I tried it with a paused download. At that time the Download History check box in private data clearance window was disabled (though it was being shown as selected) and the paused download was not removed. So I guess things have already been taken care of. 

Since I could still see the entries in the DM window their corresponding entries in the RDF files must also be there unaffected.

Now if you were referring to downloads that maintain authentication data in sessions or some similar data in cookies, then I suppose it is out of the scope of this project. I am not aware of how those things work.

Hope that made sense and was sufficient

Regards
Brahmana
(In reply to comment #7)
> One note:
> downloads.rdf already contains a "DownloadState" entry for each download -
> which indicates that it is: in progress, finished, canceled, paused, failed
> so one or both of your flags should probably just use this existing flag,
> adding more states as needed.
> 
Oh thank you. I am yet to fully find out and understand all the fields of the downloads.rdf file. I ll consider this.

> Also:
> If there are active downloads and the user closes the browser, I would suggest
> only alerting and interrupting the user if one of the active downloads doesn't
> have server support for resuming (assuming this can be determined)  
> 

Yes, from what I know this can be determined and i have proposed to put an icon to indicate that in the DM window. Currently the alert comes up for all the downloads. I ll try and find out how this can be limited to ones without resume support.

Regards
Brahmana
From the design proposal on wikispaces:
> The next time the browser is restarted, then the user is alerted about
> the downloads that were stored when the previous session ended (may be
> popping a dialog with a simple list of the auto-restore downloads) and
> he may choose to continue or cancel them.

Wouldn't it be less of a hassle to just go ahead and resume all auto-restore downloads assuming that if the user didn't want them to resume he would have or will pause them?  I think that would simplify the whole experience.
(In reply to comment #10)
> From the design proposal on wikispaces:
> > The next time the browser is restarted, then the user is alerted about
> > the downloads that were stored when the previous session ended (may be
> > popping a dialog with a simple list of the auto-restore downloads) and
> > he may choose to continue or cancel them.
> 
> Wouldn't it be less of a hassle to just go ahead and resume all auto-restore
> downloads assuming that if the user didn't want them to resume he would have or
> will pause them?  I think that would simplify the whole experience.
> 

Makes sense. 

But I felt doing it your way would be like the browser monitoring the things and will appear as if the application taking the control away from the user. Just thought of not keeping the user in dark. (In cases like the user having completed the same download by some other means). If need be we can have a preference setting just the way we can have DM window appear/not appear when a download starts.

Regards
Brahmana
have you considered using wiki.mozilla.org instead of wikispaces for the design spec?
(In reply to comment #12)
> have you considered using wiki.mozilla.org instead of wikispaces for the design
> spec?
> 

As a matter of fact i did and ended up with two pages. The link for one is present at my home page on the wiki. http://wiki.mozilla.org/User:Brahmana . The other one is at http://wiki.mozilla.org/Design_Proposal_1

Moreover I found some of the features of wikispaces very handy which i did not get here. May be they are available here also and I need to figure them out.
(In reply to comment #13)
> As a matter of fact i did and ended up with two pages. The link for one is
> present at my home page on the wiki. http://wiki.mozilla.org/User:Brahmana .
> The other one is at http://wiki.mozilla.org/Design_Proposal_1

I'd suggest deleting the second page and treating the first one as the authoritative one.  I'll have more feedback on some of the design issues here on Monday.
(In reply to comment #9)
> (In reply to comment #7)
> > One note:
> > downloads.rdf already contains a "DownloadState" entry for each download -
> > which indicates that it is: in progress, finished, canceled, paused, failed
> > so one or both of your flags should probably just use this existing flag,
> > adding more states as needed.
> > 
> Oh thank you. I am yet to fully find out and understand all the fields of the
> downloads.rdf file. I ll consider this.

I'd suggest using a separate flag, as this ensures that if someone runs an older version of Firefox on a profile that has already been used with a newer version, there's no risk of the old version getting confused by an unrecognized value for the existing flag.

(In reply to comment #7)

> Also:
> If there are active downloads and the user closes the browser, I would suggest
> only alerting and interrupting the user if one of the active downloads doesn't
> have server support for resuming (assuming this can be determined)  

I have mixed feelings about whether this is the right UI.  In the case where the user has forgotten they have a download running, this seems like the wrong behavior.  It may also be the case that they are currently on a cheap or high-bandwidth connection, and the next time they connect, they won't be.  A reasonable argument could be made, however, that those are the minority cases.  faaborg, mconnor, what are your thoughts here?

(In reply to comment #11)
> > Wouldn't it be less of a hassle to just go ahead and resume all auto-restore
> > downloads assuming that if the user didn't want them to resume he would have or
> > will pause them?  I think that would simplify the whole experience.
> > 
> 
> Makes sense. 
> 
> But I felt doing it your way would be like the browser monitoring the things
> and will appear as if the application taking the control away from the user.

This doesn't feel like monitoring to me; it's merely continuing what it was doing when you last quit.

> Just thought of not keeping the user in dark. (In cases like the user having
> completed the same download by some other means). If need be we can have a
> preference setting just the way we can have DM window appear/not appear when a
> download starts.

I don't even know that this needs to be a pref.  Having the DM window appear if an auto-restart happens sounds like the right thing to me.

(In reply to comment #14)
> (In reply to comment #13)
> > As a matter of fact i did and ended up with two pages. The link for one is
> > present at my home page on the wiki. http://wiki.mozilla.org/User:Brahmana .
> > The other one is at http://wiki.mozilla.org/Design_Proposal_1
> 
> I'd suggest deleting the second page and treating the first one as the
> authoritative one.  I'll have more feedback on some of the design issues here
> on Monday.

What I really meant there was "let's consider the wiki.mozilla.org" version authoritative and continue working there.



In general, I think the design proposal at <http://wiki.mozilla.org/Brahmana:Design_Proposal_1> as modified by discussion here is a reasonable one.  A couple more thoughts:

As far as indicating in the UI whether a specific download is considered to be resumable, I think the trick here is going to be figuring out how best to express this in a way that doesn't appear as visual clutter.  I suspect the concept of "resumability" isn't likely to be meaningful to a large chunk of our user base, so, on the surface at least, this sounds difficult to me.  

> There can be two ways in which a session can end.
>
> 1) The user ends it manually by closing all the browser windows. 

It's probably worth changing this to "the user exits the browser", since, on Mac, closing all the browser windows does not have this effect.

Finally, as far as adding a new state to RDF, I'd suggest using a string literal for this rather than integers, as it will enhance code and data readability.

Can you fold the comments here back into the design doc, noting where there are still unresolved issues? 
(In reply to comment #15)
> I'd suggest using a separate flag, as this ensures that if someone runs an
> older version of Firefox on a profile that has already been used with a newer
> version, there's no risk of the old version getting confused by an unrecognized
> value for the existing flag.
> 

That sounds reasonable but I'm curious if this is a consideration in other parts of firefox development - places/bookmarks for example.  (i.e. if everything else breaks, having a good DLmgr doesn't help much)
(In reply to comment #16)
> I have mixed feelings about whether this is the right UI.  In the case where
> the user has forgotten they have a download running, this seems like the wrong
> behavior.  It may also be the case that they are currently on a cheap or
> high-bandwidth connection, and the next time they connect, they won't be.  A
> reasonable argument could be made, however, that those are the minority cases. 
> faaborg, mconnor, what are your thoughts here?


> > Just thought of not keeping the user in dark. (In cases like the user having
> > completed the same download by some other means). If need be we can have a
> > preference setting just the way we can have DM window appear/not appear when a
> > download starts.
> 
> I don't even know that this needs to be a pref.  Having the DM window appear if
> an auto-restart happens sounds like the right thing to me.

Please don't hard code the download manager popping up.  If you don't want to add another pref for this auto-restart behavior, at least make it obey the existing pref: "Show the downloads window when downloading a file" (browser.download.manager.showWhenStarting)

Another option might be the sliding alert notification (like the "Downloads complete") but this prob doesn't work on non-Win platforms.  Nice if this obeys the existing pref too - right now called browser.download.manager.showAlertOnComplete

(In reply to comment #15)
> I'd suggest using a separate flag, as this ensures that if someone runs an
> older version of Firefox on a profile that has already been used with a newer
> version, there's no risk of the old version getting confused by an unrecognized
> value for the existing flag.

Hmm, the "paused" value already exists in the older version, does it not?

(In reply to comment #18)
> 
> That sounds reasonable but I'm curious if this is a consideration in other
> parts of firefox development - places/bookmarks for example.  (i.e. if
> everything else breaks, having a good DLmgr doesn't help much)

Discussions are ongoing (cf <http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/bfb141d38f18b886/8df29bacbacf5d52>).
We are, however, trying to be more sensitive about this than we have been in the past; there's a section in the feature requirement template that encourages everyone to think about this as they design.

(In reply to comment #19)
> Please don't hard code the download manager popping up.  If you don't want to
> add another pref for this auto-restart behavior, at least make it obey the
> existing pref: "Show the downloads window when downloading a file"
> (browser.download.manager.showWhenStarting)

This is a different situation in the sense that it's not an immediately preceding action that's causing the download to be happen, so feedback is more necessary, it seems to me.  In what cases do you think users would be likely to have an issue if this were hardcoded?

> Another option might be the sliding alert notification (like the "Downloads
> complete") but this prob doesn't work on non-Win platforms.  Nice if this 
> obeys the existing pref too - right now called
> browser.download.manager.showAlertOnComplete

Actually, I like this strategy a fair bit.  It seems preferable, if we can make it happen on all platforms.  I believe Firefox3 intends to ship with Growl support on Mac, though I don't know whether it will actually include Growl for those users who don't already have it installed.  Not sure what the Linux story is.  Perhaps a reasonable plan would be to do this on platforms that support it, and bring up the download manager on others.

(In reply to comment #20)
> Hmm, the "paused" value already exists in the older version, does it not?

The semantics under discussion here are not so much the "paused" state, but the resumption status: "auto", "manual", or "not supported".  We could live without not supported by actually re-testing the server on restart.  I'll speculate that there is a bit of value gained by having stuff auto-resume on next start if and only if it was stopped as part of quitting, which does require an extra bit, I think.  Here, too, I'd be interested in hearing thoughts from UI folks...


(In reply to comment #11)
...
> But I felt doing it your way would be like the browser monitoring the things
> and will appear as if the application taking the control away from the user.
> Just thought of not keeping the user in dark. ...

I suppose it IS monitoring, but it is monitoring something the user ASKED the computer to do, therefore it is not taking away control, but delivering it.

Keep up the good work everyone!
(In reply to comment #21)

> (In reply to comment #19)
> > Please don't hard code the download manager popping up.  If you don't want to
> > add another pref for this auto-restart behavior, at least make it obey the
> > existing pref: "Show the downloads window when downloading a file"
> > (browser.download.manager.showWhenStarting)
> This is a different situation in the sense that it's not an immediately
> preceding action that's causing the download to be happen, so feedback is more
> necessary, it seems to me.  In what cases do you think users would be likely to
> have an issue if this were hardcoded?

Not everyone uses the download manager window to visualize/manage their downloads.  I happen to use an extension (that I happened to write) called Download Statusbar.  Resuming downloads will already be shown within the browser window so there isn't a need for the DL window to popup.  Others use different extensions that put the downloads in a tab or in a sidebar.  So I think there should be a pref for either the download manager window coming up, or even a sliding alert notification.  

> (In reply to comment #20)
> > Hmm, the "paused" value already exists in the older version, does it not?
> 
> The semantics under discussion here are not so much the "paused" state, but the
> resumption status: "auto", "manual", or "not supported"...

Yes, so to be clear - in the original proposal Brahmana mentioned two flags.  The first for whether a download was "alive" or "dead" and a second for the resumption status "auto", "manual", or "not supported".  Perhaps the first proposed flag is unnecessary because we already have downloadstate.  The second flag could be added.  
(In reply to comment #21)
> (In reply to comment #19)
> > Please don't hard code the download manager popping up.  If you don't
> > want to add another pref for this auto-restart behavior, at least make
> > it obey the existing pref: "Show the downloads window when downloading
> > a file" (browser.download.manager.showWhenStarting)
> 
> This is a different situation in the sense that it's not an immediately
> preceding action that's causing the download to be happen, so feedback
> is more necessary, it seems to me.  In what cases do you think users
> would be likely to have an issue if this were hardcoded?

I agree with Devon that on auto-restart the DM should respect the existing pref: b.d.m.showWhenStarting.  Currently having that option unchecked means the DM appears only when manually opened.  I don't see a reason for auto-restart to override that option.  I have it unchecked specifically to keep the DM from cluttering my desktop unnecessarily.
(In reply to comment #23)
> (In reply to comment #21)
> 
> > (In reply to comment #19)
> > > Please don't hard code the download manager popping up.  If you don't want to
> > > add another pref for this auto-restart behavior, at least make it obey the
> > > existing pref: "Show the downloads window when downloading a file"
> > > (browser.download.manager.showWhenStarting)
> > This is a different situation in the sense that it's not an immediately
> > preceding action that's causing the download to be happen, so feedback is more
> > necessary, it seems to me.  In what cases do you think users would be likely to
> > have an issue if this were hardcoded?
> 
> Not everyone uses the download manager window to visualize/manage their
> downloads.  I happen to use an extension (that I happened to write) called
> Download Statusbar.  Resuming downloads will already be shown within the
> browser window so there isn't a need for the DL window to popup.  Others use
> different extensions that put the downloads in a tab or in a sidebar.  So I
> think there should be a pref for either the download manager window coming up,
> or even a sliding alert notification.  
> 
> > (In reply to comment #20)
> > > Hmm, the "paused" value already exists in the older version, does it not?
> > 
> > The semantics under discussion here are not so much the "paused" state, but the
> > resumption status: "auto", "manual", or "not supported"...
> 
> Yes, so to be clear - in the original proposal Brahmana mentioned two flags. 
> The first for whether a download was "alive" or "dead" and a second for the
> resumption status "auto", "manual", or "not supported".  Perhaps the first
> proposed flag is unnecessary because we already have downloadstate.  The second
> flag could be added.  
>
Yes. You are right. At the time of writing that proposal I was not fully aware of the various entries in the RDF file. I have made necessary changes to the design. 


After much IRC discussion of notification stuff, it seems to me that the right thing here is to use nsIAlertService for notification upon auto-restart, probably with a link to the download manager in the notification.  This should cover everyone except for Mac users who don't have Growl installed.  We should spin off (and nominate as blocking) a separate bug about ensuring that Mac users of Firefox 3 (or Gecko 1.9) have some sort of alert service impl, even if they don't have Growl installed.
>use nsIAlertService for notification upon auto-restart

Sorry for the long delay from the UE team, I agree with Dan.
-ALex
Hello,

I just got a suggestion regarding the proposed design. The suggestion was to disable the [Pause] button or link if there is no server side support for resume instead of having a icon/image. We actually were unable to think of a proper image for that. So this suggestion seems to be apt. 

Comments?

Regards
Brahmana
Even non-resumeable downloads are pausable by limiting them to a neglectable speed, like 1 byte/s. I don't know if something like this is planned, but if it ever gets implemented the design would have to change.
Based on the suggestion I propose having two buttons:
Pause, Play, Pause Disabled, Retry
and
Cancel, Clean up

In order to increase attention that the current download cannot be resumed after restart, the pause icon should not only be "disabled", but having an "red x" or similar stress that pausing isn't possible. A tooltip should explain the meaning of the icon.


Other thoughts:
Since bug 76111 got fixed, it might be useful implementing an "auto-resume after internet disconnect". Happens when the WLAN fails or on 24h disconnect which both are hardly noticeable by users as the internet connection is reestablished automatically, and even IM messengers do auto-reconnect.
No idea if it would be useful for a laptop user switching from cheap/fast LAN to... say some super-expensive connection via mobile phone (UMTS or whatever) though. UI is up to you :)
xeen
Flags: blocking-firefox3?
As per the PRD for Firefox 3, this isn't a blocker, but the progress looks good so let's hope that we can get it in!
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Target Milestone: --- → Firefox 3
Blocks: 381603
Depends on: 129921
Depends on: 385871
Sorry for the delay.

This patch does schema checking and upgrades the DB to version 3 to include the new column for entityID. It puts NULL initially and then in nsIDownload:OnStateChange() fetches the entityID and stores it. 

At resume it creates a new channel and resumes download from the previously cut out point.

********

I am getting a compile error for the entityIDString used at line number 1856. This is the error that I am getting:

error C2248: 'nsACString_internal::nsACString_internal' : cannot access protected member declared in class 'nsACString_internal'

I am trying to clean this up, but no lead yet. I need some help with this.

Regards,
Brahmana.
In the above comment the line number 1856 had this code:

rv = stmt->GetString(0, entityIDString);

with following declarations: 
nsACString entityIDString;
nsCOMPtr<mozIStorageStatement> stmt;

I changed that nsACString entityIDString; to nsAutoString entityIDString; The above mentioned error was resovled. But I am getting an error when I send the same string to nsIResumableChannel::ResumeAt() which is expecting an nsCAutoString I suppose.

How can I get around this?

Regards,
Brahmana.
Comment on attachment 276100 [details] [diff] [review]
This has code to do the schema checking, upgrade the DB and resume using a new channel.

>+	  nsCAutoString entityIDString;
...
>+  nsACString entityIDString;
Is there a reason for not using a nsCAutoString here as well?
Are abstract string classes like nsAString and nsACString supposed to be used for local variables at all?
+		  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+			  "CREATE TEMPORARY TABLE moz_downloads_backup ("
+			  "id INTEGER PRIMARY KEY, "
+			  "name TEXT, "
+			  "source TEXT, "
+			  "target TEXT, "
+			  "startTime INTEGER, "
+			  "endTime INTEGER, "
+			  "state INTEGER"
+			  "entityID TEXT"
+			  ")"));
+		  NS_ENSURE_SUCCESS(rv, rv);

you don't need to create entityID here

+
+		  // Fill up entityID column with NULL value.
+		  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+			  "UPDATE  moz_downloads_backup"
+			  "SET entityID = NULL"));
+		  NS_ENSURE_SUCCESS(rv, rv);

This SQL is useless if you don't create entityID in temp table

+
+		  // Drop the old table
+		  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+			  "DROP TABLE moz_downloads"));
+		  NS_ENSURE_SUCCESS(rv, rv);
+
+		  //Now recreate it with this schema version
+		  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+			  "CREATE  TABLE moz_downloads ("
+			  "id INTEGER PRIMARY KEY, "
+			  "name TEXT, "
+			  "source TEXT, "
+			  "target TEXT, "
+			  "startTime INTEGER, "
+			  "endTime INTEGER, "
+			  "state INTEGER"
+			  "entityID TEXT"
+			  ")"));
+		  NS_ENSURE_SUCCESS(rv, rv);

Use entityID TEXT DEFAULT NULL

as for strings i often see this:

nsCAutoString entityIDString;
stmt->GetUTF8String(0, entityIDString);
(In reply to comment #33)
> (From update of attachment 276100 [details] [diff] [review])
> >+	  nsCAutoString entityIDString;
> ...
> >+  nsACString entityIDString;
> Is there a reason for not using a nsCAutoString here as well?
> Are abstract string classes like nsAString and nsACString supposed to be used
> for local variables at all?
> 

That was in fact my first approach. With that I got this error.

error C2664: 'mozIStorageValueArray::GetString' : cannot convert parameter 2 from 'nsCAutoString' to 'nsAString_internal &'
(In reply to comment #34)
> +                 rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                         "CREATE TEMPORARY TABLE moz_downloads_backup ("
> +                         "id INTEGER PRIMARY KEY, "
> +                         "name TEXT, "
> +                         "source TEXT, "
> +                         "target TEXT, "
> +                         "startTime INTEGER, "
> +                         "endTime INTEGER, "
> +                         "state INTEGER"
> +                         "entityID TEXT"
> +                         ")"));
> +                 NS_ENSURE_SUCCESS(rv, rv);
> 
> you don't need to create entityID here
> 
> +
> +                 // Fill up entityID column with NULL value.
> +                 rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                         "UPDATE  moz_downloads_backup"
> +                         "SET entityID = NULL"));
> +                 NS_ENSURE_SUCCESS(rv, rv);
> 
> This SQL is useless if you don't create entityID in temp table
> 
> +
> +                 // Drop the old table
> +                 rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                         "DROP TABLE moz_downloads"));
> +                 NS_ENSURE_SUCCESS(rv, rv);
> +
> +                 //Now recreate it with this schema version
> +                 rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +                         "CREATE  TABLE moz_downloads ("
> +                         "id INTEGER PRIMARY KEY, "
> +                         "name TEXT, "
> +                         "source TEXT, "
> +                         "target TEXT, "
> +                         "startTime INTEGER, "
> +                         "endTime INTEGER, "
> +                         "state INTEGER"
> +                         "entityID TEXT"
> +                         ")"));
> +                 NS_ENSURE_SUCCESS(rv, rv);
> 
> Use entityID TEXT DEFAULT NULL
> 

Oh, I did not know that this was possible. Since we were just copying everything from the temp table to the normal one I thought of that approach. I will incorporate this change right away.

Regards,
Brahmana.
(In reply to comment #35)
> as for strings i often see this:
> 
> nsCAutoString entityIDString;
> stmt->GetUTF8String(0, entityIDString);
> 

I tried this also. When i did this i got a lot of linker errors. It said some 590 things were unresolved. (Not sure of the number). But this did resolve the earlier compile error. 

Guidance please.

Regards,
Brahmana
This is the final part of the make output:

xul.dll : fatal error LNK1120: 591 unresolved externals
make[4]: *** [xul.dll] Error 96
make[4]: Leaving directory `/e/soc-ff/mozilla/obj-ff/toolkit/library'
make[3]: *** [libs_tier_toolkit] Error 2
make[3]: Leaving directory `/e/soc-ff/mozilla/obj-ff'
make[2]: *** [tier_toolkit] Error 2
make[2]: Leaving directory `/e/soc-ff/mozilla/obj-ff'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/e/soc-ff/mozilla/obj-ff'
make: *** [build] Error 2

Regards,
Brahmana
Attached patch patch v0.1 (obsolete) — — Splinter Review 
This incorporates the changes suggested.

entityID TEXT DEFAULT NULL for the DB and
nsCAutoString entityIDString;
rv = stmt->GetUTF8String(0,entityIDString);

This has no compile errors now and that linker error was also because of some other reason. I discarded my old build and did a complete new build and it was successful.

Now what I need to know is: What should happen at Pause?
I think canceling cannot work as the nsIDownload object will be lost. Suggestions please.

Regards,
Brahmana.
Attachment #276100 - Attachment is obsolete: true
(In reply to comment #40)
> Now what I need to know is: What should happen at Pause?

The current download manager already supports pause/resume and cancel/retry/remove:
Pause pauses the download, but keeps the partial file, so that the download can be resumed from that point if the user clicks Resume.
Cancel stops the download and deletes the partial file. The user can Retry it from the beginning if he wants or Remove the entry from the download manager altogether.

The new feature is cross-session resume, i.e. the ability to resume downloads even after a restart of Firefox, so the question is what should happen in the case of restarting Firefox. Downloads should automatically be resumed after a restart by clicking on the Restart button in the Add-ons manager or in the software update dialog, since Firefox restores everything else (tabs etc.) as well.

Another question is what should happen when the user closes Firefox today in the middle of a download and opens it tomorrow. Maybe resume the download in the background, but slowly to not interrupt browsing the web, and at full speed once the user opens the download manager; software update does that already.
Or just open the download manager and let the user decide whether he wants to resume the download.

By the way, there are a bunch of tabs in the patch which should be converted to spaces.
Comment on attachment 276125 [details] [diff] [review]
patch v0.1

>+		if (schemaVersion < 3) {
>[snip]
>+		} // End of schema #3 upgrade code
This is total overkill. See http://www.sqlite.org/lang_altertable.html

>   return NS_OK;
> }
> 
> nsresult
>@@ -334,16 +406,17 @@
>     "CREATE TABLE moz_downloads ("
>       "id INTEGER PRIMARY KEY, "
>       "name TEXT, "
>       "source TEXT, "
>       "target TEXT, "
>       "startTime INTEGER, "
>       "endTime INTEGER, "
>       "state INTEGER"
>+      "entityID TEXT"
Need a comma for the line before.

> NS_IMETHODIMP
> nsDownload::OnStateChange(nsIWebProgress* aWebProgress,
>                           nsIRequest* aRequest, PRUint32 aStateFlags,
>                           nsresult aStatus)
> {
>+
>+	if(aStateFlags & STATE_START){
>[snip]
>+	}
>+	
No. Saving the entityID should be done at the same time the download state is set to DOWNLOAD_DOWNLOADING, in nsDownload::OnProgressChange64.
(In reply to comment #42)
> (From update of attachment 276125 [details] [diff] [review])
> >+		if (schemaVersion < 3) {
> >[snip]
> >+		} // End of schema #3 upgrade code
> This is total overkill. See http://www.sqlite.org/lang_altertable.html

oh yes i had fogotten to tell this, copy and create new tables is only needed when removing columns, not when adding, there you use ALTER table, Michael is totally right :)


(In reply to comment #41)
> (In reply to comment #40)
> > Now what I need to know is: What should happen at Pause?
> 
> The current download manager already supports pause/resume and
> cancel/retry/remove:
> Pause pauses the download, but keeps the partial file, so that the download can
> be resumed from that point if the user clicks Resume.

This does not succeed all the time. What now happens at Pause is that we just stop reading from the socket where as the network communication continues to happen until the socket is alive, i.e until timeout occurs. Now if the user clicks resume before that timeout then fine the download resumes. If not then the remaining data in the socket is read and the download is completed. No attempt to make another connection is made.

 This is what I have changed now. When resume is clicked I create another channel, set it to resume at the point left earlier using the entityID and size of partial download and resume the download. Now for this to happen when pause is clicked the previous channel has to be closed and download should stop absolutely. So I needed some guidance as to how this can be done. 

And we have had discussions on what needs to be done when restoring FF. The comments above have the details.

> By the way, there are a bunch of tabs in the patch which should be converted to
> spaces.
> 
I will try changing these. If those are pointed out then it will be easier.


(In reply to comment #42)
> (From update of attachment 276125 [details] [diff] [review])
> >+		if (schemaVersion < 3) {
> >[snip]
> >+		} // End of schema #3 upgrade code
> This is total overkill. See http://www.sqlite.org/lang_altertable.html
> 
I changed this part.

> >   return NS_OK;
> > }
> > 
> > nsresult
> >@@ -334,16 +406,17 @@
> >     "CREATE TABLE moz_downloads ("
> >       "id INTEGER PRIMARY KEY, "
> >       "name TEXT, "
> >       "source TEXT, "
> >       "target TEXT, "
> >       "startTime INTEGER, "
> >       "endTime INTEGER, "
> >       "state INTEGER"
> >+      "entityID TEXT"
> Need a comma for the line before.
> 
> > NS_IMETHODIMP
> > nsDownload::OnStateChange(nsIWebProgress* aWebProgress,
> >                           nsIRequest* aRequest, PRUint32 aStateFlags,
> >                           nsresult aStatus)
> > {
> >+
> >+	if(aStateFlags & STATE_START){
> >[snip]
> >+	}
> >+	
> No. Saving the entityID should be done at the same time the download state is
> set to DOWNLOAD_DOWNLOADING, in nsDownload::OnProgressChange64.
> 
I did not understand why we cannot get the entityID at STATE_START. This was what had come out of a discussion at IRC with biesi and others. Can you please clear this up?


For the pause thing, I had a discussion with biesi and the outcome was that I will be using the mCancelable::Cance(), where mCancelable is of the type nsICancalable. This will close the socket but will not destroy the temporary file. So the channel will be gone and we can safely resume download with a new channel next time.

Now for this Cancel() i need to pass an error value (of type nsresult). Though no error has actually happened something like DOWNLOAD_PAUSED or something like that has to be passed. So I wanted to know what this should be.

I have incorporated the changes for the DB thing, by using ALTER TABLE now. If things are made clear about the point when the entityID should be fetched I will do that also. I will put up the patch when these things are clear. May be by tomorrow.

Regards,
Brahmana
(In reply to comment #46)
> Now for this Cancel() i need to pass an error value (of type nsresult). Though
> no error has actually happened something like DOWNLOAD_PAUSED or something like
> that has to be passed. So I wanted to know what this should be.
NS_BINDING_ABORT


(In reply to comment #45)
> I did not understand why we cannot get the entityID at STATE_START. This was
> what had come out of a discussion at IRC with biesi and others. Can you please
> clear this up?
The download may not actually be attached to the webbrowserpersists object at that time.  This is also why we do this check here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.100#1340
Note that this is probably where you want to do your work as well now.

(In reply to comment #47)
> (In reply to comment #46)
> > Now for this Cancel() i need to pass an error value (of type nsresult). Though
> > no error has actually happened something like DOWNLOAD_PAUSED or something like
> > that has to be passed. So I wanted to know what this should be.
> NS_BINDING_ABORT

Fine. 
> 
> 
> (In reply to comment #45)
> > I did not understand why we cannot get the entityID at STATE_START. This was
> > what had come out of a discussion at IRC with biesi and others. Can you please
> > clear this up?
> The download may not actually be attached to the webbrowserpersists object at
> that time.
Not every download happens through nsIWBP object. http://developer.mozilla.org/en/docs/Overview_of_how_downloads_work

We get the entityID after the channel is opened and I guess that happens when the request starts. So I thought STATE_START would be the right place. 

>  This is also why we do this check here:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.100#1340
> Note that this is probably where you want to do your work as well now.
> 
Even if I had to do it here rather than in OnStateChange, then I understand that right after you set the state to DOWNLOAD_DOWNLOADING I fetch the entityID and do the necessary stuff. i.e in the same "if" block pointed to by the link you provided. Validate this please.

Yes, in that if block.  See my latest patch in Bug 391870.
Attached patch patch v0.2 (obsolete) — — Splinter Review 
Addresses the comments. Done it in a hurry while leaving. Sorry if there are any trivial mistakes.

mwu please review the patch.

Reagards
Brahmana.
Attachment #276125 - Attachment is obsolete: true
Attachment #276647 - Flags: review?
Attachment #276647 - Flags: review? → review?(michael.wu)
The header inclusion nsNetErrors.h was not required. It will be removed.


Comment on attachment 276647 [details] [diff] [review]
patch v0.2

>@@ -228,17 +231,17 @@ nsDownloadManager::InitDB(PRBool *aDoImp
Note that this code (nsDownloadManager::InitDB) has been changed recently.. it shouldn't be too hard to update your schema version code to work with it.

You should also remember to update the schema version constant at the top of the file.

>@@ -333,17 +354,18 @@ nsDownloadManager::CreateTable()
>   return mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>     "CREATE TABLE moz_downloads ("
>       "id INTEGER PRIMARY KEY, "
>       "name TEXT, "
>       "source TEXT, "
>       "target TEXT, "
>       "startTime INTEGER, "
>       "endTime INTEGER, "
>-      "state INTEGER"
>+      "state INTEGER,"
Add a space after the comma.

>   if (mDownloadState == nsIDownloadManager::DOWNLOAD_NOTSTARTED) {
>     nsresult rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
>     NS_ENSURE_SUCCESS(rv, rv);
>+	
>+	//Fetch the entityID
>+	nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
>+	 
>+	if(NS_FAILED(rv))
>+	  return rv;            //Is returning proper here?.
>+
>+	nsCAutoString entityIDString;
>+	rv = dChannel->GetEntityID(entityIDString);
>+	NS_ENSURE_SUCCESS(rv, rv);
>+	 
>+	// Push the entityID to the DB
>+    nsCString stmt = NS_LITERAL_CSTRING("UDPATE moz_downloads SET entityId = ");
>+	stmt += entityIDString;
>+	stmt.AppendLiteral(" WHERE id =");
>+	stmt.AppendInt(mID);
>+	
>+    rv = mDownloadManager->mDBConn->ExecuteSimpleSQL(stmt);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+	
Note that SetState calls UpdateDB. I recommend storing entityID in the nsDownload object and have UpdateDB also update entityID information from nsDownload. Getting a download from the db should then set entityID in nsDownload so new new queries really need to be added.

>+  // Create a new nsIIOService using which a new channel can be created for this download.
>+  nsCOMPtr<nsIIOService> aIOS = do_CreateInstance("@mozilla.org/network/io-service;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsIChannel *aChannel;
>+  rv = aIOS->NewChannelFromURI(mSource, &aChannel);
I recommend you look at how nsWebBrowserPersist creates its channel and use that instead. It's simpler and more correct.
(In reply to comment #52)
> (From update of attachment 276647 [details] [diff] [review])
> >@@ -228,17 +231,17 @@ nsDownloadManager::InitDB(PRBool *aDoImp
> Note that this code (nsDownloadManager::InitDB) has been changed recently.. it
> shouldn't be too hard to update your schema version code to work with it.
> 
I did not get that exactly. Did you mean that the extra if() block is not necessary. Or was it just a statement stating that my current code works properly with the recent updates.

> You should also remember to update the schema version constant at the top of
> the file.
>
I will do that. It will be 3 right?
 
> >@@ -333,17 +354,18 @@ nsDownloadManager::CreateTable()
> >   return mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >     "CREATE TABLE moz_downloads ("
> >       "id INTEGER PRIMARY KEY, "
> >       "name TEXT, "
> >       "source TEXT, "
> >       "target TEXT, "
> >       "startTime INTEGER, "
> >       "endTime INTEGER, "
> >-      "state INTEGER"
> >+      "state INTEGER,"
> Add a space after the comma.
> 
Ok.

> >   if (mDownloadState == nsIDownloadManager::DOWNLOAD_NOTSTARTED) {
> >     nsresult rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
> >     NS_ENSURE_SUCCESS(rv, rv);
> >+	
> >+	//Fetch the entityID
> >+	nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
> >+	 
> >+	if(NS_FAILED(rv))
> >+	  return rv;            //Is returning proper here?.
> >+
> >+	nsCAutoString entityIDString;
> >+	rv = dChannel->GetEntityID(entityIDString);
> >+	NS_ENSURE_SUCCESS(rv, rv);
> >+	 
> >+	// Push the entityID to the DB
> >+    nsCString stmt = NS_LITERAL_CSTRING("UDPATE moz_downloads SET entityId = ");
> >+	stmt += entityIDString;
> >+	stmt.AppendLiteral(" WHERE id =");
> >+	stmt.AppendInt(mID);
> >+	
> >+    rv = mDownloadManager->mDBConn->ExecuteSimpleSQL(stmt);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+	
> Note that SetState calls UpdateDB. I recommend storing entityID in the
> nsDownload object and have UpdateDB also update entityID information from
> nsDownload. Getting a download from the db should then set entityID in
> nsDownload so new new queries really need to be added.

Then do I need to change the interface nsIDownlaod or is it sufficient to include the member string for the nsDownload class in nsDownloadManager.h?

If it is to be a member then it will be of type nsCAutoString right? If I had to add it to the interface, i need instructions on that.
 
> >+  // Create a new nsIIOService using which a new channel can be created for this download.
> >+  nsCOMPtr<nsIIOService> aIOS = do_CreateInstance("@mozilla.org/network/io-service;1", &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  nsIChannel *aChannel;
> >+  rv = aIOS->NewChannelFromURI(mSource, &aChannel);
> I recommend you look at how nsWebBrowserPersist creates its channel and use
> that instead. It's simpler and more correct.
> 

I guess you were referring to this: http://mxr.mozilla.org/seamonkey/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#1235

I will change the code to use NS_NewChannel. 
Will put in a patch tomorrow.

Reagrds,
Brahmana.


(In reply to comment #53)
> (In reply to comment #52)
> > (From update of attachment 276647 [details] [diff] [review] [details])
> > >@@ -228,17 +231,17 @@ nsDownloadManager::InitDB(PRBool *aDoImp
> > Note that this code (nsDownloadManager::InitDB) has been changed recently.. it
> > shouldn't be too hard to update your schema version code to work with it.
> > 
> I did not get that exactly. Did you mean that the extra if() block is not
> necessary. Or was it just a statement stating that my current code works
> properly with the recent updates.
> 
Recent updates change that area of code significantly, but it shouldn't be a problem to rework your patch to fit it in.

> > You should also remember to update the schema version constant at the top of
> > the file.
> >
> I will do that. It will be 3 right?
> 
Whatever it is, plus 1.

> Then do I need to change the interface nsIDownlaod or is it sufficient to
> include the member string for the nsDownload class in nsDownloadManager.h?
> 
Just add a member string.

> If it is to be a member then it will be of type nsCAutoString right? If I had
> to add it to the interface, i need instructions on that.
> 
nsCAutoString is definitely the wrong type. Use nsCString. See http://developer.mozilla.org/en/docs/XPCOM_string_guide for more details.
Attached patch patch v0.3 (obsolete) — — Splinter Review 
Address the last comment by mwu.

Reworked with recent changes. Added memeber string entityID. No compilation errors.

Regards,
Brahmana
Attachment #276647 - Attachment is obsolete: true
Attachment #276962 - Flags: review?(michael.wu)
Attachment #276647 - Flags: review?(michael.wu)
Comment on attachment 276962 [details] [diff] [review]
patch v0.3

>+    // Safely wrap this in a transaction so we don't hose the whole DB
>+    mozStorageTransaction safeTransaction(mDBConn, PR_TRUE);
I don't believe this needs to be put in a transaction. You're doing just a single query (ALTER TABLE) which itself should happen atomically. sdwilsh?

>@@ -335,17 +354,18 @@ nsDownloadManager::CreateTable()
>   return mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
[snip]
>+      "state INTEGER, "
>+    "entityID TEXT DEFAULT NULL"
nit. The last line is off by 2 spaces.

>@@ -656,17 +676,19 @@ nsDownloadManager::GetDownloadFromDB(PRU
>-
>+  
>+  stmt->GetUTF8String(6, dl->mEntityID);
>+  
nit. Don't add whitespace for empty lines.

>@@ -1348,17 +1371,28 @@ nsDownload::OnProgressChange64(nsIWebPro
>-    nsresult rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
>+    //Fetch the entityID
>+  nsresult rv;
>+  nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
>+  if(NS_FAILED(rv))
>+    return rv;            //Is returning proper here?.
Returning probably isn't the best thing to do here; we'll want to let the download continue like before resumable downloads. But if we don't store the entityID, then we can probably assume we shouldn't try (can't) resuming later.
>+
>+  nsCAutoString entityIDString;
>+  rv = dChannel->GetEntityID(entityIDString);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  mEntityID = entityIDString;
>+  
>+    rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
nit. Check spacing and blank lines.

>@@ -1715,23 +1749,60 @@ nsDownload::GetId(PRUint32 *aId)
>+  // Create a nsIWBP object which will be used to save the channel to the local file.
>+  nsCOMPtr<nsIWebBrowserPersist> wbp =
>+            do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv);
WBP isn't the only way to have downloads enter the download manager. Downloads can come from exthandler, so I'm not sure using WBP will result in the right action if the original download was in response to an "open file in external app". biesi?
I would be interested to know whats the difference between the resuming of downloads and automatic update. By updating to a newer version you are able to stop the download and resume it later - also after a restart of Firefox. Is there used another handling as we need here?
(In reply to comment #56)
> (From update of attachment 276962 [details] [diff] [review])
> >+    // Safely wrap this in a transaction so we don't hose the whole DB
> >+    mozStorageTransaction safeTransaction(mDBConn, PR_TRUE);
> I don't believe this needs to be put in a transaction. You're doing just a
> single query (ALTER TABLE) which itself should happen atomically. sdwilsh?
>
If that is so then I will just take that off.
----
> 
> >@@ -1348,17 +1371,28 @@ nsDownload::OnProgressChange64(nsIWebPro
> >-    nsresult rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
> >+    //Fetch the entityID
> >+  nsresult rv;
> >+  nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
> >+  if(NS_FAILED(rv))
> >+    return rv;            //Is returning proper here?.
> Returning probably isn't the best thing to do here; we'll want to let the
> download continue like before resumable downloads. But if we don't store the
> entityID, then we can probably assume we shouldn't try (can't) resuming later.

So what should be done here? Just set the entityID to NS_ERROR_NOT_RESUMABLE?
Because this is what is returned by GetEntityID when resume is not possible. 

> >+
> >+  nsCAutoString entityIDString;
> >+  rv = dChannel->GetEntityID(entityIDString);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  mEntityID = entityIDString;
> >+  
> >+    rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
> nit. Check spacing and blank lines.
> 
> >@@ -1715,23 +1749,60 @@ nsDownload::GetId(PRUint32 *aId)
> >+  // Create a nsIWBP object which will be used to save the channel to the local file.
> >+  nsCOMPtr<nsIWebBrowserPersist> wbp =
> >+            do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv);
> WBP isn't the only way to have downloads enter the download manager. Downloads
> can come from exthandler, so I'm not sure using WBP will result in the right
> action if the original download was in response to an "open file in external
> app". biesi?
> 

I had a discussion with biesi about this sometime ago. At that time we decided
that this resume would support only "Save to Disk" option. 

I have one doubt here. In case of ExternalHelperApp finally
ExecuteDesiredAction() is called which moves the file to the final destination.
What does the similar thing here when we are using WBP? The biesi's diagram
here: http://developer.mozilla.org/en/docs/Overview_of_how_downloads_work does
not show something like that for the second path. 

The spaces will be taken care of. Actually they are properly aligned when i see
it in my editor ( which is Notepad++ ). In the patch it appears that way. I do
not know why. I will make it consistent with the rest of the file.

Regards
Brahmana.
(In reply to comment #58)
> (In reply to comment #56)
> > I don't believe this needs to be put in a transaction. You're doing just a
> > single query (ALTER TABLE) which itself should happen atomically. sdwilsh?
> >
> If that is so then I will just take that off.

That's correct; transactions are only necessary when more than one SQL statement needs to be executed atomically; individual statements are atomic.
as for exthandler, I couldn't think of a good way to make that work, though perhaps it's enough to store whether the user selected the default application for that type and if not which application they selected, and run that when the dl is finished.
(In reply to comment #60)
> as for exthandler, I couldn't think of a good way to make that work, though
> perhaps it's enough to store whether the user selected the default application
> for that type and if not which application they selected, and run that when the
> dl is finished.
> 
I had a discussion with biesi and quite a few things came out. Here is the summary:

What biesi has said above would be done when we want to support resume for downloads of type "Open with [some App]". Until that is supported a work around would be to do the old way of resume (i.e Suspend()/Resume() ) for these type of  downloads and the actual Pause/Resume would be done to "Save to Disk" type of downloads. Downloads will be differentiated using the nsIMIMEInfo.preferredAction value. Every nsIDownload object has a mimeInfo member. 

dmose, comments on supporting resume for "Open with" kind of downloads?

-----------------
> > >@@ -1348,17 +1371,28 @@ nsDownload::OnProgressChange64(nsIWebPro
> > >-    nsresult rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
> > >+    //Fetch the entityID
> > >+  nsresult rv;
> > >+  nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
> > >+  if(NS_FAILED(rv))
> > >+    return rv;            //Is returning proper here?.
> > Returning probably isn't the best thing to do here; we'll want to let the
> > download continue like before resumable downloads. But if we don't store the
> > entityID, then we can probably assume we shouldn't try (can't) resuming later.
> 
> So what should be done here? Just set the entityID to NS_ERROR_NOT_RESUMABLE?
> Because this is what is returned by GetEntityID when resume is not possible. 
> 
This is still unanswered. I am going ahead and setting it to NS_ERROR_NOT_RESUMABLE.

> I have one doubt here. In case of ExternalHelperApp finally
> ExecuteDesiredAction() is called which moves the file to the final destination.
> What does the similar thing here when we are using WBP? The biesi's diagram
> here: http://developer.mozilla.org/en/docs/Overview_of_how_downloads_work does
> not show something like that for the second path. 
> 
nsIWBP does not have any such function to move the downloaded data from the temp file to the target file at the end. It directly saves the contents to the target file. So we will have a problem if a download was started via the exthandler, as it would have placed the data in the temp file and resume with nsIWBP would not move the data to target file.(The current patch for resume appends data to the temp file). The solution would be to have a flag(a member of nsDownload) telling us whether we need to move the data to a different file at the end. 

Regards
Brahmana.
(In reply to comment #61)
> > > >@@ -1348,17 +1371,28 @@ nsDownload::OnProgressChange64(nsIWebPro
> > > >-    nsresult rv = SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
> > > >+  nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
> > > >+  if(NS_FAILED(rv))
> > > Returning probably isn't the best thing to do here; we'll want to let the
> > > download continue like before resumable downloads.
> > So what should be done here? Just set the entityID to NS_ERROR_NOT_RESUMABLE?
> > Because this is what is returned by GetEntityID when resume is not possible. 
> This is still unanswered. I am going ahead and setting it to
> NS_ERROR_NOT_RESUMABLE.
First off you'll want to pass in |rv| to get it set when QIing. And mEntityID is a nsCString, so you can't quite set it to NS_ERROR_NOT_RESUMABLE. But if you were talking about returning that value on failure, that won't quite work either.

This piece of code is part of OnProgressChange which gets called quite often during a file transfer, so with the current patch, it'll continuously check if it's QUEUED and fail to get a ResumableChannel and return NOT_RESUMABLE and consequentially not SetState to DOWNLOADING. This means the UI won't get updated until the download changes "Stop" state change at which point the UI might still be wrong because we never updated the progress.
> First off you'll want to pass in |rv| to get it set when QIing. And mEntityID
> is a nsCString, so you can't quite set it to NS_ERROR_NOT_RESUMABLE. But if you
> were talking about returning that value on failure, that won't quite work
> either.
> 
> This piece of code is part of OnProgressChange which gets called quite often
> during a file transfer, so with the current patch, it'll continuously check if
> it's QUEUED and fail to get a ResumableChannel and return NOT_RESUMABLE and
> consequentially not SetState to DOWNLOADING. This means the UI won't get
> updated until the download changes "Stop" state change at which point the UI
> might still be wrong because we never updated the progress.
> 

Ok. I get it. So in case the entityID is not available, then it means we cannot support resume. So mEntityID will be a null string. If possible in such a case the Pause/Resume button should be disabled. Else I can just see if the mEntityID is null string and if not proceed with the resuming. If it is null then either we can shown an alert using nsIAlertService (the sliding notification -- though as discussed earlier Mac users without Growl might have a problem). 

Regards
Brahmana
(In reply to comment #61)
> What biesi has said above would be done when we want to support resume for
> downloads of type "Open with [some App]". Until that is supported a work around
> would be to do the old way of resume (i.e Suspend()/Resume() ) for these type
> of  downloads and the actual Pause/Resume would be done to "Save to Disk" type
> of downloads. Downloads will be differentiated using the
> nsIMIMEInfo.preferredAction value. Every nsIDownload object has a mimeInfo
> member. 
> 
> dmose, comments on supporting resume for "Open with" kind of downloads?

That sounds like a fine plan.  I'd say that the thing to do here is worry about the "Save As" case, and we can spin the "Open with" case off to a separate bug.  In general, we tend to see the most success by keeping individual patches as small as we reasonably can.

> So in case the entityID is not available, then it means we cannot
> support resume.

I'm pretty sure that that was what I did 5 years ago, but I can't recall for sure... However, the reverse isn't true - an entity id does not guarantee that the URL will be resumable.

Looking at the code:

 - if a file ends up not being resumable (eg because its changed on the server), what happens? Where are the various error responses (NS_ERROR_ENTITY_CHANGED, NS_ERROR_NOT_RESUMABLE) handled? Presumably the user needs to be prompted, or notified, or at least the request needs to be restarted - what will wbp do with a errored channel, anyway?
 - Ditto for anything but a 'standard' response. Those actually look more serious - they fall down into ProcessNormal in nsHttpChannel and the flags to the webbrowserpersist object means that the error html will just be appended to the existing file, I think?
 - Redirections are Interesting, too - the http code doesn't seem to take resuming into account, either by passing the various entity/time/conditional-get/etc headers on to the new request, or by failing out as non-resumable (the second is probably correct) Is the URL saved in the download manager is the final, post-redirection URL?

BTW, some other suggested things to test:

 - Resuming a download that requires authentication (if you have 5 files resuming, can you tell which dialog is for which?)
 - Resuming a download that fails due to a timeout on the server should not invalidate the partially downloaded file. Ditto for DNS errors, etc

I'm also wondering what happens cross-version with resumed downloads. The entity id isn't exactly version safe (its a bit of a hack, really), and I'm wondering if there are odd issues that could happen if the format is changed and you try to resume a download in firefox 4 that was started in firefox 3.
I will put a patch addressing quite a few comments by tomorrow morning (my time).
I will make it consistent with the recent changes which put the referrer into the DB.

Today when i checked the Pause/Resume worked fine once. If I tried to pause for a second time the DM showed the download as still downloading with several millions of GB/sec download speed. And after the Pause/Resume would not work.

Tomorrows patch would take care of the mimeinfo thing and possibly notification on a failure.

Regards
Brahmana


(In reply to comment #65)
>  - Ditto for anything but a 'standard' response. Those actually look more
> serious - they fall down into ProcessNormal in nsHttpChannel and the flags to
> the webbrowserpersist object means that the error html will just be appended to
> the existing file, I think?

What specific code are you concerned about here? If the server sends 200 (or 203) and we attempted a resume, the HTTP channel will abort the load.

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#777


>  - Redirections are Interesting, too - the http code doesn't seem to take
> resuming into account, either by passing the various
> entity/time/conditional-get/etc headers on to the new request, or by failing
> out as non-resumable (the second is probably correct) Is the URL saved in the
> download manager is the final, post-redirection URL?

Hm, good point... probably needs some testing.

> I'm also wondering what happens cross-version with resumed downloads. The
> entity id isn't exactly version safe (its a bit of a hack, really)

I think we should avoid changing the entity ID format in future versions.


(In reply to comment #67)
> (In reply to comment #65)
> >  - Ditto for anything but a 'standard' response. Those actually look more
> > serious - they fall down into ProcessNormal in nsHttpChannel and the flags to
> > the webbrowserpersist object means that the error html will just be appended to
> > the existing file, I think?
> 
> What specific code are you concerned about here? If the server sends 200 (or
> 203) and we attempted a resume, the HTTP channel will abort the load.

Yes, and what does wbp do in that case?

My issue in the bit you quoted was meant for non 2xx responses - eg 404/500/etc just goes through ProcessNormal, via the default bit in the case statement. Won't that just append the server error html to the partly downloaded file?

Its been a number of years since I touched mozilla code, so I'm also not sure where the html error pages for page timeouts/dns failures/etc are handled - what does the download manager/nswebbrowserpersist do?

> > I'm also wondering what happens cross-version with resumed downloads. The
> > entity id isn't exactly version safe (its a bit of a hack, really)
> 
> I think we should avoid changing the entity ID format in future versions.

Well, sure, until someone suggests that you use Content-MD5 as another item, or whatever. I came up with the entity thing, so I'm allowed to call it a hack ;) I don't think that its an issue that should block this, though; at the worst you could bump the sqllite version and migrate that way. It does deserve a comment in nsHttpChannel/nsFtpChannel, though
Attached patch patch v0.4 (obsolete) — — Splinter Review 
This takes care of "Open With" type of downloads in the old way of "Suspend()/Resume()".
Clears the entityID after the download ended (Successful or Failed).
Attachment #276962 - Attachment is obsolete: true
Attachment #276962 - Flags: review?(flamingice)
Attachment #277721 - Flags: review?(flamingice)
(In reply to comment #68)
> > What specific code are you concerned about here? If the server sends 200 (or
> > 203) and we attempted a resume, the HTTP channel will abort the load.
> 
> Yes, and what does wbp do in that case?

Call onStateChange iirc.

> My issue in the bit you quoted was meant for non 2xx responses - eg 404/500/etc
> just goes through ProcessNormal, via the default bit in the case statement.
> Won't that just append the server error html to the partly downloaded file?

Wouldn't the entityID not match, so that you get an error?

> Its been a number of years since I touched mozilla code, so I'm also not sure
> where the html error pages for page timeouts/dns failures/etc are handled -
> what does the download manager/nswebbrowserpersist do?

timeouts, dns failures etc do not generate a page as such so they are not an issue here. as for 404, 500 etc webbrowserpersist just saves those...

But you're right, HTTP should probably check for error codes and abort the load if (mResuming).


Comment on attachment 277721 [details] [diff] [review]
patch v0.4

>@@ -285,16 +287,31 @@ nsDownloadManager::InitDB(PRBool *aDoImp
>+	case 3:  // This version adds a column to the database (entityID)
>+    {
>+	  // ALTER the table to include the entityID column
>+	  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+	    " ALTER TABLE moz_downloads"
>+		" ADD COLUMN entityID TEXT DEFAULT NULL"));
I don't think a default needs to be set.

>@@ -736,16 +756,17 @@ nsDownloadManager::AddDownload(DownloadT
>   if (!dl)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   // give our new nsIDownload some info so it's ready to go off into the world
>   dl->mDownloadManager = this;
>   dl->mTarget = aTarget;
>   dl->mSource = aSource;
>   dl->mTempFile = aTempFile;
>+  dl->mEntityID = "";
No need - mEntityID should be initialized to a zero len string already.

>@@ -1389,16 +1410,28 @@ nsDownload::OnProgressChange64(nsIWebPro
>         if (chan) {
>           rv = chan->GetReferrer(getter_AddRefs(mReferrer));
>           if (NS_FAILED(rv))
>             mReferrer = nsnull;
>         }
>       }
>     }
> 
>+	//Fetch the entityID
>+	nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
>+	if (NS_FAILED(rv)) {
>+	  		// We should probably set a flag here which indicates that the download is not resumable.
An empty mEntityID should be usable for that purpose.

>+			// Or directly disable the Pause/Resume button. Otherwise we will have to notify when the Pause button is clicked.
>+	} else {
>+	  nsCAutoString entityIDString;
>+	  rv = dChannel->GetEntityID(entityIDString);
>+	  NS_ENSURE_SUCCESS(rv, rv);
>+	  mEntityID = entityIDString;
Should be no need to use a temporary buffer here.. just directly pass mEntityID to GetEntityID.
(In reply to comment #71)
> >@@ -285,16 +287,31 @@ nsDownloadManager::InitDB(PRBool *aDoImp
> >+	case 3:  // This version adds a column to the database (entityID)
> >+    {
> >+	  // ALTER the table to include the entityID column
> >+	  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+	    " ALTER TABLE moz_downloads"
> >+		" ADD COLUMN entityID TEXT DEFAULT NULL"));
> I don't think a default needs to be set.
That is correct.  It should only be |ADD COLUMN entityID TEXT|
Also, spaces should go at the end of the lines, not the beginning to match the rest of the file.

more in a bit...
Comment on attachment 277721 [details] [diff] [review]
patch v0.4

You still have tabs all over this file - they need to go to spaces before this is ready.

>       // Finally, update the schemaVersion variable and the database schema
>       schemaVersion = 3;
>       rv = mDBConn->SetSchemaVersion(schemaVersion);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>     // Fallthrough to the next upgrade
> 
>+	case 3:  // This version adds a column to the database (entityID)
You do of course mean 4, right?

nit: tab

>+    {
>+	  // ALTER the table to include the entityID column
This comment isn't necessary since SQL is pretty clear in what it is doing.

>       dl->mMaxBytes = LL_MAXUINT;
>     }
>   } else {
>     dl->mPercentComplete = 0;
>     dl->mMaxBytes = LL_MAXUINT;
>     dl->mCurrBytes = 0;
>   }
> 
>+  stmt->GetUTF8String(6, dl->mEntityID);
>+
Please check the return value

>+	//Fetch the entityID
>+	nsCOMPtr<nsIResumableChannel> dChannel(do_QueryInterface(aRequest));
Why dChannel?  the variable name doesn't make much sense to me

>+	if (NS_FAILED(rv)) {
>+	  		// We should probably set a flag here which indicates that the download is not resumable.
>+			// Or directly disable the Pause/Resume button. Otherwise we will have to notify when the Pause button is clicked.
>+	} else {
>+	  nsCAutoString entityIDString;
>+	  rv = dChannel->GetEntityID(entityIDString);
>+	  NS_ENSURE_SUCCESS(rv, rv);
>+	  mEntityID = entityIDString;
>+	}
>+
So, this logic seems completely wrong to me.  You aren't even getting an rv from the QueryInterface call.  You don't need to though, and can just check dChannel (it will be null of the QueryInterface call failed).  Like mwu said, you don't need to do anything in the case where it fails, but when it succeeds you will need to store the entityID.

>   if (aPause) {
>-    nsresult rv = mRequest->Suspend();
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsresult rv;
please don't declare rv until you need to use it.

>+	nsHandlerInfoAction action;
>+	mMIMEInfo->GetPreferredAction(&action);
check result

>-  nsresult rv = mRequest->Resume();
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  nsresult rv;
>+  nsHandlerInfoAction action;
>+  mMIMEInfo->GetPreferredAction(&action);
ditto

>+    wbp->SetProgressListener(this);
please cast return result to void.  You also should be setting the mCancelable on the nsDownload to the wbp like here (and include the comment!):
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.106#1048

>+    // Create a new channel for the source uri
>+    nsCOMPtr<nsIChannel> newChannel;
why don't we just call this "channel"

>+    nsCOMPtr<nsIInterfaceRequestor> newIIR(do_QueryInterface(wbp));
and "ir"

>+      if (NS_FAILED(rv) || newChannel == nsnull) {
>+        //EndDownload(NS_ERROR_FAILURE);
>+        return NS_ERROR_FAILURE;
>+      }
just do NS_ENSURE_SUCCESS(rv, rv)

>+    // Get the size of the temporary file to be used as offset.
>+    nsCOMPtr<nsILocalFile> tempFile = mTempFile;
>+    nsCOMPtr<nsIFile> newFile(do_QueryInterface(tempFile));
>+    PRInt64 fileSize;
>+    rv = newFile->GetFileSize(&fileSize);
mTempFile is an nsILocalFile, which dervives from an nsIFile, so you can call GetFileSize right off of the mTempFile

>+
>+    // Set the channel to resume at the required position along with the entityID
>+    nsCOMPtr<nsIResumableChannel> newResumableChannel(do_QueryInterface(newChannel));
"resumableChannel"
>+    NS_ENSURE_SUCCESS(rv, rv);
a) you aren't getting the rv from the QI call
b) what if it doesn't QI here?  do we *really* want to fail?  chances are we want to check if resumableChannel is null, and if it isn't, call ResumeAt

>+    newResumableChannel->ResumeAt(fileSize, mEntityID);
check result

>+  nsCString mEntityID;
Can we have this by the other string var?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.h&rev=1.35&root=/cvsroot#238

Also, with the schema update change, this needs unit tests like the ones here:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/downloads/test/schema_migration/
I can provide the test DB to use that (v3.sqlite) which I'll do shortly
Attached file test database — 
This is the v3.sqlite file that should be dropped into toolkit/components/downloads/test/schema_migration.

Basically, you'll copy test_migration_to_3.js to test_migration_to_4.js and change the values to the following:
id: 27
name: Firefox 2.0.0.6.dmg
source: http://ftp-mozilla.netscape.com/pub/mozilla.org/firefox/releases/2.0.0.6/mac/en-US/Firefox%202.0.0.6.dmg
target: file:///Users/sdwilsh/Desktop/Firefox%202.0.0.6.dmg
startTime: 1187390974170783
endTime: 1187391001257446
state: 1
referrer: http://www.mozilla.com/en-US/products/download.html?product=firefox-2.0.0.6&os=osx&lang=en-US

you'll need to add one additional check to make sure that entityID is null for this entry since it shouldn't have one.

To get this file in the diff, you'll have to use cvsdo to add it, then diff with cvs diff -u8pN to get the new file.
(In reply to comment #70)
> (In reply to comment #68)
> > > What specific code are you concerned about here? If the server sends 200 (or
> > > 203) and we attempted a resume, the HTTP channel will abort the load.
> > 
> > Yes, and what does wbp do in that case?
> 
> Call onStateChange iirc.

what happens to the partly downloaded file?

> > My issue in the bit you quoted was meant for non 2xx responses - eg 404/500/etc
> > just goes through ProcessNormal, via the default bit in the case statement.
> > Won't that just append the server error html to the partly downloaded file?
> 
> Wouldn't the entityID not match, so that you get an error?

Does nsHttpChannel check that? I don't see where it does. It probably should.
Attached patch patch v0.5 (obsolete) — — Splinter Review 
Addresses the comments. Will submit test case later in a separate file. No perl and hence cant use cvsdo. Will do it tomorrow.
Attachment #277721 - Attachment is obsolete: true
Attachment #277721 - Flags: review?(flamingice)
Attached file Test case (obsolete) — 
The test case for version 4 migration.
Attachment #277912 - Attachment mime type: application/x-javascript → text/plain
Comment on attachment 277912 [details]
Test case

Don't forget to add your name to the list of contributors (this is true for the other files as well).
(In reply to comment #75)
> > Wouldn't the entityID not match, so that you get an error?
> 
> Does nsHttpChannel check that? I don't see where it does. It probably should.

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#924


> No perl and hence cant use cvsdo. Will do it tomorrow.

if you can build mozilla you have perl (MozillaBuild contains it)
Attached patch patch v0.6 (obsolete) — — Splinter Review 
Patch got after doing a sync today. Builds fine.
Includes the test case for schema migration also.
Spaces and tabs taken care of.
Included name in the contributors list.

Regards
Brahmana
Attachment #277911 - Attachment is obsolete: true
Attachment #277912 - Attachment is obsolete: true
Attachment #278068 - Flags: review?(flamingice)
Comment on attachment 278068 [details] [diff] [review]
patch v0.6

>Index: src/nsDownloadManager.cpp
For the patch, you'll want to "cvs diff -u8p toolkit/components/downloads/src" from the mozilla directory (instead of the downloads directory).

>+  case 3:  // This version adds a column to the database (entityID)
(In reply to comment #73)
> You do of course mean 4, right?
You probably already noticed, but it's correct at 3. (We're checking if we need to update from 3 to 4.)

nit: you have 2 spaces before the comment instead of 1

>+      rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+        " ALTER TABLE moz_downloads"
>+        " ADD COLUMN entityID TEXT"));
No need for the leading space on each line, but make sure to put a trailing space for the first line.

>+    }  // End of schema #4 upgrade code
Don't need the comment here, the next line shows that we're at the beginning of the next upgrade.

>@@ -570,18 +588,18 @@ nsDownloadManager::Init()
>+    "SET startTime = ?1, endTime = ?2, state = ?3, referrer = ?4, entityID = ?5 "
>+    "WHERE id = ?6"), getter_AddRefs(mUpdateDownloadStatement));
Wrap at 80 columns, so just put the entityID = ?5 on a new line -- the style seems to have WHERE on its own line.

>+    if (resumableChannel)
>+      rv = resumableChannel->GetEntityID(mEntityID);
Did you want to check |rv|? If it failed, mEntityID will be null or the original value |""|?

>@@ -2033,24 +2059,77 @@ nsDownload::GetReferrer(nsIURI **referre
>   if (aPause) {
>-    nsresult rv = mRequest->Suspend();
>+    nsresult rv;
>+    nsHandlerInfoAction action;
>+    rv = mMIMEInfo->GetPreferredAction(&action);
>     NS_ENSURE_SUCCESS(rv, rv);
>+    if (action == nsIMIMEInfo::saveToDisk) {
>+      rv = mCancelable->Cancel(NS_BINDING_ABORTED);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } else {	// For "Open With" kind of downloads
You've got a tab before the comment.
>+      rv = mRequest->Suspend();
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>     mPaused = PR_TRUE;
>     return SetState(nsIDownloadManager::DOWNLOAD_PAUSED);
>   }
> 
>-  nsresult rv = mRequest->Resume();
>+  nsresult rv;
>+  nsHandlerInfoAction action;
>+  rv = mMIMEInfo->GetPreferredAction(&action);
>   NS_ENSURE_SUCCESS(rv, rv);
>+  if (action == nsIMIMEInfo::saveToDisk) {
We could refactor this to GetPreferredAction one time before checking aPause because it's needed down both paths of pause or resume. Question is if we want to split things by aPause then saveToDisk vs saveToDisk then aPause -- the latter would keep the logic for saveToDisk/openWith together. if (save) { if (pause) { } else { /* resume */ } } else { /* openWith */ ... }

>+    // Create a nsIWBP object which will be used to save the channel to the local file.
>+    nsCOMPtr<nsIWebBrowserPersist> wbp =
>+              do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv);
>+    rv = wbp->SetPersistFlags(nsIWebBrowserPersist::PERSIST_FLAGS_APPEND_TO_FILE |
>+                              nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
>+    // Set the channel to resume at the required position along with the entityID
nit: Check 80 width linewrap on those lines.

>+    nsCOMPtr<nsIResumableChannel> resumableChannel(do_QueryInterface(channel));
>+    if(resumableChannel) {
>+      rv = resumableChannel->ResumeAt(fileSize, mEntityID);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } else
>+      return NS_ERROR_UNEXPECTED;
Probably add {}s for the else even though it's a one line return -- the if block above it has brackets.

>+    // Save the channel using nsIWBP.
>+    rv = wbp->SaveChannel(channel, mTarget);
>+    NS_ENSURE_SUCCESS(rv, rv);
biesi: If we try to SaveChannel here with ResumeAt set, this will fail if it turns out it can't resume the file? (Or does that failure get sent down wbp later?) If it /is/ at this point, we could just restart the download instead of resuming it.
(In reply to comment #82)
> For the patch, you'll want to "cvs diff -u8p toolkit/components/downloads/src"
> from the mozilla directory (instead of the downloads directory).
No, you'll want to run "cvs diff -u8pN toolkit/components/downloads"

> >+    rv = wbp->SetPersistFlags(nsIWebBrowserPersist::PERSIST_FLAGS_APPEND_TO_FILE |
> >+                              nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
> >+    // Set the channel to resume at the required position along with the entityID
> nit: Check 80 width linewrap on those lines.
Normally, I'd agree, but for long flags like this, going over 80 chars is fine.

> >+    nsCOMPtr<nsIResumableChannel> resumableChannel(do_QueryInterface(channel));
> >+    if(resumableChannel) {
> >+      rv = resumableChannel->ResumeAt(fileSize, mEntityID);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+    } else
> >+      return NS_ERROR_UNEXPECTED;
> Probably add {}s for the else even though it's a one line return -- the if
> block above it has brackets.
Please do.
(In reply to comment #82)
> biesi: If we try to SaveChannel here with ResumeAt set, this will fail if it
> turns out it can't resume the file? (Or does that failure get sent down wbp
> later?) If it /is/ at this point, we could just restart the download instead of
> resuming it.

It will get sent down later, because we need to talk to the server to find out whether we can resume.


Comment on attachment 278068 [details] [diff] [review]
patch v0.6

>   if (aPause) {
>-    nsresult rv = mRequest->Suspend();
>+    nsresult rv;
>+    nsHandlerInfoAction action;
>+    rv = mMIMEInfo->GetPreferredAction(&action);
declare nsresult rv here, not at the top of the block.

>-  nsresult rv = mRequest->Resume();
>+  nsresult rv;
>+  nsHandlerInfoAction action;
>+  rv = mMIMEInfo->GetPreferredAction(&action);
ditto

>+    mCancelable = wbp;
>+    wbp->SetProgressListener(this);
please cast return result to void.
(void)wbp->S...

>+    // Set the APPEND flag so that the data will not be overwritten.
>+    rv = wbp->SetPersistFlags(nsIWebBrowserPersist::PERSIST_FLAGS_APPEND_TO_FILE |
>+                              nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
>+    NS_ENSURE_SUCCESS(rv, rv);
Actually, if anything fails after this point you need to remove the cycle:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.106#1054

Please make sure you address all of my review comments next time.  Some of these were repeats, and Edward pointed out one that I asked you to fix last time as well.  It's fine to post work-in-progress patches where not everything is addressed, but please clearly state when you're doing this.
Attachment #278068 - Flags: review?(flamingice)
(In reply to comment #68)
> My issue in the bit you quoted was meant for non 2xx responses - eg 404/500/etc
> just goes through ProcessNormal, via the default bit in the case statement.
> Won't that just append the server error html to the partly downloaded file?

You're right though, the code should check for that. Filed Bug 393624
(In reply to comment #65)
>  - Redirections are Interesting, too - the http code doesn't seem to take
> resuming into account, either by passing the various
> entity/time/conditional-get/etc headers on to the new request, or by failing
> out as non-resumable (the second is probably correct) Is the URL saved in the
> download manager is the final, post-redirection URL?

BTW, the code does take that into account. See nsHttpChannel::SetupReplacementChannel, which transfers the resuming information to the new channel.

(As for the WBP questions you asked and that I ignored, I don't know the answer to them.. needs testing)
why set mEntityID to the empty string when the download is complete? There doesn't seem to be a reason for that...
(In reply to comment #88)
> why set mEntityID to the empty string when the download is complete? There
> doesn't seem to be a reason for that...
It would be less to store in the DB
Attached patch patch v0.7 (obsolete) — — Splinter Review 
Addresses all the comments, except the following:
1) biesi's about making the mEntityID null after the download.
2) Edward's comment about "if (save) { if
(pause) { } else { /* resume */ } } else { /* openWith */ ... }"

As the function is PauseResume I thought of making Pause/Resume the first condition to be checked.

3) For the Line wrap comments, I went with sdwilsh.

*************  REQUEST ***************
I request people to apply this patch and test it. Because when I tried it, Minefield crashed for every download.. and even "Save Page As". Though my patch deals with Pause/Resume, Minefield was crashing for every "Save image as".

So please try it and tell me what happened.

Regards,
Brahmana.
Attachment #278068 - Attachment is obsolete: true
Attachment #278411 - Flags: review?(flamingice)
I tried to download Notepad2 from this page http://www.flos-freeware.ch/
Minefield crashed when I clicked on the download link there. I tried to debug it and set breakpoints at the nsDownloadManager::Init() and few other places where I have changed the code. But Minefiled just crashed and never reached those breakpoints. 

So I just wanted to know whether this patch is causing the crashes.

Regards,
Brahmana.
Comment on attachment 278411 [details] [diff] [review]
patch v0.7

>+    // Creates a cycle that will be broken when the download finishes
>+    mCancelable = wbp;
>+    (void)wbp->SetProgressListener(this);
>+
>+    // Set the APPEND flag so that the data will not be overwritten.
>+    rv = wbp->SetPersistFlags(nsIWebBrowserPersist::PERSIST_FLAGS_APPEND_TO_FILE |
>+                              nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
>+    if(NS_FAILED(rv)) {
>+      mCancelable = nsnull;
>+      (void)wbp->SetProgressListener(nsnull);
>+      return rv;
>+    }
...
>+    NS_ENSURE_SUCCESS(rv, rv);
..
>+    NS_ENSURE_SUCCESS(rv, rv);
..
>+    NS_ENSURE_SUCCESS(rv, rv);
We'll want to break the mCancelable cycle at each of those NS_ENSURE_SUCCESS. We can probably use |goto|s for a failure case as in..

if (FAILED) goto fail;
...
if (FAILED) goto fail;
...
// end of function
return;
fail: mCancelable = nsnull...; return;
}

Or if we don't want gotos here, we might be able to pull that code after setting the progress listener into a function that returns a nsresult, so if it returns failure, we know to break the mCancelable cycle.
(In reply to comment #93)
> Or if we don't want gotos here, we might be able to pull that code after
> setting the progress listener into a function that returns a nsresult, so if it
> returns failure, we know to break the mCancelable cycle.
That would be the better solution.  Something like:

nsresult
nsDownload::BreakCycle(nsresult rv)
{
  mCancelable = nsnull;
  (void)wbp->SetProgressListener(nsnull);
  return rv;
}

Then we can just do:
NS_ENSURE_SUCCESS(rv, BreakCycle(rv));
on all of those lines.
Comment on attachment 278411 [details] [diff] [review]
patch v0.7


>@@ -145,16 +147,17 @@ nsDownloadManager::CancelAllDownloads()
>   return rv;
> }
> 
> void
> nsDownloadManager::CompleteDownload(nsDownload *aDownload)
> {
>   // we've stopped, so break the cycle we created at download start
>   aDownload->mCancelable = nsnull;
>+  aDownload->mEntityID = "";
aDownload->mEntityID.Truncate(); is probably a bit better.

>@@ -2032,26 +2060,80 @@ nsDownload::GetReferrer(nsIURI **referre
>-  nsresult rv = mRequest->Resume();
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (action == nsIMIMEInfo::saveToDisk) {
>+    // Create a nsIWBP object which will be used to save the channel to the local file.
>+    nsCOMPtr<nsIWebBrowserPersist> wbp =
>+              do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Creates a cycle that will be broken when the download finishes
>+    mCancelable = wbp;
>+    (void)wbp->SetProgressListener(this);
>+
>+    // Set the APPEND flag so that the data will not be overwritten.
>+    rv = wbp->SetPersistFlags(nsIWebBrowserPersist::PERSIST_FLAGS_APPEND_TO_FILE |
>+                              nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
>+    if(NS_FAILED(rv)) {
There should be a space after an |if|.

>+      mCancelable = nsnull;
>+      (void)wbp->SetProgressListener(nsnull);
>+      return rv;
>+    }
>+
>+    // Create a new channel for the source uri
>+    nsCOMPtr<nsIChannel> channel;
>+    nsCOMPtr<nsIInterfaceRequestor> ir(do_QueryInterface(wbp));
>+    rv = NS_NewChannel(getter_AddRefs(channel), mSource, nsnull, nsnull, ir);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Get the size of the temporary file to be used as offset.
>+    PRInt64 fileSize;
>+    rv = mTempFile->GetFileSize(&fileSize);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Set the channel to resume at the required position along with the entityID
>+    nsCOMPtr<nsIResumableChannel> resumableChannel(do_QueryInterface(channel));
>+    if(resumableChannel) {
You should first check if mEntityID is empty or not with mEntityID.IsEmpty() and act accordingly.
When the app crashes while running under the debugger, you can tell (a certain amount about) what's happening by looking at the stack; no breakpoints necessary.  Does the stack reveal anything interesting?
(In reply to comment #94)
> (In reply to comment #93)
> > Or if we don't want gotos here, we might be able to pull that code after
> > setting the progress listener into a function that returns a nsresult, so if it
> > returns failure, we know to break the mCancelable cycle.
> That would be the better solution.  Something like:
> 
> nsresult
> nsDownload::BreakCycle(nsresult rv)
> {
>   mCancelable = nsnull;
>   (void)wbp->SetProgressListener(nsnull);
>   return rv;
> }
> 
> Then we can just do:
> NS_ENSURE_SUCCESS(rv, BreakCycle(rv));
> on all of those lines.
> 
Disregard all of that.  Juset create the cycle before you call ResumeAt, and handle failure of ResumeAt properly.

(In reply to comment #91)
> *************  REQUEST ***************
> I request people to apply this patch and test it. Because when I tried it,
> Minefield crashed for every download.. and even "Save Page As". Though my patch
> deals with Pause/Resume, Minefield was crashing for every "Save image as".
> 
I haven't tried the patch, but it looks like there's a possibility of mMIMEInfo being null, which you are using without any null checks. According to sdwilsh, you should be able to assume save to disk in those cases.

(In reply to comment #97)
> Disregard all of that.  Juset create the cycle before you call ResumeAt, and
> handle failure of ResumeAt properly.
> 

ResumeAt() never seems to fail. I mean it always returns NS_OK. Here are the implementations for HTTP and FTP channels:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4680
http://mxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFTPChannel.cpp#115

So what exactly do you mean by handling the failure of ResumeAt() ?

From what I could gather, a wrong entityID would result in a failure in the nsHttpChannel::ProcessNormal() where a check is performed between the entityID sent and the one fetched from the server at that time. Though it is checked for empty string in SetUpTransaction here : http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#562
no failure is expected even if the string is empty.

So based on a discussion with biesi I am checking for mEntityID.IsEmpty() and if it is so then the download will follow the old Suspend/Resume path.
Attached patch patch v0.8 (obsolete) — — Splinter Review 
Addresses mwu's comment: https://bugzilla.mozilla.org/show_bug.cgi?id=377243#c95 
and also sdwilsh's comment partly. I just moved the code which creates cycle to the place just before ResumeAt(), but as I said in my previous comment I need know what sort of checking do I need to do as ResumeAt() always returns NS_OK.

biesi: Do we get an error from SaveChannel() because of wrong entityID? I followed the code till nsHttpChannel::SetUpTransaction and found out that no error would occur. So if the error occurs at ProcessNormal() will it come here?

Should I check for the return of SaveChannel() to break the cycle between nsIWBP  and nsIDownload?

And in this patch, if the entityID is null then it follows the old Suspend/Resume path. This may be later changed to notify the user that Resume is not possible.

Regards,
Brahmana
Attachment #278411 - Attachment is obsolete: true
Attachment #278587 - Flags: review?(flamingice)
Attachment #278411 - Flags: review?(flamingice)
(In reply to comment #96)
> When the app crashes while running under the debugger, you can tell (a certain
> amount about) what's happening by looking at the stack; no breakpoints
> necessary.  Does the stack reveal anything interesting?
> 
Discussed it with sdwilsh. Seems like there is a problem with mTempFile or mMIMEInfo. Here is the stack trace:
*********************

xpcom_core.dll!nsQueryInterface::operator()(const nsID & aIID={...}, void * * answer=0x0012f62c)  Line 47 + 0x12 bytes	C++
docshell.dll!nsCOMPtr<nsILocalFile>::assign_from_qi(nsQueryInterface qi={...}, const nsID & aIID={...})  Line 1244 + 0x10 bytes	C++
docshell.dll!nsCOMPtr<nsILocalFile>::nsCOMPtr<nsILocalFile>(nsQueryInterface qi={...})  Line 646	C++
docshell.dll!nsMIMEInfoWin::LaunchDefaultWithFile(nsIFile * aFile=0x00000001)  Line 61	C++
docshell.dll!nsMIMEInfoBase::Release()  Line 49 + 0x92 bytes	C++
docshell.dll!nsMIMEInfoWin::Release()  Line 50 + 0xd bytes	C++
xpc3250.dll!XPCJSRuntime::GCCallback(JSContext * cx=0x01fc9af8, JSGCStatus status=JSGC_END)  Line 604 + 0x14 bytes	C++
gklayout.dll!DOMGCCallback(JSContext * cx=0x01fc9af8, JSGCStatus status=JSGC_END)  Line 3325 + 0x17 bytes	C++
xpc3250.dll!XPCCycleGCCallback(JSContext * cx=0x01fc9af8, JSGCStatus status=JSGC_END)  Line 523 + 0x17 bytes	C++
js3250.dll!js_GC(JSContext * cx=0x01fc9af8, JSGCInvocationKind gckind=GC_NORMAL)  Line 2552 + 0x11 bytes	C
js3250.dll!JS_GC(JSContext * cx=0x01fc9af8)  Line 2378 + 0xb bytes	C
xpc3250.dll!nsXPConnect::BeginCycleCollection()  Line 572 + 0xa bytes	C++
xpcom_core.dll!nsCycleCollector::Collect(unsigned int aTryCollections=1)  Line 2070	C++
xpcom_core.dll!nsCycleCollector_collect()  Line 2602	C++
gklayout.dll!nsJSContext::Notify(nsITimer * timer=0x05bbcf88)  Line 3241	C++
xpcom_core.dll!nsTimerImpl::Fire()  Line 388	C++
xpcom_core.dll!nsTimerEvent::Run()  Line 459	C++
xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fa00)  Line 491	C++
xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bcdf78, int mayWait=1)  Line 227 + 0x16 bytes	C++
gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
tkitcmps.dll!nsAppStartup::Run()  Line 170 + 0x1c bytes	C++
xul.dll!XRE_main(int argc=3, char * * argv=0x00bc8858, const nsXREAppData * aAppData=0x00bc8c20)  Line 3069 + 0x25 bytes	C++

*****************
This is the stack trace copied after a crash which occurred after the code to check mMIMEInfo for null (the current patch). But Minefield is crashing even when I just download and not do any Pause/Resume stuff.

Regards,
Brahmana.
Comment on attachment 278587 [details] [diff] [review]
patch v0.8

>-        "SELECT id, name, source, target, startTime, endTime, state, referrer "
>+        "SELECT id, name, source, target, startTime, endTime, state, referrer, entityID "
nit: 80 column

>@@ -1899,26 +1927,85 @@ nsDownload::GetReferrer(nsIURI **referre
> nsresult
> nsDownload::PauseResume(PRBool aPause)
> {
>   if (mPaused == aPause || !mRequest)
>     return NS_OK;
>+    
>+  nsHandlerInfoAction action;
>+  nsresult rv;
>+  if (!mMIMEInfo) {
>+    action = nsIMIMEInfo::saveToDisk;
>+  } else {
>+    rv = mMIMEInfo->GetPreferredAction(&action);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
> 
>   if (aPause) {
>+    if (action == nsIMIMEInfo::saveToDisk && !mEntityID.IsEmpty()) {
>+  if (action == nsIMIMEInfo::saveToDisk && !mEntityID.IsEmpty()) {
Might as well create a boolean that's shared between the two if checks here and later.

>+      rv = mCancelable->Cancel(NS_BINDING_ABORTED);
sdwilsh: Doesn't the cycle need to be broken here because we're not going to reuse the same wbp later? More interesting is that we should probably set something else here so that CancelDownload doesn't do the "oh we're paused, so let's resume to be able to cancel" (no need to cancel because we've already canceled it here.)

Perhaps just set mCancelable to nsnull and check if (mCancelable) then decide to resume (this only happens if we're stall-the-channel-pausing) and Cancel.

>+    // Creates a cycle that will be broken when the download finishes
>+    mCancelable = wbp;
>+    (void)wbp->SetProgressListener(this);
>+
>+    // Set the channel to resume at the required position along with the entityID
>+    nsCOMPtr<nsIResumableChannel> resumableChannel(do_QueryInterface(channel));
>+    if(resumableChannel) {
>+      rv = resumableChannel->ResumeAt(fileSize, mEntityID);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } else {
>+      return NS_ERROR_UNEXPECTED;
>+    }
>+
>+    // Save the channel using nsIWBP.
>+    rv = wbp->SaveChannel(channel, mTarget);
>+    NS_ENSURE_SUCCESS(rv, rv);
I would have also created the cycle just before SaveChannel instead of before ResumeAt, but sdwilsh and biesi say it should be before. Don't forget to break the cycle if SaveChannel fails.
(In reply to comment #100)
> biesi: Do we get an error from SaveChannel() because of wrong entityID? I
> followed the code till nsHttpChannel::SetUpTransaction and found out that no
> error would occur. So if the error occurs at ProcessNormal() will it come here?

No, you get the error asynchronously, since you have to connect to the server to find out whether it's wrong. You either get the error in onStateChange or onStatusChange, I forgot which.

> Should I check for the return of SaveChannel() to break the cycle between
> nsIWBP  and nsIDownload?

You do have to handle a failure return from SaveChannel.



(In reply to comment #102)
> I would have also created the cycle just before SaveChannel instead of before
> ResumeAt, but sdwilsh and biesi say it should be before. Don't forget to break
> the cycle if SaveChannel fails.

Er, no. I always said it should be done before SaveChannel.


As for mTempFile, that variable can be null. You need to handle that case.
Comment on attachment 278587 [details] [diff] [review]
patch v0.8

>@@ -1899,26 +1927,85 @@ nsDownload::GetReferrer(nsIURI **referre
>   return NS_OK;
> }
> 
> nsresult
> nsDownload::PauseResume(PRBool aPause)
> {
>   if (mPaused == aPause || !mRequest)
>     return NS_OK;
>+    
>+  nsHandlerInfoAction action;
>+  nsresult rv;
>+  if (!mMIMEInfo) {
>+    action = nsIMIMEInfo::saveToDisk;
>+  } else {
>+    rv = mMIMEInfo->GetPreferredAction(&action);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
This would be better if written like this:
nsHandlerInfoAction action = nsIMIMEInfo::saveToDisk;
nsresult rv;
if (mMIMEInfo) {
...


>+  if (action == nsIMIMEInfo::saveToDisk && !mEntityID.IsEmpty()) {
>+    // Create a nsIWBP object which will be used to save the channel to the local file.
>+    nsCOMPtr<nsIWebBrowserPersist> wbp =
>+              do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Set the APPEND flag so that the data will not be overwritten.
>+    rv = wbp->SetPersistFlags(nsIWebBrowserPersist::PERSIST_FLAGS_APPEND_TO_FILE |
>+                              nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
>+    if (NS_FAILED(rv)) {
>+      mCancelable = nsnull;
>+      (void)wbp->SetProgressListener(nsnull);
>+      return rv;
>+    }
Just NS_ENSURE_SUCCESS here now

>+    // Creates a cycle that will be broken when the download finishes
>+    mCancelable = wbp;
>+    (void)wbp->SetProgressListener(this);
I see that I did tell you to put this before ResumeAt, but as biesi said, it should be before SaveChannel.

>+
>+    // Set the channel to resume at the required position along with the entityID
>+    nsCOMPtr<nsIResumableChannel> resumableChannel(do_QueryInterface(channel));
>+    if(resumableChannel) {
>+      rv = resumableChannel->ResumeAt(fileSize, mEntityID);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } else {
>+      return NS_ERROR_UNEXPECTED;
We should probably set the download to a failed state at this point so the user can retry it.
Comment on attachment 278587 [details] [diff] [review]
patch v0.8

>+    // Get the size of the temporary file to be used as offset.
>+    PRInt64 fileSize;
>+    rv = mTempFile->GetFileSize(&fileSize);
...
>+    // Save the channel using nsIWBP.
>+    rv = wbp->SaveChannel(channel, mTarget);
biesi: Is the temp file only used by exthandler? I remember running into issues when trying to deleting files for Link Fingerprints. WBP is for "save links" and doesn't use mTempFile and only mTarget.

This has a couple issues: 1) if we do have a temp file, we should continue saving to mTempFile instead of mTarget but yet somehow (??) trigger exthandler's moving the temporary to the actual target and 2) if we don't have a temp file, we should be getting the size of mTarget and continue appending to mTarget.


brahmana: The reason why you're getting insanely huge download speeds is because OnProgressChange64 is getting an aCurTotalProgress based on the newly created channel -- it starts back at 0 when you resume. But the download already knows that it downloaded some amount, so when it does..

nsUint64 diffBytes = curTotalProgress - nsUint64(mCurrBytes);

it overflows to a really big unsigned 64.

biesi: Is the channel supposed to report total progress from where it resumed?


biesi: Additionally, some reason it's sometimes hitting an assert in nsHttpChannel::OnDataAvailable after resuming..

NS_ASSERTION(progress <= progressMax, "unexpected progress values");
http://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4475

which would indicate it's getting more bytes than it expected which is contrary to the issue above...
Attached patch patch v0.81 (obsolete) — — Splinter Review 
Address almost all the comments except for Edward's about CancelDownload() calling PauseResume(). I need more inputs on that. And what I do not understand is why do we have to first resume before Canceling if it is paused? Is it because the mCancelable is not available? or because some other member is not available?

About the crashes, it appears that it is not because of my code at all. I set a break point at the point where an instance of DM is created, i.e in GetSingleton(). The control did not reach there at all. When I tried to download Firefox (for ext handler type of download) and Notepad2 (for Save link as type, from: http://www.flos-freeware.ch/zip/notepad2.zip ). In both cases it crashed before it hit the breakpoint.

So I guess my code is safe. At least with the latest patch where I am checking for almost every member which I am using.

And about the ExecuteDesiredAction() functionality for nsIWBP based downloads, I guess that should come out as a different bug. (This is needed in 2 cases:
1) If we plan to support resume for "Open with" kind of downloads
2) For a download started by Ext Handler we need to move the data to the final mTarget file from the mTempFile.)

dmose, biesi, what do you think?

Regards,
Brahmana.
Attachment #278587 - Attachment is obsolete: true
Attachment #278787 - Flags: review?(flamingice)
Attachment #278587 - Flags: review?(flamingice)
(In reply to comment #106)
> (From update of attachment 278587 [details] [diff] [review])
> >+    // Get the size of the temporary file to be used as offset.
> >+    PRInt64 fileSize;
> >+    rv = mTempFile->GetFileSize(&fileSize);
> ...
> >+    // Save the channel using nsIWBP.
> >+    rv = wbp->SaveChannel(channel, mTarget);
> biesi: Is the temp file only used by exthandler? I remember running into issues
> when trying to deleting files for Link Fingerprints. WBP is for "save links"
> and doesn't use mTempFile and only mTarget.

Right, only exthandler uses mTempFile.

> This has a couple issues: 1) if we do have a temp file, we should continue
> saving to mTempFile instead of mTarget but yet somehow (??) trigger
> exthandler's moving the temporary to the actual target

Clearly we can't make exthandler do that renaming; dl manager could do it itself though.

> and 2) if we don't have
> a temp file, we should be getting the size of mTarget and continue appending to
> mTarget.

Indeed.

> brahmana: The reason why you're getting insanely huge download speeds is
> because OnProgressChange64 is getting an aCurTotalProgress based on the newly
> created channel -- it starts back at 0 when you resume. But the download
> already knows that it downloaded some amount, so when it does..
> 
> nsUint64 diffBytes = curTotalProgress - nsUint64(mCurrBytes);

These days you can/should use PRInt64/PRUint64 directly, instead of using the nsInt64 wrappers.

> biesi: Is the channel supposed to report total progress from where it resumed?

I think it makes more sense for the channel to only report progress for what it is actually downloading. That makes it consistent with the contentLength property, and it makes aCurProgress match the amount of the data passed to onDataAvailable.

> biesi: Additionally, some reason it's sometimes hitting an assert in
> nsHttpChannel::OnDataAvailable after resuming..
> 
> NS_ASSERTION(progress <= progressMax, "unexpected progress values");
> http://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4475
> 
> which would indicate it's getting more bytes than it expected which is contrary
> to the issue above...

Hmm, normally that's a server bug, but it could easily be a but in our code here. Can you file a new bug on that, ideally with a testcase?

(In reply to comment #107)
> Address almost all the comments except for Edward's about CancelDownload()
> calling PauseResume(). I need more inputs on that. And what I do not understand
> is why do we have to first resume before Canceling if it is paused? Is it
> because the mCancelable is not available? or because some other member is not
> available?

Cancelling a paused channel only takes effect once it is resumed. It's because channels are supposed not to send any notifications while paused.

> About the crashes, it appears that it is not because of my code at all. I set a
> break point at the point where an instance of DM is created, i.e in
> GetSingleton(). The control did not reach there at all. When I tried to
> download Firefox (for ext handler type of download) and Notepad2 (for Save link
> as type, from: http://www.flos-freeware.ch/zip/notepad2.zip ). In both cases it
> crashed before it hit the breakpoint.

What was the call stack in the debugger for the crash?

> And about the ExecuteDesiredAction() functionality for nsIWBP based downloads,
> I guess that should come out as a different bug. (This is needed in 2 cases:
> 1) If we plan to support resume for "Open with" kind of downloads
> 2) For a download started by Ext Handler we need to move the data to the final
> mTarget file from the mTempFile.)

You need to take care of 2) in this bug, it's important for correctly doing resuming...

(In reply to comment #108)

> Cancelling a paused channel only takes effect once it is resumed. It's because
> channels are supposed not to send any notifications while paused.
> 
Ok. So a channel being pause would mean a Suspend() being called on the nsIRequest object, right? (Which would again be the Suspend() of the nsIInputStreamPump).

> > About the crashes, it appears that it is not because of my code at all. I set a
> > break point at the point where an instance of DM is created, i.e in
> > GetSingleton(). The control did not reach there at all. When I tried to
> > download Firefox (for ext handler type of download) and Notepad2 (for Save link
> > as type, from: http://www.flos-freeware.ch/zip/notepad2.zip ). In both cases it
> > crashed before it hit the breakpoint.
> 
> What was the call stack in the debugger for the crash?

Here it is: http://pastebin.ca/675758 

> 
> > And about the ExecuteDesiredAction() functionality for nsIWBP based downloads,
> > I guess that should come out as a different bug. (This is needed in 2 cases:
> > 1) If we plan to support resume for "Open with" kind of downloads
> > 2) For a download started by Ext Handler we need to move the data to the final
> > mTarget file from the mTempFile.)
> 
> You need to take care of 2) in this bug, it's important for correctly doing
> resuming...
> 
So this should be part of CompleteDownload() or another function with similar name being called from CompleteDownload(). And probably we also need a flag to know when to call this. Because for a normal download via ext handler, this will be done separately. Inputs on this please.

Regards,
Brahmana
(In reply to comment #109)
> Ok. So a channel being pause would mean a Suspend() being called on the
> nsIRequest object, right? (Which would again be the Suspend() of the
> nsIInputStreamPump).

yes.

> > What was the call stack in the debugger for the crash?
> 
> Here it is: http://pastebin.ca/675758 

try make clean in uriloader/exthandler, then make in uriloader/exthandler and docshell/build.



Since we're getting very close to the deadline (the tree closes this coming Wednesday), I'd like to propose the following: brahmana, if you could get your patch as complete as you can by tomorrow (Saturday, September 1, Pacific time), that'd be great.  mwu/sdwilsh: if after that, one of you guys could review, make any necessary changes, and then submit for module owner review, address any comments, and land, that would be fantastic.  Since, as I understand it, the crash that's been discussed here still happens even after the patch has been backed out, we should be in good shape.

(In reply to comment #111)
> Since we're getting very close to the deadline (the tree closes this coming
> Wednesday), I'd like to propose the following: brahmana, if you could get your
> patch as complete as you can by tomorrow (Saturday, September 1, Pacific time),
> that'd be great.
Sorry for the delay in the reply(I do not have access to internet at home). From what I discussed with mwu, I need to handle the error messages from server which we get in OnStateChange().  Then I have to bring in the functionality of ExecuteDesiredAction(), when the download was started through Ext handler and paused and resumed(only for saveToDisk kind of downloads). For this we need 3 things:

1) know whether ext handler started it: mTempFile can tell this -- it is null if ext handler is not involved
2) It was a Save To Disk download: We are already doing it.. mMIMEInfo tells us this
3) whether it was paused and resumed: I did not get a way to find this out. Should we have a flag that tell us whether a download was paused and resumed so that appropriate action can be taken in CompleteDownload().

And about the crashes, biesi's solution worked. Now I am not getting any more crashes(with the resume code).

Regards
Brahmana


Does this mean you'll have a new patch today?
Attached patch patch v0.9 (obsolete) — — Splinter Review 
This has the code to move the data to target file.

Problem:

When I checked, all downloads we getting corrupt. From what I could figure out GetFileSize() was not returning the right value. This led to repeated part data and corrupt downloads.

Regards
Brahmana.
Attachment #278787 - Attachment is obsolete: true
Attachment #279742 - Flags: review?(comrade693+bmo)
Attachment #278787 - Flags: review?(flamingice)
Attached patch patch v0.91 (obsolete) — — Splinter Review 
Changed the variable SaveToDisk to saveToDisk.

Will put in code to clone file before getting size tomorrow by 10 am here(India).

Regards,
Brahmana
Attachment #279742 - Attachment is obsolete: true
Attachment #279745 - Flags: review?(comrade693+bmo)
Attachment #279742 - Flags: review?(comrade693+bmo)
Attached patch v1.0 (obsolete) — — Splinter Review 
Fixes review nits.  Also adds the referrer to the channel.

This still gets corrupted downloads when pausing - and it seems like it is restarting the download even though we are calling ResumeAt based on the number of bytes we get at the end.
Attachment #279745 - Attachment is obsolete: true
Attachment #279745 - Flags: review?(comrade693+bmo)
Blocks: 395134
Attached patch v1.1 (obsolete) — — Splinter Review 
and here we go.  Turns out the append flag is a bad thing to do.  This works flawlessly for me.  Normal downloads, multiple pauses, single pause.  Nothing is corrupted.

There is still an issue with the UI, but that's not so trivial to fix (Edward and I are still working out the best way to solve that).  Downloads still work, so this is OK to land.  I've filed Bug 395134 about the UI.
Attachment #279801 - Attachment is obsolete: true
Attachment #279858 - Flags: review?(mconnor)
Comment on attachment 279858 [details] [diff] [review]
v1.1

r=me based on the interdiff, with a nit over IRC to make the caching issue comment clearer
Attachment #279858 - Flags: review?(mconnor) → review+
Blocks: 395144
Attached patch v1.2 — — Splinter Review 
For checkin.
Attachment #279858 - Attachment is obsolete: true
Comment on attachment 279745 [details] [diff] [review]
patch v0.91

Just to be clear, this had r=me with nits fixed, which I did.  mconnor reviewed additional changes that were made.
Attachment #279745 - Flags: review+
Thanks!

Checking in toolkit/components/downloads/test/schema_migration/v3.sqlite;
initial revision: 1.1
Checking in toolkit/components/downloads/test/schema_migration/test_migration_to_4.js;
initial revision: 1.1
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.112; previous revision: 1.111
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.39; previous revision: 1.38
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Flags: in-litmus?
Resolution: --- → FIXED
Target Milestone: Firefox 3 → Firefox 3 M8
Version: unspecified → Trunk
Depends on: 395188
Blocks: 395207
Depends on: 395330
Depends on: 395362
Blocks: 230870
No longer depends on: 230870
Blocks: 395537
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091005 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Bug 395537 added test_resume.js that tests real download resuming.
Flags: in-testsuite? → in-testsuite+
https://litmus.mozilla.org/show_test.cgi?id=4992
https://litmus.mozilla.org/show_test.cgi?id=4577

I'll be writing more as time goes along, too.

in-litmus+
Flags: in-litmus? → in-litmus+
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Product: Firefox → Toolkit
So when will this in 2007 "verified fixed" bug finally land in Firefox? Now we soon enter 2012, I run Firefox 8.0, and this browser still can't resume downloads. It says "download finished" when it is not finished at all, and the result is a corrupted file which I cannot open. Guess I'm the only person downloading files with Firefox these days. Why is Mozilla forcing me to use an external download manager that can actually resume interrupted downloads? Maybe another shift is necessary. May I suggest the name "Winter of Code"?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: