Closed Bug 830205 Opened 7 years ago Closed 7 years ago

Download button shows progress remaining after download finishes

Categories

(Firefox :: Downloads Panel, defect)

20 Branch
x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 21
Tracking Status
firefox19 --- unaffected
firefox20 + verified
firefox21 --- verified

People

(Reporter: sparky, Assigned: mconley)

References

Details

Attachments

(1 file, 2 obsolete files)

Setting browser.download.panel.removeFinishedDownloads = true breaks the download button in various ways:

* The finished notification animation isn't displayed
* The button continues to should some amount of time/progress left even through the download has finished.

I'm not sure when this started happening, but it should probably be fixed for Firefox 20 if it is going to be enabled by default.


STR
1. Set browser.download.panel.removeFinishedDownloads = true
2. Download a file

Expected results:
When the download finishes, the button turns green and the notification animation is displayed.

Actual results:
When the download finishes, the button continues to display some time remaining and/or the progress bar. Clicking the button resets it to the default appearance.
I agree, a simple workaround would be to just not support that pref for 20, in case a proper fix is complicated.  Let's see.
I think the first issue is minor, though the wrong progressbar is not.
Confirmed on Ubuntu and OSX. Investigating...
Assignee: nobody → mconley
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Attached patch Patch v1 (obsolete) — Splinter Review
It looks like when browser.download.panel.removeFinishedDownloads is true, the download is removed *during* the state change from in-progress to complete, and so we get notified of its removal *before* we hear about it changing state to complete.

I believe this confuses our indicator code, since it gets a state change update when there are no downloads underway.

This patch makes the indicator data provider a little smarter in that it wipes out the counter and progress values when there are no more downloads in progress.

This doesn't, however, cause the indicator to get attention when the download completes - and this might be fine, since I don't know if we really want the download indicator to get attention on a completed download, since the button itself won't display anything useful after clicking it.
Attached patch Patch v1.1 (obsolete) — Splinter Review
This seems to fix the bug.
Attachment #702007 - Attachment is obsolete: true
Attachment #702011 - Flags: review?(paolo.mozmail)
The fact is that, for the indicator only, we enumerate the dataItems directly
from the DownloadsData object (without keeping our own copy of the list) during
the removal notification. But, we invoke the removal notification before we
actually removed the dataItem from the list.

Probably, moving the removal as the first line in _removeDataItem, before
onDataItemRemoved is called, will fix the issue.

I then suggest double-checking that the Library view isn't regressed by this
change, but I think not because it properly keeps its own copy of the dataItems,
it doesn't seem to use the global list.
Comment on attachment 702011 [details] [diff] [review]
Patch v1.1

Mike, if the approach in comment 6 works, please go for it instead of this
workaround - no need to ask for review again unless you just want more eyes
on the new patch. Otherwise, feel free to land the workaround.
Attachment #702011 - Flags: review?(paolo.mozmail) → review+
This change does indeed fix the problem - thanks for clearing that up!

Library doesn't seem to be affected at all.
Attachment #702011 - Attachment is obsolete: true
Comment on attachment 702460 [details] [diff] [review]
Patch v2 (r+'d by paolo)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): downloads panel feature
User impact if declined: broken ui
Testing completed (on m-c, etc.): m-i, merge pending
Risk to taking this patch (and alternatives if risky): limited to the feature
String or UUID changes made by this patch: none
Attachment #702460 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c9e635c7c6eb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Attachment #702460 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I confirm the fix is verified on FF 20.b1. Verified fixed due to Testday 20130222 too.

Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
(In reply to MarioMi (:MarioMi) from comment #13)

Verified fixed on FF21b1.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0

Builds ID: 20130401192816
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.