754335 - Request for ability to select which privacy and security items to clear over entire wipe

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[donrhummy]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 2012042100

Steps to reproduce:

1. Tried to clear only recent history of the last hour and leave older stuff untouched. 
2. Tried to clear specific cookies



Actual results:

Was presented with an all-or-nothing option


Expected results:

I should have the same options as Firefox on the desktop

Comment 1

[Aaron Train [:aaronmt]]

Yeah, I'd like this too. Chrome Beta on Android does it nice.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Implement MultiChoicePreference

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Enable clearing individual privacy items

Comment 4

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Created attachment 632951 [details] [diff] [review]
> Part 2: Enable clearing individual privacy items

Brian is there any way I can help you by testing this? Would the nightly version allow me to test your changes?

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Implement MultiChoicePreference, v2

This gets rid of the Map version of getValues() in favor of just returning a boolean array. I included more documentation, and this implementation more closely follows Android's ListPreference.

I also added an mDefaultValues array that allows the checkbox states to be reset if user clicks cancel. This is consistent with how the desktop dialog behaves.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Enable clearing individual privacy items, v2

Renamed some keys/strings, and rebased with changes from the first patch.

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to donrhummy from comment #4)
> Brian is there any way I can help you by testing this? Would the nightly
> version allow me to test your changes?

It won't be available in nightly until it has passed review, but if you'd like, you can try out these changes in this custom build:
http://dl.dropbox.com/u/35559547/fennec-privacy-prefs.apk

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Enable clearing individual privacy items, v2.1

Updated to remove the old clear_history_* and clear_private_* strings.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Enable clearing individual privacy items, v2.2

We still need the pref_clear_private_data string, so adding that back in.

Comment 10

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> It won't be available in nightly until it has passed review, but if you'd
> like, you can try out these changes in this custom build:
> http://dl.dropbox.com/u/35559547/fennec-privacy-prefs.apk

My Initial observations:

1. The initial menu popup gave no indication that it would scroll down and show "settings" option so it took a bit to realize it was "beyond the fold"

2. "Clear private data" should indicate it's a menu that opens more (maybe with "...")

3. No way to clear individual cookies or individual history items

4. There's no indication that it cleared anything, the popup just goes away. It should show a small "tooltip" saying "Data deleted."

Some Tests
-----------
1. Deleting history but leaving cookies - successful

2. Delete cookies but leave history - successful

3. Delete both history & cookies - successful

(Not sure how I test if it deletes offline website data, active logins or the cache)

Hope this helps.

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to donrhummy from comment #10)
> My Initial observations:
> 
> 1. The initial menu popup gave no indication that it would scroll down and
> show "settings" option so it took a bit to realize it was "beyond the fold"

I noticed this too, but unfortunately, I don't think there's much we can do about this. This happens for all lists in Android that overflow past the screen. A scrollbar is shown to the right of the list for a few seconds as a small visual cue that the list can be scrolled.

> 2. "Clear private data" should indicate it's a menu that opens more (maybe
> with "...")

Ian, any input on what we should do here?

> 3. No way to clear individual cookies or individual history items

While this would be a nice feature, I don't think we'll be implementing this as part of this bug. We can file a separate bug to add this, along with other features (such as the ability to choose a time range).

> 4. There's no indication that it cleared anything, the popup just goes away.
> It should show a small "tooltip" saying "Data deleted."

Good idea - I can add this.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Implement MultiChoicePreference, v2.1

This change clones the arrays in setEntries()/setEntryKeys()/setDefaultValues().

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Add toast notification for cleared private data

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Add instance state support to MultiChoicePreference

I noticed that changing orientation while the dialog was up dismissed the dialog, so this patch fixes that. Preference.onSaveInstanceState() requires a preference key to save its state [1], so adding a key to the preference keeps the dialog on rotation. I still had to override onSaveInstanceState/onRestoreInstance to restore the mValues array; I extended BaseSaveState to accomplish this (which is also done in DialogPreference, ListPreference, etc.).

One problem with adding a key to this preference is that we automatically try to sync it with Gecko. Since there is no Gecko-side preference associated with the dialog itself, we don't want to do that. I named the key "android.only.privacy.clear" and added a check in GeckoPreferences for prefs prefixed with "android.only." to work around this.

[1] http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/1.5_r4/android/preference/Preference.java#1479

