392152 - Clear Private Data key combination doesn't clear downloads when DM is open (new DM)

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Scott A.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081305 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081305 Minefield/3.0a8pre

When download manager is open, clearing private data (shift-ctrl-del) does not clear the download history until DM is closed.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Scott A.]

As a follow up to this, clear private data doesn't even work when the DM is focused.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081321 Minefield/3.0a8pre

When I close the Download Manager and reopen it, it is empty. You mean it should visibly erase the download history?

Comment 3

[Ria Klaassen (not reading all bugmail)]

I see; confirmed. This is only about the key combination, not about the "Clear Private Data" menu option. They react differently.

Comment 4

[Shawn Wilsher :sdwilsh]

Are we really sure that this regressed from Bug 388517?  I don't think it did...

Comment 5

[Ria Klaassen (not reading all bugmail)]

I just assumed, because it regressed only a few days ago, so I retested and it was indeed caused by Bug 388517.

Comment 6

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

Scott, what's your value in about:config for privacy.item.downloads ?

If that pref is set to |true|, is the checkbox for "Download History" checked, but the menu item greyed out in "Tools | Clear Private Data"?

Comment 7

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

Actually, I wasn't completely understanding how this pref works.  It appears that the "Clear Private Data" dialog knows if we have download history, and then enables/disables the ability to toggle the checkbox for "Download History" based on that.  This actually wasn't working for me at first only because I had a stuck download (we can obviously only clear the "Completed" downloads, whether that be "Cancelled," "Failed," or "Done," etc...)

That said, I still see this failing with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre; I have two "Done" downloads, and when I pull up Clear Private Data, "Download History" is disabled, and so clicking on "Clear Private Data Now" does nothing.

Seems to me that the state of the downloads isn't being kept or passed to the Clear Private Data window correctly (or, not read).

Comment 8

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

http://litmus.mozilla.org/show_test.cgi?id=4635

in-litmus+

Comment 9

[Shawn Wilsher :sdwilsh]

Is this with the download manager window open?  I'd suspect that it doesn't remove things from an open window, but if you reopen it, it should work.

Comment 10

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

(In reply to comment #9)
> Is this with the download manager window open?  I'd suspect that it doesn't
> remove things from an open window, but if you reopen it, it should work.

Yes, and Scott said as much in comment 0.  I'm just particularly daft :( 

Comment 11

[Scott A.]

(In reply to comment #6)
> Scott, what's your value in about:config for privacy.item.downloads ?
> 
> If that pref is set to |true|, is the checkbox for "Download History" checked,
> but the menu item greyed out in "Tools | Clear Private Data"?
> 
Not in windows right now but I believe that value is for the privacy options, so it would have been set to true (I have everything set to true except for cookies as I clear them separately).

The only issue related to this bug is that the download clearing status is not updated until the DM is closed and re-opened (which can be somewhat of a privacy issue and also could make users believe it didn't work)... The secondary issue I had but never retested recently was the key combo not even working at all when DM is focused (key combo should still be passed).

Comment 13

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

Attachment: patch

This is a bit hacky, because it makes the browser sanitize code depend on a function defined in the DM code in a non-obvious way, but it certainly is simple. We could alternatively use an observer topic, or something. I thought of having the DM notify the front-end via nsIDownloadManagerUI from it's CleanUp function, but the nsIDownloadManagerUI implementation isn't really any closer to the DM frontend code than sanitize.js is.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 282552 [details] [diff] [review]
patch

I'd honestly prefer to see an observer type thing go on so that add-ons can provide completely new UI and still work correctly.  This will work, but like you said, it's hacky.

Comment 15

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

Attachment: observer service

Should we also clear the "search" field?

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 283014 [details] [diff] [review]
observer service

>+
>+        // Clear the UI if there's an open DM window
>+        var obs = Components.classes["@mozilla.org/observer-service;1"].
>+                  getService(Components.interfaces.nsIObserverService);
>+        obs.notifyObservers(null, "download-manager-clear-history", "");
Hrm - shouldn't this be a part of the download manager api?  This way if some add-on calls the function, it updates the UI as well?

As for the search field, I wouldn't bother since that seems to have been removed with the latest mock-ups from the UX team.

Comment 17

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

(In reply to comment #16)
> Hrm - shouldn't this be a part of the download manager api?  This way if some
> add-on calls the function, it updates the UI as well?

You mean have the sanitize code call a DM method that notifies the front end code via the observer service?

Comment 18

[Shawn Wilsher :sdwilsh]

It already calls the method - cleanup

Comment 19

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

Attachment: updated

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 283076 [details] [diff] [review]
updated

r=sdwilsh

Comment 21

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

mozilla/toolkit/mozapps/downloads/content/downloads.js 	1.90
mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 	1.128

Comment 22

[Shawn Wilsher :sdwilsh]

Can you add some information to the download manager's idl stating that an observer topic is dispatched for cleanup please?  I forgot to mention that in my review.

Comment 23

[Shawn Wilsher :sdwilsh]

rs=sdwilsh on the previously mentioned changed

Comment 24

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

Attachment: documentation

Comment 25

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

mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl 	1.21 

Comment 26

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

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100505 Minefield/3.0a9pre and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007100504 Minefield/3.0a9pre