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)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: sheppy, Assigned: Mardak)
References
Details
(Keywords: polish)
Attachments
(2 files, 3 obsolete files)
6.13 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
6.00 KB,
patch
|
Details | Diff | Splinter Review |
When you call nsIDownloadManager:removeDownload(), the download window should refresh to no longer display the removed download, but does not.
Comment 1•17 years ago
|
||
ug - this used to just work because we used the template engine :(
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M9
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 2•17 years ago
|
||
Let's use the hammer gavin made for bug 392152. :)
Assignee | ||
Updated•17 years ago
|
Depends on: 392152
OS: Mac OS X → All
Hardware: Macintosh → All
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Comment 3•17 years ago
|
||
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-
Assignee | ||
Comment 4•17 years ago
|
||
Like so? Return of gDownloadObserver.. :p
Attachment #283128 -
Attachment is obsolete: true
Attachment #283527 -
Flags: review?(comrade693+bmo)
Assignee | ||
Comment 5•17 years ago
|
||
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•17 years ago
|
||
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-
Assignee | ||
Comment 7•17 years ago
|
||
(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 8•17 years ago
|
||
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?
Comment 9•17 years ago
|
||
Hrm, shouldn't we use the same (renamed) topic with a different "data" param, instead of adding a separate observer?
Comment 10•17 years ago
|
||
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•17 years ago
|
||
Sure, I guess that would work too.
Assignee | ||
Comment 12•17 years ago
|
||
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•17 years ago
|
||
I think the former option is best.
Updated•17 years ago
|
Attachment #283848 -
Flags: approval1.9? → approval1.9+
Comment 14•17 years ago
|
||
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+
Assignee | ||
Comment 15•17 years ago
|
||
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 16•17 years ago
|
||
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+
Assignee | ||
Comment 17•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: in-litmus? → in-litmus-
Comment 19•16 years ago
|
||
I'll land this when the tree greens up a bit
Comment 20•16 years ago
|
||
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!
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•