Comment 15

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

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> (In reply to donrhummy from comment #10)
> > My Initial observations:
> > 
> > 1. The initial menu popup gave no indication that it would scroll down and
> > show "settings" option so it took a bit to realize it was "beyond the fold"
> 
> I noticed this too, but unfortunately, I don't think there's much we can do
> about this. This happens for all lists in Android that overflow past the
> screen. A scrollbar is shown to the right of the list for a few seconds as a
> small visual cue that the list can be scrolled.

The subtle scrollbar fade-in / fade-out is normal Android. That's all we should need to do for now.

> > 2. "Clear private data" should indicate it's a menu that opens more (maybe
> > with "...")
> 
> Ian, any input on what we should do here?

Yes, "..." (the single character version) is normal for "menu will display a dialog"

> > 3. No way to clear individual cookies or individual history items
> 
> While this would be a nice feature, I don't think we'll be implementing this
> as part of this bug. We can file a separate bug to add this, along with
> other features (such as the ability to choose a time range).

Agreed

> > 4. There's no indication that it cleared anything, the popup just goes away.
> > It should show a small "tooltip" saying "Data deleted."
> 
> Good idea - I can add this.

tooltip == toast, but yes, that would be nice.

Comment 16

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

Comment on attachment 633293 [details] [diff] [review]
Part 2: Enable clearing individual privacy items, v2.2

Looks OK, but we should get Ian to OK changing from two settings buttons -> to one setting button.

Post some screenshots?

Comment 17

[Brian Nicholson (:bnicholson)]

Attachment: (screenshot) new prefs menu

Comment 18

[Brian Nicholson (:bnicholson)]

Attachment: (screenshot) clear private data dialog

Comment 19

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

Comment on attachment 634169 [details] [diff] [review]
Part 1: Implement MultiChoicePreference, v2.1

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

