223099 - Obsolete FTP channel code in nsProgressDialog.js

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[[e-mail]]

The linked code in nsProgressDialog.js used to enable the pause/resume button
for FTP downloads. However this facility was bypassed in revision 1.10 when
blake made the button permanently visible. Should this code just be completely
removed (for which I am willing to write a patch), or should some attempt be
made to detect a suspendable channel, and disable/hide the button as appropriate?

Comment 1

[Boris Zbarsky [:bzbarsky]]

All channels should be suspendable, no?  Any that are not, will break badly
anyway when we fix our pre-downloading problems...

Comment 2

[Darin Fisher]

i think most of the channels should be suspendable now.  there may be some
exceptions in mailnews.  i think biesi may still be working on adding Suspend/
Resume support to a few more (nsIconChannel for example).

Comment 3

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

yes, I am working on adding Suspend/Resome to the (win) nsIconChannel (bug
221593), and we have a bug about the imap channel (bug 217434). to my knowledge,
all other channels support suspend/resume; except the iconchannels for other
platforms (may be addressed by Bug 223128, Bug 223130), but I really doubt
people would download anything from a moz-icon url and want to pause that.


so... just always enabling that button seems perfectly fine to me.

Comment 4

[[e-mail]]

Attachment: Proposed patch

Note that while researching this patch I found a really ugly bug - the
startTime setter doesn't return the value that was set :-( Fortunately nobody
uses the return value, but still...

Comment 5

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

Comment on attachment 133848 [details] [diff] [review]
Proposed patch

-		     this.dialogElement("pauseResume").label =
this.getString("pause");

doesn't this require other changes, like removing a .properties entry or
something like that?

Comment 6

[[e-mail]]

No, you need those strings for the real pause/resume function.

Comment 7

[Darin Fisher]

Comment on attachment 133848 [details] [diff] [review]
Proposed patch

sr=darin

it'll be interesting to see how well our non-FTP suspend/resume holds up in
practice.

Comment 8

[[e-mail]]

darin, that's why I was moved to file this bug, it already works.

Comment 9

[[e-mail]]

Fix checked in.

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

I believe this bug caused bug 224449