Download Manager page is not updated after clearing private data

VERIFIED FIXED in Firefox 21

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: AdrianT, Assigned: bnicholson)

Tracking

({productwanted})

17 Branch
Firefox 21
ARM
Android
productwanted
Points:
---

Firefox Tracking Flags

(firefox15 affected, firefox16 affected, firefox17 affected, firefox18 affected, relnote-firefox 21+, fennec20+)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
It does not listen for changes.
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.
Summary: Download Manger page is not refreshed after clearing private data → Download Manger page is not updated after clearing private data
Product wanted to make a call on if we want to delete the users downloads or not
tracking-fennec: ? → 15+
Keywords: productwanted
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.

Updated

5 years ago
Keywords: productwanted

Updated

5 years ago
status-firefox18: --- → affected
tracking-fennec: 15+ → ?
Flags: needinfo?(krudnitski)
Keywords: productwanted
Assignee: nobody → bnicholson
tracking-fennec: ? → 19+
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.
Flags: needinfo?(krudnitski)
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Updated

5 years ago
Flags: needinfo?(ibarlow)
Brian - Can any of this bbe wrapped into the work you are doing for private browsing?
tracking-fennec: 19+ → 20+
(Assignee)

Comment 8

4 years ago
(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.
(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).
Flags: needinfo?(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 :)
Summary: Download Manger page is not updated after clearing private data → Download Manager page is not updated after clearing private data
(Assignee)

Comment 11

4 years ago
Created attachment 709307 [details] [diff] [review]
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.
Attachment #709307 - Flags: review?(mark.finkle)
(Assignee)

Comment 12

4 years ago
Created attachment 709308 [details] [diff] [review]
Part 2: Move sanitize.js to Sanitizer.jsm
Attachment #709308 - Flags: review?(mark.finkle)
(Assignee)

Comment 13

4 years ago
Created attachment 709310 [details] [diff] [review]
Part 3: Add ability to delete downloaded files
Attachment #709310 - Flags: review?(mark.finkle)
(Assignee)

Comment 14

4 years ago
Still remaining is updating about:downloads after clearing private data without refreshing the page.
(Assignee)

Comment 15

4 years ago
Created attachment 709945 [details] [diff] [review]
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
Attachment #709310 - Attachment is obsolete: true
Attachment #709310 - Flags: review?(mark.finkle)
Attachment #709945 - Flags: review?(mark.finkle)
(Assignee)

Comment 16

4 years ago
Created attachment 709946 [details] [diff] [review]
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.
Attachment #709946 - Flags: review?(wjohnston)
Attachment #709307 - Flags: review?(mark.finkle) → review+
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(...)
Attachment #709308 - Flags: review?(mark.finkle) → review+
Attachment #709945 - Flags: review?(mark.finkle) → review+
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.
Attachment #709946 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Comment 21

4 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21

Updated

4 years ago
relnote-firefox: --- → ?

Comment 23

4 years ago
Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-18)
-device: HTC Desire 
-OS: Android 2.2
Status: RESOLVED → VERIFIED

Updated

4 years ago
relnote-firefox: ? → 21+

Comment 24

4 years ago
I no spick inglésh.bot thank berimosh

Comment 25

4 years ago
hi I no spick inglish bot thánks somosh moshXxxxx
You need to log in before you can comment on or make changes to this bug.