Closed Bug 754335 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

14 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox15 verified, firefox16 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: donrhummy, Assigned: bnicholson)

References

Details

(Keywords: uiwanted)

Attachments

(7 files, 7 obsolete files)

21.35 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
9.46 KB, patch
blassey
: review+
Details | Diff | Splinter Review
5.45 KB, patch
blassey
: review+
Details | Diff | Splinter Review
48.21 KB, image/png
ibarlow
: review+
ibarlow
: feedback+
Details
106.25 KB, image/png
ibarlow
: feedback+
Details
4.81 KB, patch
blassey
: review+
Details | Diff | Splinter Review
36.91 KB, patch
Details | Diff | Splinter Review
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
Yeah, I'd like this too. Chrome Beta on Android does it nice.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Keywords: uiwanted
OS: Linux → Android
Hardware: x86_64 → ARM
Summary: No way to clear only recent history, or just cookies, etc → Request for ability to select which privacy and security items to clear over entire wipe
Assignee: nobody → bnicholson
Attachment #632948 - Flags: review?(mark.finkle)
Attachment #632951 - Flags: review?(mark.finkle)
(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?
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.
Attachment #632948 - Attachment is obsolete: true
Attachment #632948 - Flags: review?(mark.finkle)
Attachment #633286 - Flags: review?(blassey.bugs)
Renamed some keys/strings, and rebased with changes from the first patch.
Attachment #632951 - Attachment is obsolete: true
Attachment #632951 - Flags: review?(mark.finkle)
Attachment #633289 - Flags: review?(blassey.bugs)
(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
Updated to remove the old clear_history_* and clear_private_* strings.
Attachment #633289 - Attachment is obsolete: true
Attachment #633289 - Flags: review?(blassey.bugs)
Attachment #633292 - Flags: review?(blassey.bugs)
We still need the pref_clear_private_data string, so adding that back in.
Attachment #633292 - Attachment is obsolete: true
Attachment #633292 - Flags: review?(blassey.bugs)
Attachment #633293 - Flags: review?(blassey.bugs)
(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.
(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.
This change clones the arrays in setEntries()/setEntryKeys()/setDefaultValues().
Attachment #633286 - Attachment is obsolete: true
Attachment #633286 - Flags: review?(blassey.bugs)
Attachment #634169 - Flags: review?(blassey.bugs)
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
Attachment #634177 - Flags: review?(blassey.bugs)
(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 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?
Attachment #633293 - Flags: review?(blassey.bugs) → review+
Attachment #634201 - Flags: feedback?(ibarlow)
Attachment #634203 - Flags: feedback?(ibarlow)
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
Attachment #634169 - Flags: review?(blassey.bugs) → review+
Attachment #634170 - Flags: review?(blassey.bugs) → review+
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 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
Attachment #634177 - Flags: review?(blassey.bugs) → review-
added comment
Attachment #634243 - Flags: review?(blassey.bugs)
Attachment #634177 - Attachment is obsolete: true
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?
(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.".'
(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?
(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.
Attachment #634201 - Flags: review+
Attachment #634203 - Flags: feedback?(ibarlow) → feedback+
Attachment #634201 - Flags: feedback?(ibarlow) → feedback+
(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.
Attachment #634243 - Attachment is obsolete: true
Attachment #634243 - Flags: review?(blassey.bugs)
Attachment #635392 - Flags: review?(blassey.bugs)
Attachment #635392 - Flags: review?(blassey.bugs) → review+
Feature needs a QA owner
Flags: in-moztrap?(fennec)
(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.
(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"
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.
(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.
(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.
(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?
(In reply to donrhummy from comment #37)
> Are you sure the arrows are not an HTC thing?

Those screenshots are from a Droid RAZR.
(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.
Attached patch patch for auroraSplinter Review
[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
Attachment #638052 - Flags: approval-mozilla-aurora?
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...)
Blocks: 769896
Attachment #638052 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified Fixed on Mozilla-Central & Aurora (07/10)
Status: RESOLVED → VERIFIED
Depends on: 775029
Depends on: 775039
Depends on: 775042
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.
Adrian, please add test-cases around this feature.
Flags: in-moztrap?(fennec) → in-moztrap?(adrian.tamas)
Tests created for the split options in "Clear private data" under the Firefox Mobile: Privacy & Content Managing test suite in MozTrap.
Flags: in-moztrap?(adrian.tamas) → in-moztrap+
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.