410289 - Do not allow the pausing of downloads that cannot actually be resumed

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Shawn Wilsher :sdwilsh]

Right now, we still support the pausing of downloads that can't be resumed.  We pause them by suspending the channel in the hopes that things will magically work when we try to resume the channel.  This has the potential to cause some odd behavior, and breaks user's expectations when their download ends up being corrupted because this isn't reliable (and hard to detect errors on).

Shaver suggested (bug 406203 comment 10) that we drop this "fake pausing".  The more I think about it, the more I agree with it.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 back-end

Back-end first...

Comment 2

[Ed Lee :Mardak]

Comment on attachment 294923 [details] [diff] [review]
v1.0 back-end

> nsresult
> nsDownload::Pause()
> {
>+  if (!IsResumable())
>+    return NS_ERROR_UNEXPECTED;

> nsresult
> nsDownload::Resume()
> {
>-  nsresult rv = NS_ERROR_FAILURE;
>-  if (IsResumable())
>-    rv = RealResume();
Should we check resumable like for pause?

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.1 back-end

(In reply to comment #2)
> Should we check resumable like for pause?
No, but we should check that it's paused, which I missed...

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

So what would the UE be like, here? Just remove the button, or show a disabled button? I'd support disabled button with a tooltip that states "This download can't be paused" or something similar. Might want to blamecast to the server, might not care.

Comment 5

[Madhava Enros [:madhava]]

I'm for the disabled button with tooltip as well.  There's discussion of a similar issue going on in bug 406203.

Comment 6

[Ed Lee :Mardak]

Comment on attachment 295004 [details] [diff] [review]
v1.1 back-end

>+    /**
>+     * Indicates if the download can be resumed after being paused or not.  This
>+     * is only the case if the download is over HTTP/1.1 and if the server
>+     * supports it.
>+     */
>+    readonly attribute boolean resumable;
FTP downloads can be resumed as well. Also, this is only if we /think/ the download can be resumed.. The server can very well be lying to us.
> nsresult
> nsDownload::Resume()
> {
>-  nsresult rv = NS_ERROR_FAILURE;
>-  if (IsResumable())
>-    rv = RealResume();
>+  if (!IsPaused())
>+    return NS_ERROR_UNEXPECTED;
If we do get a resume when it's not resumable, it would be unexpected as well, no?

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.2 back-end

Addresses review comments.  I'm seeking approval for this now, but it *will not* land until the UI is done, but that needs another bug to land first.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.0 front-end

Turns out, this is pretty simple :)

Comment 9

[Ed Lee :Mardak]

Comment on attachment 295690 [details] [diff] [review]
v1.0 front-end

>+    if ("cmd_pause" == cmd && !enabled) {
>+      // We need to add the tooltip indicating that the download cannot be
>+      // paused now.
>+      button.setAttribute("tooltiptext", gStr.cannotPause);
Do we need a way to remove the tooltip if it 'becomes' resumable? We don't get an entity id until after transfer starts, so it might be possible that the pause button is initially disabled.

Comment 10

[Shawn Wilsher :sdwilsh]

Ug - yes, we need to be able to switch between the two.

Comment 11

[Shawn Wilsher :sdwilsh]

Actually, we don't go into DOWNLOAD_DOWNLOADING until the transfer starts.  That is the only state that we actually have a pause button so we do not have to worry about this.

Comment 12

[Ed Lee :Mardak]

Queued downloads can show up in the UI and the corresponding nsIDownload could have a resumable attribute that reports false because the entity id isn't set.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Queued downloads can show up in the UI and the corresponding nsIDownload could
> have a resumable attribute that reports false because the entity id isn't set.
Yes, but queued downloads cannot be paused.  We cannot do anything about being wrong early on about the resumable property, but we can update it once we go.  Perhaps add a comment that the resumable property isn't set until the download enters DOWNLOAD_DOWNLOADING?

Comment 14

[Ed Lee :Mardak]

Comment on attachment 295690 [details] [diff] [review]
v1.0 front-end

>       case "cmd_pause":
>-        return dl.inProgress && !dl.paused;
>+        var download = gDownloadManager.getDownload(dl.getAttribute("dlid"));
>+        return dl.inProgress && !dl.paused && download.resumable;
"let" in switch should work again now that bug 411279 is fixed. But just to be safe, perhaps we should just go with...
case "..":
let (download = gDow...) {
  return download.inPr..;
}

> function updateButtons(aItem)
..
>+    if ("cmd_pause" == cmd && !enabled) {
>+      // We need to add the tooltip indicating that the download cannot be
>+      // paused now.
>+      button.setAttribute("tooltiptext", gStr.cannotPause);
I would feel better if we had some way to remove the tooltip as well. (Potentially something can lose its resumability if we do some probing to see if the server actually supports resuming.)
b.setA("tooltip", enabled ? "" : cannotPause);
or removeAttr if enabled.

Comment 15

[Ed Lee :Mardak]

(In reply to comment #14)
> Potentially something can lose its resumability if we do some probing to see
> if the server actually supports resuming.
Just to clarify, this could happen if we switch to DOWNLOADING and get an entityid and at some later point we open a new connection to the server trying out the entityid. If the server doesn't actually support resume, we would remove the entityid and the download would be considered not resumable.

Comment 16

[Ed Lee :Mardak]

Comment on attachment 295690 [details] [diff] [review]
v1.0 front-end

Nevermind about the needing to revert adding the tooltip. That would be if we went from not resumable to resumable which wouldn't be able to happen. Even if we did implement checking of 'server actually supports resume', it would go from resumable to not.

nit: var -> let or let block (pretty sure let inside switch/case will stay working for js1.7 and maybe for js2 if brendan has his way)

Comment 17

[Shawn Wilsher :sdwilsh]

Alright - I'll update the patch and get a test for this later today so we can land her.

Comment 18

[Shawn Wilsher :sdwilsh]

Having a difficult time with the test - getting to the anonymous content of the downloads is *hard* :(

Comment 19

[Shawn Wilsher :sdwilsh]

The test has demonstrated that my patch doesn't actually work.

Comment 20

[Shawn Wilsher :sdwilsh]

Updating - the tests have flagged something that is seriously wrong with this patch.  I haven't tracked it down yet...

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

ug - this contains everything.  Stupid mistakes can make for a painful debugging experience :/

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

now with more l10n strings

Comment 23

[Madhava Enros [:madhava]]

Instead of "The server does not support the pausing of downloads" (for cannotPause), how about: "This download cannot be paused"

I realize that it's less information (doesn't convey that it's the server's fault, with the additional implication that other downloads from the server will have the same problem), but the question we're really trying to answer here is "What is _that_ button?" This simpler message tells people that the button is about pausing, but won't work in this case.  A message about things not being supported by servers, while true, feels like it's getting more complicated than it has to be.

Comment 24

[Ed Lee :Mardak]

Comment on attachment 297826 [details] [diff] [review]
v2.1

madhava: see comment past the middle about disabled pause button

>+cannotPause=The server does not support the pausing of downloads
Switch this to madhava'a suggestion.

>       <property name="buttons">
>         <getter>
>         <![CDATA[
>+          var startEl = document.getAnonymousNodes(this);
>+          if (!startEl.length)
>+            startEl = [this];
>+
>           const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>-          return this.getElementsByTagNameNS(XULNS, "button");
>+          return startEl[0].getElementsByTagNameNS(XULNS, "button");
Some comments here would be nice why "this" doesn't always work.

>+    let download = null; // used for getting an nsIDownload object
..
>       case "cmd_pause":
>-        return dl.inProgress && !dl.paused;
>+        download = gDownloadManager.getDownload(dl.getAttribute("dlid"));
>+        return dl.inProgress && !dl.paused && download.resumable;
>       case "cmd_pauseResume":
>-        return dl.inProgress || dl.paused;
>+        download = gDownloadManager.getDownload(dl.getAttribute("dlid"));
>+        return (dl.inProgress || dl.paused) && download.resumable;
>       case "cmd_resume":
>-        return dl.paused;
>+        download = gDownloadManager.getDownload(dl.getAttribute("dlid"));
>+        return dl.paused && download.resumable;
If you want, I suppose there's a mini optimization of not doing gDownloadManager.getDownload unless we /really/ have to go through xpconnect. E.g...
return dl.inProgress && !dl.paused && gDownloadManager.getDownload(dl.getAttribute("dlid")).resumable;
(And we wouldn't have to worry about the "let" issue either! :p)

> function updateButtons(aItem)
> {
>   let buttons = aItem.buttons;
> 
>   for (let i = 0; i < buttons.length; ++i) {
>     let cmd = buttons[i].getAttribute("cmd");
>     let enabled = gDownloadViewController.isCommandEnabled(cmd, aItem);
>     buttons[i].disabled = !enabled;
We'll probably need a followup bug to have a disabled pause button... because the current one could look like it's always disabled.. it's all gray. And the user doesn't know until s/he keeps clicking and nothing happens or waits for the tooltip.

madhava: It sounded like there won't be new buttons for the download manager? And we're sure we want to show the pause button when it can't be paused?
sdwilsh: I suppose the easy fix would just be .paused[disabled] { display: none; }
>+
>+    if ("cmd_pause" == cmd && !enabled) {
>+      // We need to add the tooltip indicating that the download cannot be
>+      // paused now.
>+      //buttons[i].setAttribute("tooltiptext", gStr.cannotPause);
This needs to be uncommented
>+      try {
>+        // XXX button is undefined.  previously, so was gStr.cannotPause
>+        button.setAttribute("tooltiptext", gStr.cannotPause);
>+      } catch (e) { debugger; throw e; }
And that deleted

Comment 25

[Madhava Enros [:madhava]]

(In reply to comment #24)

> madhava: It sounded like there won't be new buttons for the download manager?
> And we're sure we want to show the pause button when it can't be paused?
> sdwilsh: I suppose the easy fix would just be .paused[disabled] { display:
> none; }

We're going to have a disabled state for the pause button -- it's in the list of icons to create for all the platforms.  The current mac on is grey, it's true, but I'm pretty sure that they can make a disabled one that will look comparatively disabled.

Comment 26

[Ed Lee :Mardak]

Alternatively, we can show an icon that indicates it can't be paused.. like a pause button with a red circle/slash like "no smoking" signs.

Comment 27

[Shawn Wilsher :sdwilsh]

Attachment: v2.2

For checkin

(In reply to comment #24)
> Some comments here would be nice why "this" doesn't always work.
It's some weird XBL stuff - basically I was told to copy what richlistitem does for it to work properly :/

> If you want, I suppose there's a mini optimization of not doing
> gDownloadManager.getDownload unless we /really/ have to go through xpconnect.
> E.g...
> return dl.inProgress && !dl.paused &&
> gDownloadManager.getDownload(dl.getAttribute("dlid")).resumable;
> (And we wouldn't have to worry about the "let" issue either! :p)
I opted not to since it makes the formatting of the code yucky at best.

> We'll probably need a followup bug to have a disabled pause button... because
> the current one could look like it's always disabled.. it's all gray. And the
> user doesn't know until s/he keeps clicking and nothing happens or waits for
> the tooltip.
It's supposed to be in the new theme like this.

Comment 28

[Shawn Wilsher :sdwilsh]

yay automated tests :)

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
new revision: 1.20; previous revision: 1.19
Checking in toolkit/components/downloads/public/nsIDownload.idl;
new revision: 1.22; previous revision: 1.21
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.155; previous revision: 1.154
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.60; previous revision: 1.59
Checking in toolkit/mozapps/downloads/content/download.xml;
new revision: 1.52; previous revision: 1.51
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.131; previous revision: 1.130
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
new revision: 1.8; previous revision: 1.7
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_410289.js;
initial revision: 1.1

Comment 29

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

I spun off bug 413610.