Closed Bug 394039 Opened 17 years ago Closed 17 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: sheppy, Assigned: Mardak)

References

Details

(Keywords: polish)

Attachments

(2 files, 3 obsolete files)

When you call nsIDownloadManager:removeDownload(), the download window should refresh to no longer display the removed download, but does not.
ug - this used to just work because we used the template engine :(
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M9
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: polish
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Attached patch v1 (obsolete) — Splinter Review
Let's use the hammer gavin made for bug 392152. :)
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #283128 - Flags: review?(comrade693+bmo)
Depends on: 392152
OS: Mac OS X → All
Hardware: Macintosh → All
Target Milestone: Firefox 3 M10 → Firefox 3 M9
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.
Attachment #283128 - Flags: review?(comrade693+bmo) → review-
Attached patch v2 (obsolete) — Splinter Review
Like so? Return of gDownloadObserver.. :p
Attachment #283128 - Attachment is obsolete: true
Attachment #283527 - Flags: review?(comrade693+bmo)
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 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).
Attachment #283527 - Flags: review?(comrade693+bmo) → review-
Attached patch v2.1 (obsolete) — Splinter Review
(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.
Attachment #283527 - Attachment is obsolete: true
Attachment #283848 - Flags: review?(comrade693+bmo)
Comment on attachment 283848 [details] [diff] [review]
v2.1

r=sdwilsh

This patch is low risk.
Attachment #283848 - Flags: review?(comrade693+bmo)
Attachment #283848 - Flags: review+
Attachment #283848 - Flags: approval1.9?
Hrm, shouldn't we use the same (renamed) topic with a different "data" param, instead of adding a separate observer? 
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...
Sure, I guess that would work too.
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.
I think the former option is best.
Attachment #283848 - Flags: approval1.9? → approval1.9+
Comment on attachment 283848 [details] [diff] [review]
v2.1

Obsoleting given that we're going to go with a different approach.
Attachment #283848 - Attachment is obsolete: true
Attachment #283848 - Flags: approval1.9+
Attached patch v3Splinter Review
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+.
Attachment #283912 - Flags: review?(comrade693+bmo)
Comment on attachment 283912 [details] [diff] [review]
v3

r=sdwilsh

I forgot we went back to orange, so yay no need for approval.
Attachment #283912 - Flags: review?(comrade693+bmo) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Flags: in-litmus?
Resolution: --- → FIXED
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.
Flags: in-litmus? → in-litmus-
Attached patch testcaseSplinter Review
I'll land this when the tree greens up a bit
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
Flags: in-testsuite? → in-testsuite+
Eric; when you get a chance, could you do the honors and verify that this bug is fixed?  Thanks!
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: