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

RESOLVED FIXED in mozilla1.9beta3

Status

()

P2
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla1.9beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

11 years ago
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.
Flags: blocking-firefox3?
(Assignee)

Comment 1

11 years ago
Created attachment 294923 [details] [diff] [review]
v1.0 back-end

Back-end first...
Attachment #294923 - Flags: review?(edilee)
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review 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?
(Assignee)

Comment 3

11 years ago
Created attachment 295004 [details] [diff] [review]
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...
Attachment #294923 - Attachment is obsolete: true
Attachment #295004 - Flags: review?(edilee)
Attachment #294923 - Flags: review?(edilee)
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.
I'm for the disabled button with tooltip as well.  There's discussion of a similar issue going on in bug 406203.
(Assignee)

Updated

11 years ago
Blocks: 401172
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?
Attachment #295004 - Flags: review?(edilee) → review+
(Assignee)

Comment 7

11 years ago
Created attachment 295645 [details] [diff] [review]
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.
Attachment #295004 - Attachment is obsolete: true
Attachment #295645 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Depends on: 392293
Whiteboard: [has patch][needs review Mardak] → [has back-end patch][has reivew for back-end][needs approval back-end][needs front-end patch]
(Assignee)

Comment 8

11 years ago
Created attachment 295690 [details] [diff] [review]
v1.0 front-end

Turns out, this is pretty simple :)
Attachment #295690 - Flags: review?(edilee)

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+

Updated

11 years ago
Attachment #295645 - Flags: approval1.9? → approval1.9+
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.
(Assignee)

Comment 10

11 years ago
Ug - yes, we need to be able to switch between the two.
Whiteboard: [has back-end patch][has reivew for back-end][needs approval back-end][needs front-end patch] → [needs new patch]
(Assignee)

Comment 11

11 years ago
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.
Whiteboard: [needs new patch] → [has patch][needs review Mardak]
(Assignee)

Updated

11 years ago
Priority: -- → P2
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.
(Assignee)

Comment 13

11 years ago
(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 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.
Attachment #295690 - Flags: review?(edilee) → review-
(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 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)
Attachment #295690 - Flags: review- → review+
(Assignee)

Comment 17

11 years ago
Alright - I'll update the patch and get a test for this later today so we can land her.
Whiteboard: [has patch][needs review Mardak] → [has patch][has Mardak][needs test]
(Assignee)

Comment 18

11 years ago
Having a difficult time with the test - getting to the anonymous content of the downloads is *hard* :(
(Assignee)

Comment 19

11 years ago
The test has demonstrated that my patch doesn't actually work.
Whiteboard: [has patch][has Mardak][needs test] → [needs new patch]
(Assignee)

Comment 20

11 years ago
Updating - the tests have flagged something that is seriously wrong with this patch.  I haven't tracked it down yet...
(Assignee)

Comment 21

11 years ago
Created attachment 297824 [details] [diff] [review]
v2.0

ug - this contains everything.  Stupid mistakes can make for a painful debugging experience :/
Attachment #295645 - Attachment is obsolete: true
Attachment #295690 - Attachment is obsolete: true
Attachment #297824 - Flags: review?(edilee)
(Assignee)

Updated

11 years ago
Attachment #297824 - Attachment is patch: true
Attachment #297824 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

11 years ago
Whiteboard: [needs new patch] → [has patch][needs review Mardak]
(Assignee)

Comment 22

11 years ago
Created attachment 297826 [details] [diff] [review]
v2.1

now with more l10n strings
Attachment #297824 - Attachment is obsolete: true
Attachment #297826 - Flags: review?(edilee)
Attachment #297824 - Flags: review?(edilee)
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 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
Attachment #297826 - Flags: review?(edilee) → review+
(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.
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.
(Assignee)

Comment 27

11 years ago
Created attachment 298401 [details] [diff] [review]
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.
Attachment #297826 - Attachment is obsolete: true
(Assignee)

Comment 28

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][needs review Mardak]
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.