394546 - Paused retried downloads keep updating status

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

When pausing a download that I first canceled then retried, it'll display the "Paused -- X of X MB" momentarily and then continue updating with the usual progress status "X of X MB" several times.

On my slower iBook, it can update 4-5 times meaning 2-3 seconds. On my windows box, it's less likely to switch away from the "Paused --" status, but if I hit pause at the right time, it'll update the status one time.

Closing the download manager and reopening it and resume/pause doesn't make the problem go away.

(At first I was thinking it might be related somehow to RetryDownload -> GetDownloadFromDB -> new nsDownload().. with the UI still mysteriously getting updates from a previous nsDownload.. ??)

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Seems like strange stuff happens if we try to reuse the same item..

Comment 2

[Ed Lee :Mardak]

Attachment: v1.1

spam! nit: { on newline... :(

Comment 3

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

http://litmus.mozilla.org/show_test.cgi?id=4630

in-litmus+ (I hope I captured the major issue.)

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 279552 [details] [diff] [review]
v1.1

So, I guess I don't fully understand why you remove the download before adding it.  Care to explain (like what strange stuff)?

Comment 5

[Ed Lee :Mardak]

Strange stuff == this bug ;) I'm not sure exactly what's the path that gets taken that eventually leads to these display issues, but removing the download richlist item and making a new one when restarting gives us a clean slate instead of needying to tidy up the was-completed download to be an active download again.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 279552 [details] [diff] [review]
v1.1

>+function removeFromView(aDownload)
nit: this function should now go under the "Utility functions" section at the end of the file

>+function retryDownload(aDownload)
>+{
>+  removeFromView(aDownload);
>+  gDownloadManager.retryDownload(aDownload.getAttribute("dlid"));
> }
> 
> function showDownload(aDownload)
>@@ -570,11 +581,6 @@ function showDownloadInfo(aDownload)
> 
>   var button = document.getAnonymousElementByAttribute(aDownload, "anonid", "info");
>   gDownloadInfoPopup.openPopup(button, "after_end", 0, 0, false, false);
>-}
>-
>-function retryDownload(aDownload)
>-{
>-  gDownloadManager.retryDownload(aDownload.getAttribute("dlid"));
> }
Any reason why you moved this?

Comment 7

[Ed Lee :Mardak]

(In reply to comment #6)
> (From update of attachment 279552 [details] [diff] [review])
> >+function removeFromView(aDownload)
> nit: this function should now go under the "Utility functions" section at the
> end of the file
Btw.. there's a utility functions section at the top as well.... top or bottom? Or merge to bottom...

> >-function retryDownload(aDownload)
> >-{
> >-  gDownloadManager.retryDownload(aDownload.getAttribute("dlid"));
> > }
> Any reason why you moved this?
It didn't seem to belong down there. ;) All the other *Download ui actions seemed to be around there.

cancelDownload
pauseDownload
** resumeDownload
showDownload
...
openReferrer
showDownloadInfo
** retryDownload
onUpdateProgress

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Btw.. there's a utility functions section at the top as well.... top or bottom?
> Or merge to bottom...
put in the bottom...
we'll ignore the top ones for now.

> It didn't seem to belong down there. ;) All the other *Download ui actions
> seemed to be around there.
> 
> cancelDownload
> pauseDownload
> ** resumeDownload
> showDownload
> ...
> openReferrer
> showDownloadInfo
> ** retryDownload
> onUpdateProgress
fine by me then

Comment 9

[Ed Lee :Mardak]

Attachment: v1.2

Moved the utility function to where it belongs.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 280440 [details] [diff] [review]
v1.2

r=sdwilsh

Comment 11

[Ed Lee :Mardak]

Attachment: patch for checkin

Fix a couple lines of bitrot, but mercurial pushed it on fine with its default settings.

$ hg qpush
applying remove.onretry.patch
patching file toolkit/mozapps/downloads/content/downloads.js
Hunk #3 succeeded at 664 with fuzz 2 (offset -222 lines).
Now at: remove.onretry.patch

Comment 12

[Reed Loden [:reed]]

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.89; previous revision: 1.88
done

Comment 13

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

I ran http://litmus.mozilla.org/show_test.cgi?id=4630 with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007092705 Minefield/3.0a9pre and it's all good.

Verified FIXED