Last Comment Bug 777639 - Download Manager page is not updated after clearing private data
: Download Manager page is not updated after clearing private data
Status: VERIFIED FIXED
: productwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 17 Branch
: ARM Android
: -- normal (vote)
: Firefox 21
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-26 00:02 PDT by Adrian Tamas (:AdrianT)
Modified: 2016-07-29 14:28 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected
affected
21+
20+


Attachments
Part 1: Remove unused code from Sanitizer (3.86 KB, patch)
2013-02-01 17:32 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
Part 2: Move sanitize.js to Sanitizer.jsm (6.99 KB, patch)
2013-02-01 17:33 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
Part 3: Add ability to delete downloaded files (8.06 KB, patch)
2013-02-01 17:34 PST, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Part 3: Add ability to delete downloaded files, v2 (8.20 KB, patch)
2013-02-04 15:42 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
Part 4: Listen for removed downloads in about:downloads (3.99 KB, patch)
2013-02-04 15:44 PST, Brian Nicholson (:bnicholson)
wjohnston2000: review+
Details | Diff | Splinter Review

Description Adrian Tamas (:AdrianT) 2012-07-26 00:02:40 PDT
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] 2012-07-26 07:23:39 PDT
It does not listen for changes.
Comment 2 Wesley Johnston (:wesj) 2012-07-26 09:07:06 PDT
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?) 2012-08-09 10:44:17 PDT
Product wanted to make a call on if we want to delete the users downloads or not
Comment 4 Karen Rudnitski [:kar] 2012-08-31 07:05:49 PDT
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] 2012-10-29 05:36:43 PDT
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) 2012-11-21 12:08:47 PST
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?) 2012-12-20 10:58:58 PST
Brian - Can any of this bbe wrapped into the work you are doing for private browsing?
Comment 8 Brian Nicholson (:bnicholson) 2012-12-20 12:55:28 PST
(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) 2013-01-03 19:35:46 PST
(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) 2013-01-03 19:40:29 PST
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) 2013-02-01 17:32:55 PST
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.
Comment 12 Brian Nicholson (:bnicholson) 2013-02-01 17:33:38 PST
Created attachment 709308 [details] [diff] [review]
Part 2: Move sanitize.js to Sanitizer.jsm
Comment 13 Brian Nicholson (:bnicholson) 2013-02-01 17:34:55 PST
Created attachment 709310 [details] [diff] [review]
Part 3: Add ability to delete downloaded files
Comment 14 Brian Nicholson (:bnicholson) 2013-02-01 17:35:54 PST
Still remaining is updating about:downloads after clearing private data without refreshing the page.
Comment 15 Brian Nicholson (:bnicholson) 2013-02-04 15:42:56 PST
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
Comment 16 Brian Nicholson (:bnicholson) 2013-02-04 15:44:32 PST
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.
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2013-02-04 21:27:27 PST
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) 2013-02-05 12:37:19 PST
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) 2013-02-05 16:00:57 PST
(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) 2013-02-05 16:04:00 PST
(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 23 Andreea Pod 2013-02-18 06:22:23 PST
Verified fixed on:
-build:  Firefox for Android 21.0a1 (2013-02-18)
-device: HTC Desire 
-OS: Android 2.2
Comment 24 bbello56 2013-05-24 13:33:16 PDT
I no spick inglésh.bot thank berimosh
Comment 25 bbello56 2013-05-24 13:51:27 PDT
hi I no spick inglish bot thánks somosh moshXxxxx

Note You need to log in before you can comment on or make changes to this bug.