Closed Bug 814099 Opened 9 years ago Closed 9 years ago

Paused downloads do not behave properly in panel after the session is restored.

Categories

(Firefox :: Downloads Panel, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 20

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(1 file, 3 obsolete files)

STR:

1) Start a download, and then pause it.
2) Restart Firefox
3) Open the Downloads Panel, right-click on the paused item, and choose to Resume it.
4) Right-click on the downloading item again.

What happens?

"Resume" is still an option in the context menu, and "Pause" is missing.

The ability to cancel the download is missing from the list. If you click on the "X" to cancel the download, the downloading item does not reflect the cancelled status - it just ceases to receive progress updates.
Attached patch Patch v1 (obsolete) — Splinter Review
It turns out that we prep the dataItem in DownloadsCommon with a getter for the nsIDownload if we load it from the database row.

Changing states overwrites the download attribute - but we can't do that if it's a getter. We have to delete the getter first.

At least, that's my understanding of it.
Assignee: nobody → mconley
So, this looks like a regression from bug 808277. Why do we need to assign the
"download" property of the data item explicitly in the first place? Is this just
an optimization, or is it now needed for the summary view? In either case, a
comment there might be helpful.

In fact, the "download" property is rarely accessed directly, so if it's an
optimization, I'm not sure whether it's better to delete and re-assign the
property for each status update, or just let getDownload be executed later.
(In reply to Paolo Amadini [:paolo] from comment #2)
> So, this looks like a regression from bug 808277. Why do we need to assign
> the
> "download" property of the data item explicitly in the first place? Is this
> just
> an optimization, or is it now needed for the summary view? In either case, a
> comment there might be helpful.

So yes, I introduced this in bug 808277, because I noticed that if I cancelled and restarted a download that was part of the summarized group, the summary would not reflect the status / progress of the restarted download.

Drilling into this, I noticed that this was because the restarted download that was being stored in the dataItem's inside of the DownloadsSummaryData was still referring to a download that was cancelled. I assumed that this was because the reference that the dataItem was holding onto was invalidated, and needed to be updated.

We don't hit this same problem with the indicator, since the indicator iterates over Services.downloads.activeDownloads as opposed to an internal collection of dataItems.

So, my solution was to update the reference. Does that sound like the right approach to you?

Note that the time that the internal download is accessed that I'm concerned about is in the generator _downloadsForSummary in DownloadsSummaryData.
That's true, when a download is retried we call GetDownloadFromDB in
DownloadManager.cpp, creating a different download object from the database.

It makes sense to update the reference in the dataItem, since the download has
the same ID and the new object is added to the list of active downloads.
So maybe, detect whether dataItem.download is a getter and if not replace it? This would be really really explicit, and if it's still a getter we don't need to do anything.

A nice comment above it explaining the reason we update the reference would be welcome, the current comment in the patch doesn't seem to be as clear as the comments here in the bug.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #684174 - Attachment is obsolete: true
Attachment #684174 - Flags: review?(mak77)
Comment on attachment 684456 [details] [diff] [review]
Patch v2

Thanks Paolo! Ok, I've updated the comment. Is this more clear?
Attachment #684456 - Flags: review?(paolo.mozmail)
isgetter = Object.getOwnPropertyDescriptor(dataItem, "download").value === undefined
Attached patch Patch v3 (obsolete) — Splinter Review
Ok, cool - I'll use Marco's suggestion here.
Attachment #684456 - Attachment is obsolete: true
Attachment #684456 - Flags: review?(paolo.mozmail)
Comment on attachment 684466 [details] [diff] [review]
Patch v3

So I'm not sure who to direct this review request to, so I'll just default to Marco.
Attachment #684466 - Flags: review?(mak77)
Comment on attachment 684466 [details] [diff] [review]
Patch v3

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

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +764,5 @@
> +    // it alone - the next time the download property is accessed, the right
> +    // download object will be retrieved.
> +    //
> +    // Otherwise, we overwrite the download property with the passed in
> +    // nsIDownload.

I think this last phrase is just repeating concepts from the first one ("might now need updating"), so I'd remove it.

@@ +767,5 @@
> +    // Otherwise, we overwrite the download property with the passed in
> +    // nsIDownload.
> +    let isGetter = Object.getOwnPropertyDescriptor(dataItem, "download")
> +                         .value === undefined;
> +    if (!isGetter) {

I'm sorry that I may have misguided you, I was leaving and didn't have enough time to comment properly. I meant "the usual way we do is-getter check is...", and ended up suggesting a temp isGetter var with pseudocode :)

Btw, now that I look at it, it seems to add readability value. So maybe just rename to downloadIsGetter (otherwise I'm fine with just putting the condition in the if).
Attachment #684466 - Flags: review?(mak77) → review+
Ok, gotcha. I agree, I prefer the alias for readability - so I've kept it in.

Thanks!
Attachment #684466 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/78ed9104dadd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Verified on the latest Nightly that the paused downloads behave properly after the session is restored. After restoring the session and resuming the download, the items in the context menu are:
Pause
Open Containing Folder/Show In Finder(on Mac OS X)
Go To Download Page
Copy Download
Clear List.

Verified on Ubuntu 12.10, Mac OS X 10.7 and Windows 7:
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20121203 Firefox/20.0 Build ID: 20121203030801
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20121203 Firefox/20.0 Build ID: 20121203030801
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121202 Firefox/20.0 Build ID: 20121202030723
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.