971743 - Problem with downloads and "Clear private data"

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Christian Ascheberg]

STR:
1. download file in Android Firefox
2. go to privacy settings -> "Clear private data"
3. check option "Browsing & download history" but do NOT select "Downloaded files"
4. confirm

Result:
The downloaded file is kept on the device, but there is no reference to it, neither in Firefox nor in Android. It is only accessible with a third-party filebrowser or by PC via USB.

Suggestion:
Do not clear the download history with the browsing history. The way this UI is designed at the moment makes it impossible to delete the browsing history while keeping downloads.

Comment 1

[Aaron Train [:aaronmt]]

This is a known issue. We're going to be registering downloads with the system manager very shortly.

Comment 2

[Christian Ascheberg]

Attachment: possible patch

I was thinking that taking a patch now would fix this bug in an earlier release. Also from bug 816318 comment 19 it is not clear to me yet if the integrated download manager will be removed completely.
I think the patch is quite easy, although I have to admit that I could not test it. What do you think about this?

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8378940 [details] [diff] [review]
possible patch

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

The patch seems fine, but UX should make the call here on what we do. I also kinda think that we should keep the old behaviour but couple it to deleting the old files.

::: mobile/android/modules/Sanitizer.jsm
@@ -181,5 @@
> -      clear: function ()
> -      {
> -        downloads.iterate(function (dl) {
> -          dl.remove();
> -        });

I think we should probably move this logic into the downloadFiles section and clean that up when you select "Downloaded Files" (maybe we could rename that to "Downloads").

Comment 4

[Christian Ascheberg]

(In reply to Wesley Johnston (:wesj) from comment #3)
> I also kinda think that we should keep the old behaviour but couple it to deleting the old files.

What do you mean by that?
In the current implementation you only have options to
1. delete the history *and* download references (which does not delete the actual files)
and/or 2. delete download references and the actual files

That makes it impossible to keep your downloads if you want to clear the history.

The patch modifies the first option so that you should be able to
1. delete *only* the history
and/or 2. delete download references and the actual files

> I think we should probably move this logic into the downloadFiles section
> and clean that up when you select "Downloaded Files"

That code is already in the downloadFiles section as well, and that is exactly what happens.

Comment 5

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #3)
> Comment on attachment 8378940 [details] [diff] [review]
> possible patch
> 
> Review of attachment 8378940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch seems fine, but UX should make the call here on what we do. I also
> kinda think that we should keep the old behaviour but couple it to deleting
> the old files.
> 

Wes, can you elaborate -- I'm not clear on what is being proposed here. Thanks

Comment 6

[Christian Ascheberg]

needinfo based on comment 4 and comment 5

Comment 7

[Wesley Johnston (:wesj)]

Oh, Sorry, you are right Christian. So ian, Right now we have two options in our Clear dialog:

Browsing & Download History - This will clear the Downloads database, but doesn't touch the files
Downloads - This will clear the Downloads database and the downloaded files

I think what Christian is saying (and I agree we should do) is just get rid of the Downloads from "Browsing & Download History" (i.e. it just becomes "Browinsg History"). Its confusing to have two similar things, and most users aren't going to care about having both.

Even with the new dm manager, we'll probably want to do what we can to clear the system download manager when Clear Private Data is called.

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Oh, Sorry, you are right Christian. So ian, Right now we have two options in
> our Clear dialog:
> 
> Browsing & Download History - This will clear the Downloads database, but
> doesn't touch the files
> Downloads - This will clear the Downloads database and the downloaded files
> 
> I think what Christian is saying (and I agree we should do) is just get rid
> of the Downloads from "Browsing & Download History" (i.e. it just becomes
> "Browinsg History"). Its confusing to have two similar things, and most
> users aren't going to care about having both.
> 
> Even with the new dm manager, we'll probably want to do what we can to clear
> the system download manager when Clear Private Data is called.

So, just so I'm clear, in this new model:

Clear Browsing History would do just that.
Clear Downloads would clear both the download history (which may not even be relevant anymore with a native download manager) *and* remove the downloaded files. 

That seems reasonable to me.

Comment 9

[Christian Ascheberg]

Exactly. (And even only the code for the first option, clearing "Browsing history", is changed in the patch.)

Comment 10

[Christian Ascheberg]

To clarify my last (maybe badly written) comment:
The patch should do exactly what you said in comment 8. To achieve this, only the first option, clearing "Browsing history", needs to be changed. This is done in the patch. Clearing downloads already behaves as expected.

So who should review the patch?

Comment 11

[Ian Barlow (:ibarlow)]

(In reply to Christian Ascheberg from comment #10)

> So who should review the patch?

Maybe Wes?

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8378940 [details] [diff] [review]
possible patch

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

Yeah, I'll take this one, but this looks pretty good as is! You're right, I misinterpreted this a bit the first time through. And its nice to get rid of some special case code here :)

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +147,5 @@
>  <!ENTITY pref_private_data_passwords "Saved passwords">
>  <!ENTITY pref_private_data_cache "Cache">
>  <!ENTITY pref_private_data_offlineApps "Offline website data">
>  <!ENTITY pref_private_data_siteSettings2 "Site settings">
>  <!ENTITY pref_private_data_downloadFiles "Downloaded files">

Lets rename this to just Downloads.

Comment 13

[Christian Ascheberg]

Attachment: patch v2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5ec886a84e94

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/decb74650baf

https://tbpl.mozilla.org/php/getParsedLog.php?id=37720066&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=37720473&tree=Fx-Team

Comment 16

[Christian Ascheberg]

Attachment: patch v3

Do you know about anything else that might be missing?

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 8405913 [details] [diff] [review]
patch v3

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

A little string nit, but I think we can live with it. Thanks :)

::: mobile/android/base/strings.xml.in
@@ +166,5 @@
>    <string name="pref_restore_quit">&pref_restore_quit;</string>
>    <string name="pref_show_product_announcements">&pref_show_product_announcements;</string>
>    <string name="pref_sync">&pref_sync;</string>
>    <string name="pref_search_suggestions">&pref_search_suggestions;</string>
> +  <string name="pref_private_data_history2">&pref_private_data_history2;</string>

You don't actually need to rev these id's (which makes these changes nice :)) Just the ones in files with actual strings.

Comment 18

[Christian Ascheberg]

(In reply to Wesley Johnston (:wesj) from comment #17)
> You don't actually need to rev these id's (which makes these changes nice
> :)) Just the ones in files with actual strings.

OK, interesting to know. I was basically following the "pref_private_data_cookies2" pattern.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/623749c03065

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/623749c03065