830205 - Download button shows progress remaining after download finishes

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Matthew Turnbull [Bluefang]]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Marco Bonardo [:mak]]

I think the first issue is minor, though the wrong progressbar is not.

Comment 3

[Mike Conley (:mconley) (:⚙️) (PTO June 8 - 12)]

Confirmed on Ubuntu and OSX. Investigating...

Comment 4

[Mike Conley (:mconley) (:⚙️) (PTO June 8 - 12)]

Attachment: Patch v1

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.

Comment 5

[Mike Conley (:mconley) (:⚙️) (PTO June 8 - 12)]

Attachment: Patch v1.1

This seems to fix the bug.

Comment 6

[:Paolo Amadini]

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 7

[:Paolo Amadini]

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.

Comment 8

[Mike Conley (:mconley) (:⚙️) (PTO June 8 - 12)]

Attachment: Patch v2 (r+'d by paolo)

This change does indeed fix the problem - thanks for clearing that up!

Library doesn't seem to be affected at all.

Comment 9

[Mike Conley (:mconley) (:⚙️) (PTO June 8 - 12)]

Landed Paolo's fix on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e635c7c6eb

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c9e635c7c6eb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18d4e50896ca

Comment 13

[Mihai Morar, (:MihaiMorar)]

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

Comment 14

[Mihai Morar, (:MihaiMorar)]

(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