777639 - Download Manager page is not updated after clearing private data

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Firefox Mobile Native 15.0b2 build 1/ Nightly 17.0a1 2012-07-25/ Aurora 16.0a2 2012-07-25
Device: HTC Desire (Android 2.2)/Motorola Droid Pro (Android 2.3.4)

Steps to reproduce:
1. Download a few files to create a download history
2. Open the download manager
3. Go to "Settings" -> Clear private data
4. Return to the Download Manager

Expected results:
The download history is cleared

Actual results:
The download manager needs to be reloaded

Notes:
The issue could have more impact if the user has the Download Manager opened in another tab which is not the active tab. Opening the Download Manager from the menu will switch to the Download Manager tab where the info is not updated.

Comment 1

[Aaron Train [:aaronmt]]

It does not listen for changes.

Comment 2

[Wesley Johnston (:wesj)]

I just want to remove refresh from the title here. It makes me think of refrshing the page. We can listen to the nsIDownloadManager for changes and remove items when they're removed.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Product wanted to make a call on if we want to delete the users downloads or not

Comment 4

[Karen Rudnitski [:kar]]

I (as a user) would assume that if I cleared private data, this would include what I have downloaded - otherwise I would have to go specifically to the download manager and ensure everything was cleared (I would care if I'm passing the phone over to lend to a friend, switching phones, etc). Therefore I would suggest that we want to update the download manager and clear the data there.

Comment 5

[Karen Rudnitski [:kar]]

There is a user expectation that when they clear private data, that means all the data would be removed from their browsing session(s), including files downloaded to the phone. I would therefore like to see that this action also removes files that were downloaded from the phone.

I would assume this to be in the form of an added line in the 'clear private data' screen stating 'downloaded files' as a selectable option.

Comment 6

[Brian Nicholson (:bnicholson)]

So it sounds like we're discussing two bugs here:

1) The bug that was originally filed was the clearing private data isn't immediately clearing the list of downloads in the download manager.

2) The files themselves would be deleted as one of the options when clearing private data. Some thoughts:
* Perhaps this checkbox item in "Clear private data" should be disabled by default? On desktop, checking all items in the "Clear Recent History" dialog doesn't delete any files, so the different behavior on mobile could surprise users (and delete files they didn't want to delete).
* Having separate "Browsing & download history" and "Downloaded files" entries is a bit confusing.

Also, what's the expected behavior if someone clears the browsing & download history first, then clears the downloaded files separately after that?

CC'ing Ian for desired UX.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Brian - Can any of this bbe wrapped into the work you are doing for private browsing?

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Brian - Can any of this bbe wrapped into the work you are doing for private
> browsing?

If we only need to remove items from the downloads list (and not the files themselves), it should be very simple to listen for some notification to clear entries like we do with private browsing. There may be overlap with deleting files when they're cleared, but I haven't seen a definitive answer to whether (or how) we should do that here.

Comment 9

[Ian Barlow (:ibarlow)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> 1) The bug that was originally filed was the clearing private data isn't
> immediately clearing the list of downloads in the download manager.

I agree that we should fix this so that you don't have to hit reload to see your download history cleared out. 
 
