394039 - Calling removeDownload() works but doesn't update download window

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Eric Shepherd [:sheppy]]

When you call nsIDownloadManager:removeDownload(), the download window should refresh to no longer display the removed download, but does not.

Comment 1

[Shawn Wilsher :sdwilsh]

ug - this used to just work because we used the template engine :(

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Let's use the hammer gavin made for bug 392152. :)

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 283128 [details] [diff] [review]
v1

Sorry, but I'd rather see a more applicable topic, and ideally, we send the download id (nsISupportsPRUInt32 I think) with it so that we can only remove that from the UI.

Comment 4

[Ed Lee :Mardak]

Attachment: v2

Like so? Return of gDownloadObserver.. :p

Comment 5

[Ed Lee :Mardak]

Whoops. some reason this didn't get into the qrefresh..
$ hg diff
diff --git a/toolkit/mozapps/downloads/content/downloads.js b/toolkit/mozapps/downloads/content/downloads.js
--- a/toolkit/mozapps/downloads/content/downloads.js
+++ b/toolkit/mozapps/downloads/content/downloads.js
@@ -502,6 +502,7 @@ let gDownloadObserver = {
         buildDefaultView();
         break;
       case "download-manager-remove-download":
+        // Remove a single download
         let id = aSubject.QueryInterface(Ci.nsISupportsPRUint32);
         let dl = getDownload(id.data);
         removeFromView(dl);

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 283527 [details] [diff] [review]
v2

>-  if (aDownload && (pref.getIntPref(PREF_BDM_RETENTION) == 0)) {
you should also remove the constant PREF_BDM_RETENTION up top.

> function removeFromView(aDownload)
> {
>+  if (!aDownload) return;
can you add a comment stating why we return early (because the download might not exist in the view).

Comment 7

[Ed Lee :Mardak]

Attachment: v2.1

(In reply to comment #6)
> >-  if (aDownload && (pref.getIntPref(PREF_BDM_RETENTION) == 0)) {
> you should also remove the constant PREF_BDM_RETENTION up top.
Removed.

> >+  if (!aDownload) return;
> can you add a comment stating why we return early
Commented.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 283848 [details] [diff] [review]
v2.1

r=sdwilsh

This patch is low risk.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Hrm, shouldn't we use the same (renamed) topic with a different "data" param, instead of adding a separate observer? 

Comment 10

[Shawn Wilsher :sdwilsh]

so, we could use the topic we add for this bug, but for cleanup, don't pass a data param?

I kinda like this idea...

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Sure, I guess that would work too.

Comment 12

[Ed Lee :Mardak]

I suppose just to be clear, for this, I'm passing a subject of an nsISupportsPRUint32.

So something like.. (subject: nsISupports, topic: string, data: string)
remove download: <download id>, "download-manager-remove-download", null
remove all: null, "download-manager-remove-download", null
or
remove all: null, "download-manager-remove-download", "all"
but that requires an extra string.

Comment 13

[Shawn Wilsher :sdwilsh]

I think the former option is best.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 283848 [details] [diff] [review]
v2.1

Obsoleting given that we're going to go with a different approach.

Comment 15

[Ed Lee :Mardak]

Attachment: v3

I thought sdwilsh wanted this in a separate bug, but this is fine -- combined the topics and updated idl, downloads.js appropriately. Btw, we're in orange and this is b-ff3+.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 283912 [details] [diff] [review]
v3

r=sdwilsh

I forgot we went back to orange, so yay no need for approval.

Comment 17

[Ed Lee :Mardak]

Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
/cvsroot/mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl,v  <--  nsIDownloadManager.idl
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.136; previous revision: 1.135
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.93; previous revision: 1.92
done

Comment 18

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

Litmus is for end-user/UI-driven tests; does this really have a use-case for front-end testers?  It seems like it's an API call that an extension developer/whitebox tester would use, and therefore is better served by a testsuite entry.

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: testcase

I'll land this when the tree greens up a bit

Comment 20

[Shawn Wilsher :sdwilsh]

dwitte is backing out, so I can land now!

Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
new revision: 1.7; previous revision: 1.6
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_394039.js;
initial revision: 1.1

Comment 21

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

Eric; when you get a chance, could you do the honors and verify that this bug is fixed?  Thanks!