::: mobile/android/base/MultiChoicePreference.java
@@ +84,5 @@
> +     * set.
> +     *
> +     * @param defaultValues The entry values
> +     */
> +    public void setDefaultValues(CharSequence[] defaultValues) {

these look to be initial values, not default values

@@ +102,5 @@
> +     * 
> +     * @return The array of entries.
> +     */
> +    public CharSequence[] getEntries() {
> +        return mEntries;

use.clone() for the getters

Comment 20

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

Comment on attachment 634177 [details] [diff] [review]
Add instance state support to MultiChoicePreference

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

::: mobile/android/base/GeckoPreferences.java
@@ +106,5 @@
>                  initGroups((PreferenceGroup)pref);
>              else {
>                  pref.setOnPreferenceChangeListener(this);
> +                String key = pref.getKey();
> +                if (key != null && !key.startsWith(ANDROID_ONLY_PREFIX))

I don't understand this change

Comment 21

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

Comment on attachment 634177 [details] [diff] [review]
Add instance state support to MultiChoicePreference

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

::: mobile/android/base/GeckoPreferences.java
@@ +106,5 @@
>                  initGroups((PreferenceGroup)pref);
>              else {
>                  pref.setOnPreferenceChangeListener(this);
> +                String key = pref.getKey();
> +                if (key != null && !key.startsWith(ANDROID_ONLY_PREFIX))

just read your bug comment. This needs a code comment

Comment 22

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Add instance state support to MultiChoicePreference, v1.1

added comment

Comment 23

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

Comment on attachment 634243 [details] [diff] [review]
Part 4: Add instance state support to MultiChoicePreference, v1.1

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

::: mobile/android/base/GeckoPreferences.java
@@ +107,5 @@
>              else {
>                  pref.setOnPreferenceChangeListener(this);
> +                String key = pref.getKey();
> +                // There are some preferences we don't want to sync with Gecko;
> +                // these pref keys are prefixed with "android.only.".

what I'm getting at is why we don't want to sync these. As I understand it, the reason is that they aren't actually prefs. Is that right?

Comment 24

[Brian Nicholson (:bnicholson)]

(In reply to Brad Lassey [:blassey] from comment #23)
> Comment on attachment 634243 [details] [diff] [review]
> Part 4: Add instance state support to MultiChoicePreference, v1.1
> 
> Review of attachment 634243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoPreferences.java
> @@ +107,5 @@
> >              else {
> >                  pref.setOnPreferenceChangeListener(this);
> > +                String key = pref.getKey();
> > +                // There are some preferences we don't want to sync with Gecko;
> > +                // these pref keys are prefixed with "android.only.".
> 
> what I'm getting at is why we don't want to sync these. As I understand it,
> the reason is that they aren't actually prefs. Is that right?

They aren't actually Gecko prefs, correct. How about this:

'Some preferences are used only in the Java UI and do not exist in Gecko, so we do not want to sync them. These pref keys are prefixed with "android.only.".'

Comment 25

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

(In reply to Brian Nicholson (:bnicholson) from comment #24)
> (In reply to Brad Lassey [:blassey] from comment #23)
> > what I'm getting at is why we don't want to sync these. As I understand it,
> > the reason is that they aren't actually prefs. Is that right?
> 
> They aren't actually Gecko prefs, correct. How about this:
> 
> 'Some preferences are used only in the Java UI and do not exist in Gecko, so
> we do not want to sync them. These pref keys are prefixed with
> "android.only.".'

What are they? Can you give examples?

Comment 26

[Brian Nicholson (:bnicholson)]

(In reply to Brad Lassey [:blassey] from comment #25)
> (In reply to Brian Nicholson (:bnicholson) from comment #24)
> > (In reply to Brad Lassey [:blassey] from comment #23)
> > > what I'm getting at is why we don't want to sync these. As I understand it,
> > > the reason is that they aren't actually prefs. Is that right?
> > 
> > They aren't actually Gecko prefs, correct. How about this:
> > 
> > 'Some preferences are used only in the Java UI and do not exist in Gecko, so
> > we do not want to sync them. These pref keys are prefixed with
> > "android.only.".'
> 
> What are they? Can you give examples?

The "Clear private data" dialog pref requires a key for its state to be saved when the orientation changes. It uses the "android.only.privacy.clear" key - which doesn't exist in Gecko - to satisfy this requirement.

Comment 27

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

(In reply to Brian Nicholson (:bnicholson) from comment #26)
> The "Clear private data" dialog pref requires a key for its state to be
> saved when the orientation changes. It uses the "android.only.privacy.clear"
> key - which doesn't exist in Gecko - to satisfy this requirement.

Ok, I'd rather it be android.not_a_preference or something similar, because that's why we wouldn't want to sync it with Gecko.

Comment 28

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Add instance state support to MultiChoicePreference, v1.2

Comment 29

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f3c2734016bd
http://hg.mozilla.org/integration/mozilla-inbound/rev/03cd608177cc
http://hg.mozilla.org/integration/mozilla-inbound/rev/dfced0e2fdaf
http://hg.mozilla.org/integration/mozilla-inbound/rev/81cf04b16eb1

Comment 30

[Aaron Train [:aaronmt]]

Feature needs a QA owner

Comment 31

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/mozilla-central/rev/f3c2734016bd
http://hg.mozilla.org/mozilla-central/rev/03cd608177cc
http://hg.mozilla.org/mozilla-central/rev/dfced0e2fdaf
http://hg.mozilla.org/mozilla-central/rev/81cf04b16eb1

Comment 32

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> http://hg.mozilla.org/mozilla-central/rev/f3c2734016bd
> http://hg.mozilla.org/mozilla-central/rev/03cd608177cc
> http://hg.mozilla.org/mozilla-central/rev/dfced0e2fdaf
> http://hg.mozilla.org/mozilla-central/rev/81cf04b16eb1

Brian, if you point me to another build with this fix, I'm happy to test it for you again.

Comment 33

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> http://hg.mozilla.org/mozilla-central/rev/f3c2734016bd
> http://hg.mozilla.org/mozilla-central/rev/03cd608177cc
> http://hg.mozilla.org/mozilla-central/rev/dfced0e2fdaf
> http://hg.mozilla.org/mozilla-central/rev/81cf04b16eb1

BTW, if "Nightly" has the changes, then it's still missing the "..." in the "Clear private data"

Comment 34

[Brian Nicholson (:bnicholson)]

Yes, Nightly contains this feature. We didn't add the "..." since that isn't a convention on Android. Android shows an arrow icon next to dialog menu items; see https://bugzilla.mozilla.org/attachment.cgi?id=634201 for an example.

Comment 35

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #34)
> Yes, Nightly contains this feature. We didn't add the "..." since that isn't
> a convention on Android. Android shows an arrow icon next to dialog menu
> items; see https://bugzilla.mozilla.org/attachment.cgi?id=634201 for an
> example.

Although it looks like some phones may not show this icon. It's worth looking into in a separate bug, but if we change this, we should make this change to all of our settings items with dialogs - not just this one.

Comment 36

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #35)
> (In reply to Brian Nicholson (:bnicholson) from comment #34)
> > Yes, Nightly contains this feature. We didn't add the "..." since that isn't
> > a convention on Android. Android shows an arrow icon next to dialog menu
> > items; see https://bugzilla.mozilla.org/attachment.cgi?id=634201 for an
> > example.
> 
> Although it looks like some phones may not show this icon. It's worth
> looking into in a separate bug, but if we change this, we should make this
> change to all of our settings items with dialogs - not just this one.

Yes, the issue for me is it's not showing an arrow on my phone (which is stock Ice Cream Sandwich) Nexus S.

Comment 37

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #35)
> (In reply to Brian Nicholson (:bnicholson) from comment #34)
> > Yes, Nightly contains this feature. We didn't add the "..." since that isn't
> > a convention on Android. Android shows an arrow icon next to dialog menu
> > items; see https://bugzilla.mozilla.org/attachment.cgi?id=634201 for an
> > example.
> 
> Although it looks like some phones may not show this icon. It's worth
> looking into in a separate bug, but if we change this, we should make this
> change to all of our settings items with dialogs - not just this one.

Are you sure the arrows are not an HTC thing?

Comment 38

[Brian Nicholson (:bnicholson)]

(In reply to donrhummy from comment #37)
> Are you sure the arrows are not an HTC thing?

Those screenshots are from a Droid RAZR.

Comment 39

[donrhummy]

(In reply to Brian Nicholson (:bnicholson) from comment #38)
> (In reply to donrhummy from comment #37)
> > Are you sure the arrows are not an HTC thing?
> 
> Those screenshots are from a Droid RAZR.

Is there a skin on Droid Razrs? Because the arrows might be from a skin. I don't have the arrows on either a Nexus S or a Galaxy Tab 10.1.

Comment 40

[Brian Nicholson (:bnicholson)]

Attachment: patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: User cannot choose which items to clear
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk, isolated feature
String or UUID changes made by this patch: string added for each item in menu (see https://bugzilla.mozilla.org/attachment.cgi?id=634203&action=edit), and also for toaster notifications

Comment 41

[Gian-Carlo Pascutto [:gcp]]

I get the arrow keys on a Galaxy S2 with TouchWiz 2.3.6, but not on the Tab 10.1 with TouchWiz 3.2 and not on the Nexus S with OASP 4.0.x.

Adding the arrow may hence be an enhancement made by the providers.

Looking at the Design Guideline, it is missing the arrow too, but doesn't use the "..." convention:
http://developer.android.com/design/patterns/settings.html

Even more, the "Pure Android" style guide says:
http://developer.android.com/design/patterns/pure-android.html

"Don't use right-pointing carets on line items

A common pattern on other platforms is the display of right-pointing carets on line items that allow the user to drill deeper into additional content.

Android does not use such indicators on drill-down line items. Avoid them to stay consistent with the platform and in order to not have the user guess as to what the meaning of those carets may be."

So basically, the providers added the arrows to make the behavior clearer, but Google disliked it and probably forbid it for subsequent versions, which is why they're gone. 

Our behavior is proper. (...but whether this Android design guideline is such a good idea, that's another matter...)

Comment 42

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/7a514fb967d2

Comment 43

[Aaron Train [:aaronmt]]

Verified Fixed on Mozilla-Central & Aurora (07/10)

Comment 44

[Adrian Tamas (:AdrianT)]

Tested on Nightly 17.0a1 2012-07-17 and Firefox Mobile 15 beta 1 build 2 using an HTC Desire running Android 2.2 and a Samsung Galaxy S2 running Android 2.3.4.

Tested each option in the list and here are the findings:

Browsing&Download history - download history is not cleared - Bug 775042
Form&search history - pass - tested bugzilla search history
Cookies - pass - items removed from shopping cart at g2play.net when cookies are cleared and no user signed in
Cache - about:cache displays 0 for all values after clearing private data
Active logins - fail - Bug 775039 and Bug 775029 
Offline website data - pass using http://vingtetun.org/todo/
Site preferences - pass using geolocation, popuptest and offline storage websites.

Comment 45

[Aaron Train [:aaronmt]]

Adrian, please add test-cases around this feature.

Comment 46

[Adrian Tamas (:AdrianT)]

Tests created for the split options in "Clear private data" under the Firefox Mobile: Privacy & Content Managing test suite in MozTrap.