> 2) The files themselves would be deleted as one of the options when clearing
> private data. Some thoughts:
> * Perhaps this checkbox item in "Clear private data" should be disabled by
> default? On desktop, checking all items in the "Clear Recent History" dialog
> doesn't delete any files, so the different behavior on mobile could surprise
> users (and delete files they didn't want to delete).
> * Having separate "Browsing & download history" and "Downloaded files"
> entries is a bit confusing.

I'm not sure I agree that this is confusing -- I think mobile device users are generally aware that files get handled differently on their mobile devices than they do on their desktop machines, so somewhat inconsistent behaviour isn't necessarily a bad thing here -- we are simply optimizing for a different environment.

I also wouldn't leave a "downloaded files" item as the only unchecked thing in that list. It feels like an arbitrary choice for a list that is otherwise completely checked off.

So we would end up with a list that looks something like this:

-------------------------------
Clear private data
Browsing & download history [x]
Form & search history       [x]
Cookies & active logins     [x]
Saved passwords             [x]
Cache                       [x]
Offline website data        [x]
Site preferences            [x]
Downloaded files            [x]
-------------------------------

Where "Browsing & download history" clears both history and download lists, and does so without the user having to refresh the Downloads panel, 

and

"Downloaded files" clears both the actual files, and the download list itself just in case (since it is unlikely you would ever want to keep the list of downloads after having removed the files themselves).

Comment 10

[Ian Barlow (:ibarlow)]

In addition, while I am ok to make this change right now for the sake of this bug, I think we can all agree that using a single popup menu to manage all these private data choices like this is no longer a good design solution. The list is just too dense and too long to help users make informed choices about their personal data anymore.

It's probably about time to revisit how we handle this in Settings -- and probably the rest of our Settings UI architecture while we are at it :)

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Remove unused code from Sanitizer

I don't think we include http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js, so a lot of the logic in sanitize.js isn't getting used.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Move sanitize.js to Sanitizer.jsm

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Add ability to delete downloaded files

Comment 14

[Brian Nicholson (:bnicholson)]

Still remaining is updating about:downloads after clearing private data without refreshing the page.

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Add ability to delete downloaded files, v2

In the previous patch, the Sanitizer and dlm were independently querying the downloads database, so there was a (small) potential race condition where a file could finish downloading between being deleted and removed from history. If that happened, a downloaded file would be removed from the downloads list, but not from the device.

With this patch, we no longer call dlm.cleanUp(), and we manually call remove() on each download in the database. This does the deletion/history removal in the same iteration, so there's no potential race as described above. And since we're calling dl.remove() for each download, it also means we can fix the other part of this bug by just adding a download remove listener to handle removed downloads (patch forthcoming).

The downside of this patch is that it may be less efficient. dl.remove() is called on every download, which means we're deleting downloads from the database individually rather than using a single SQL statement like we do here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2018

Comment 16

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Listen for removed downloads in about:downloads

Adds a "download-manager-remove-download-guid" observer to about:downloads so that we can listen for downloads removed elsewhere.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 709308 [details] [diff] [review]
Part 2: Move sanitize.js to Sanitizer.jsm


>diff --git a/mobile/android/modules/Makefile.in b/mobile/android/modules/Makefile.in

> EXTRA_JS_MODULES = \

>   linuxTypes.jsm \
>   video.jsm \

We could probably remove these in a new bug

> EXTRA_PP_JS_MODULES = \
>   contacts.jsm \

And this one too

>diff --git a/mobile/android/chrome/content/sanitize.js b/mobile/android/modules/Sanitizer.jsm

>+function dump(a) {
>+  Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(a);
>+}

>+        var os = Cc["@mozilla.org/observer-service;1"]
>+                   .getService(Components.interfaces.nsIObserverService);
>         os.notifyObservers(null, "net:clear-active-logins", null);

Remove the var os ... and just use Services.os.notifyObservers(...)

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 709946 [details] [diff] [review]
Part 4: Listen for removed downloads in about:downloads

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

::: mobile/android/chrome/content/aboutDownloads.js
@@ -458,5 @@
>        } catch (ex) {
>          this.logError("removeDownload() " + ex, aDownload);
>        }
>      }.bind(this));
> -    aItem.parentNode.removeChild(aItem);

We should probably fade the row or something while its being removed. But we should also probably show a useful error message to the user if the remove fails.

Comment 19

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #18)
> Comment on attachment 709946 [details] [diff] [review]
> Part 4: Listen for removed downloads in about:downloads
> 
> We should probably fade the row or something while its being removed. But we
> should also probably show a useful error message to the user if the remove
> fails.

Filed bug 838366.

Comment 20

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> Comment on attachment 709308 [details] [diff] [review]
> Part 2: Move sanitize.js to Sanitizer.jsm
> 
> >diff --git a/mobile/android/modules/Makefile.in b/mobile/android/modules/Makefile.in
> 
> > EXTRA_JS_MODULES = \
> 
> >   linuxTypes.jsm \
> >   video.jsm \
> 
> We could probably remove these in a new bug

Filed bug 838368.

Comment 21

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffeac43b95e
https://hg.mozilla.org/integration/mozilla-inbound/rev/997812c6023d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba2e7dc2951
https://hg.mozilla.org/integration/mozilla-inbound/rev/27d51d9f7495

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ffeac43b95e
https://hg.mozilla.org/mozilla-central/rev/997812c6023d
https://hg.mozilla.org/mozilla-central/rev/9ba2e7dc2951
https://hg.mozilla.org/mozilla-central/rev/27d51d9f7495

Comment 23

[Andreea Pod]

Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-18)
-device: HTC Desire 
-OS: Android 2.2

Comment 24

[bbello56]

I no spick inglésh.bot thank berimosh

Comment 25

[bbello56]

hi I no spick inglish bot thánks somosh moshXxxxx