Closed Bug 836271 Opened 7 years ago Closed 7 years ago

The progress animation in the Downloads view keeps on loading until the view is closed

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- verified

People

(Reporter: simona.marcu, Assigned: mano)

References

Details

Attachments

(1 file)

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130129 Firefox/21.0
Build ID: 20130129030851

Prerequisites: In about:config - set the preference browser.download.panel.removeFinishedDownloads to true and restart Firefox

Steps to reproduce:
1. Launch Firefox 
2. Perform a download
3. Open the Downloads View by clicking on Show All Downloads in the downloads panel
4. Without closing the Downloads View - perform several more downloads.

Expected results:
The downloads are properly made and displayed in the Downloads View.

Actual results:
The downloads have an unknown size in the Downloads View and the progress animation is loading until the Library window is closed.

Please see the screencast for more details: http://screencast.com/t/CN5W9H8FN3
I'm wondering if we should change the way browser.download.panel.removeFinishedDownloads works, the current implementation doesn't make much sense.
It basically makes the panel totally pointless, while I think the scope of the user was probably to not have download history remembered across sessions.

use-cases:
1. keep the list clean. There is basically no reason (the old DM had some perf reasons though) to do this, apart psychological need to constantly have clean environment. I actually suffer that personally, but since the panel is limited to 3 items and commonly closed, I don't feel it badly.
2. don't store downloads history. We don't have a good support for this, maybe the pref should just make all downloads info per session, that means it should stop saving downloads history. We could split Remember browsing and downloads history (not in Aurora though!), though that would be bogus since disabling history also disables downloads history, but not viceversa (though we may indent download history under browsing history, so
    [] Remember my browsing history
       [] Remember downloads
3. privacy. Basically private browsing covers this need and is the intended way to keep private downloads, so this is likely not a good use-case to keep supporting.

I think the most requested use-case is 2, on the other side, it's hard to figure why one would want this behavior only for a subset of history? This adds sort of a special case to history expiration that has uncertain benefits.

If we can't figure the right use-case or make an Aurora backportable patch, we should probably just stop supporting that pref for now and evaluate it proper for the next version.
A plausible backportable fix could be, if the pref is set, to not store downloads history, and enqueue the removeDownload call from here (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2733) so it properly notifies removal after state change.
As I said this makes the panel pointless: will notify a download completed, but when you open the panel it would be empty.
(In reply to Marco Bonardo [:mak] from comment #1)
> If we can't figure the right use-case or make an Aurora backportable patch,
> we should probably just stop supporting that pref for now and evaluate it
> proper for the next version.

I second this, following your detailed analysis in comment 1. Let's evaluate if
there is still a valid use case for the feature now that the panel has been
redesigned from its original version. By the way, this has always been an
about:config only preference.

We can just remove it from firefox.js, and always return 0 here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsStartup.js#125
(In reply to Paolo Amadini [:paolo] from comment #3)
> We can just remove it from firefox.js, and always return 0 here:

I meant "return 2".
I would rather fix this. I'm pretty sure there is a bug in the view that causes this, which we should fix regardless of this preference.
Assignee: nobody → mano
(In reply to Mano from comment #5)
> I would rather fix this. I'm pretty sure there is a bug in the view that
> causes this, which we should fix regardless of this preference.

The fact is, surely we can fix the bug in the view, or we could just enqueue the removeDownload call, so that events are sent in an expected order. But regardless what we do the preference still makes no sense as it is.
Attached patch fix the view bugSplinter Review
As I thought, regardless of the DM issue, this has uncovered a view bug. Tha attached patch fixes it. The reported bug is also fixed for most cases.

Regardless of this patch though, the unexpected order of events makes it so DownloadsCommon cannot store the meta-data annotation (because the state change is not reported).

So, we should take this patch anyway, but either here or in a follow up, we should either:
a) remove support for this preference
b) change the events order.
Attachment #710659 - Flags: review?(mak77)
Both!

(In reply to Mano from comment #7)
> a) remove support for this preference

I think we figured it's not really useful as is, and releasing it to users in this state to rename it later to something more meaningful could be a mistake. I suppose we are going to have complains both ways.
Probably safer to just remove it at this point, in a separate bug and file a further bug to introduce a better pref or alternative "always forget downloads" mode.
I'd be fine to do the removal in this bug and have a separate bug to implement a meaningful pref (future).

> b) change the events order.

This is likely something we should do, the current order doesn't make any sense unless it was done for a specific reason in the old DM window (changing this order also changes order for it). So, I'd say separate bug, investigate if the old DM window is happy about the order change, and fix it.
Comment on attachment 710659 [details] [diff] [review]
fix the view bug

Review of attachment 710659 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below comments addressed:

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +930,5 @@
>      }
>  
>      if (shouldCreateShell) {
>        let shell = new DownloadElementShell(aDataItem, aPlacesNode,
> +                                           aPlacesNode ? this._getAnnotationsFor(downloadURI) : null);

this deserves:
1. let cachedAnnotation = aPlacesNode ? this._getAnnotationsFor(downloadURI) : null;
2. a really nice comment above that, about why we need to skip the cache when the places node is not defined
Attachment #710659 - Flags: review?(mak77) → review+
actually "let cachedAnnotations" (plural)
Comment on attachment 710659 [details] [diff] [review]
fix the view bug

[Approval Request Comment]
Bug caused by (feature/regressing bug #): downloads panel feature
User impact if declined: broken ui (until Library closed and reopened)
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): limited to the feature
String or UUID changes made by this patch: none
Attachment #710659 - Flags: approval-mozilla-aurora?
Attachment #710659 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/25de4a425e88
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Verified fixed on the latest beta, Firefox 20 beta 6. (Build ID: 20130320062118)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
User Agent: Mozilla/5.0 (Windows NT 6.2; rv:20.0) Gecko/20100101 Firefox/20.0
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Based on Comment 15 setting the status for Firefox 20 to verified.
You need to log in before you can comment on or make changes to this bug.