Lots of people seem to want to clear history. We can provide an easy pref to do it. The hard part here is determining exactly WHEN to clear, since we rarely are shut down cleanly and don't have a real "Quit" item for users. Perhaps an easy first pass is just to clear any time the app is "paused" (i.e. put in the background or the screen is blanked while Fennec is running). From there we could evolve into a more complex behavior if we need/want to (i.e. re-enabling the "Quit" menu or adding timeouts so that we only clear if we're in the background for an extended period. Note, we already do expire SOME old history when we're put in the background. We might be able to reuse that to make this a fairly simple change.

Feedback seems to suggest people want "clear private data", not just browsing history. I have seen a few specific requests for clearing data on exit, "like desktop firefox does".

This is a WIP that lets users choose what data is cleared when we're put in the "background". Its pretty easy to reuse the current private data dialog, so I figured I'd give it a shot. I'd like to refine the settings UI a bit so that this looks more like a checkbox that changes state if you've selected any of the options to clear. Note, for now "background" includes even just opening the settings page. It can make for an odd experience if there's a failure (i.e. you'll open settings and suddenly see "Could not sanitize" in a toast. We might be able to do a little better there and clear this in one of the onTrimMemory events instead. I'll throw up a build in a minute. Had to clobber.

Comment on attachment 8419187 [details] [diff] [review] WIP Patch Review of attachment 8419187 [details] [diff] [review]: ----------------------------------------------------------------- Random thoughts from me: This is a tough one! On the one hand, some of the things we want to clear live in Gecko, so we need to run this when Gecko is alive. On the other hand, user expectations are probably that switching to receive a text message doesn't wipe their browser; they have some wooly understanding of what they mean by "done"/"quit", and onPause isn't it. If we wait a while, Gecko might not be around, but if we don't wait I suspect we'll get a cry of "but that wasn't what I meant!". I might be inclined to abuse onFinish for this, or to try one of a few other approaches. Thoughts on these? * Explicitly exposing the concept of inactivity: "Clear private data when Firefox is in the background for [1, 2, 5, 10] minutes" * Adding a "Quit" menu item if you opt in to this feature. That Quit item would just: 1. Clear data 2. Finish the activity. That is -- it's modeling the 'quit' part of this user's concept without exposing a kill switch to all users. ::: mobile/android/app/mobile.js @@ +855,5 @@ > > // How frequently to check if we should sync home provider data. > pref("home.sync.checkIntervalSecs", 3600); > + > +// A list of things to clear on exit Nit: full sentence, provide examples. ::: mobile/android/chrome/content/browser.js @@ +7096,5 @@ > } > isForeground = false; > + > + // TODO: Move this to when the pref is set... > + var prefs = Services.prefs.getCharPref("history.clear_on_exit"); Nit: let.

Another option would be an "always start in private browsing mode" pref. The history associated with a tab goes away whenever the tab is closed, which is an explicit user action that's already available in our UI. No arbitrary timer, no extra menu items. One caveat to this approach: while the tab's history is gone after that single tab is closed, other private data (cookies, cache, etc.) hangs around until either 1) *all* private tabs are closed, or 2) Gecko is killed.

(In reply to Brian Nicholson (:bnicholson) from comment #4) > Another option would be an "always start in private browsing mode" pref. The > history associated with a tab goes away whenever the tab is closed, which is > an explicit user action that's already available in our UI. No arbitrary > timer, no extra menu items. > > One caveat to this approach: while the tab's history is gone after that > single tab is closed, other private data (cookies, cache, etc.) hangs around > until either 1) *all* private tabs are closed, or 2) Gecko is killed. Yes. The "always start in private browsing mode" idea is slightly different than "clear data on exit". (In reply to Richard Newman [:rnewman] from comment #3) > * Adding a "Quit" menu item if you opt in to this feature. That Quit item > would just: > 1. Clear data > 2. Finish the activity. > > That is -- it's modeling the 'quit' part of this user's concept without > exposing a kill switch to all users. As much as we push back on the Quit menu, it might be the best and simplest model for this use case. I would be in favor of showing the Quit menu if this option is enabled. We could even try to make the "quit" action be less severe and not take Gecko down, just the Activity. But, we'd still clear the data. It might be the easiest way for these types of users to wrap their heads around it too. Ian?

I've been holding off on this until UX had a chance to give some feedback, but never put up a build. Here's one: http://people.mozilla.org/~wjohnston/clearOnExit.apk As I mentioned earlier, this still needs some work, but I didn't want to put too much in if UX wanted to go a different route here.

(In reply to Mark Finkle (:mfinkle) from comment #5) > > * Adding a "Quit" menu item if you opt in to this feature. That Quit item > > would just: > > 1. Clear data > > 2. Finish the activity. > > > > That is -- it's modeling the 'quit' part of this user's concept without > > exposing a kill switch to all users. > > As much as we push back on the Quit menu, it might be the best and simplest > model for this use case. I would be in favor of showing the Quit menu if > this option is enabled. We could even try to make the "quit" action be less > severe and not take Gecko down, just the Activity. But, we'd still clear the > data. It might be the easiest way for these types of users to wrap their > heads around it too. > > Ian? Yeah, I agree. Apps being backgrounded is such a brutally opaque concept, users can't wrap their heads around that and tying a feature to it seems like a recipe for confusion. We could sort of hack around it by doing something time based like "Clear history every x hours" or something, but if we want to put real control in the user's hands I think adding a Quit button when this feature is enabled is the way to go.

Note for future implementer: need to loop in capella here re QuitNow. https://addons.mozilla.org/en-US/android/addon/quitnow/

Just a thought: We do not need to really terminate Gecko, like we do with the current Quit on Gingerbread (and QuitNow does). We could just finish() the Activity or even less. Depends on what we want to do. The point is: "Clear data on exit" seems to be most easily grasped using a Quit menu to trigger it. Brian has had to deal with various "quit" and activity death in the past. He might have some thought here too. There might be pros and cons to various kinds of shutdown approaches.

Also fyi, there are two other add-ons already available that accomplish basically these goals. ObliviousSession This requires the Quit button (uses native FF menu option if pre-ICS, or piggy-backs QuitNow) and enhances it with sanitization-on-exit. CleanQuit This is an all-in-one version that provides the a custom Quit button and sanitization-on-exit.

(In reply to Mark Finkle (:mfinkle) from comment #9) > We could just finish() the Activity or even less. Calling finish() would basically be the same behavior as Don't Keep Activities, which can be pretty buggy (messages sent from Gecko -> Java are lost, we end up with weird Context-related bugs, etc). So if we aren't going to kill Gecko, I would just send Fennec to the background. That's not actually quitting, of course, but I don't know whether that matters. > Just a thought: We do not need to really terminate Gecko, like we do with > the current Quit on Gingerbread (and QuitNow does). If we don't need to terminate Gecko, do we need a Quit button at all? I wonder if we could get by with a "Clear data" button directly in the menu that clears whatever "Clear data on exit" would clear, then people can just close Fennec as they normally would. Or is it absolutely essential that this all happen in a single click? Another crazy idea: what about clear data on *startup*? We can't hook into anything when we're killed, but we can determine how we were killed when we reopen. That would allow swiping from the recent apps list to be used as the quit method, which is still Android-ish. And we could still call this "Clear data on exit" since the difference won't be noticeable to the user.

(In reply to Brian Nicholson (:bnicholson) from comment #11) > Calling finish() would basically be the same behavior as Don't Keep > Activities, which can be pretty buggy (messages sent from Gecko -> Java are > lost, we end up with weird Context-related bugs, etc). That sounds like something we should be working on fixing (e.g., pushing remaining queued messages back into Gecko in onFinish?). Simply not finishing GeckoApp ourselves is a mitigation, but if this is really a big enough deal that we'd let it affect this decision, we should be setting up a test case and trying to address it. Do you have a bug open for this? > to kill Gecko, I would just send Fennec to the background. That's not > actually quitting, of course, but I don't know whether that matters. Seems like a reasonable option for clearing data. I'm not sure how much we care about those users who are wanting to quit Firefox to get resources back -- this will be actively misleading those folks, but maybe they should be trusting Android to allocate resources? > If we don't need to terminate Gecko, do we need a Quit button at all? I > wonder if we could get by with a "Clear data" button directly in the menu Need UI telemetry data, I guess. My speculation (with no data to back it up) is that the people who are asking for clear data on quit want to be able to tell Firefox "OK, I'm done, and clean up as you leave". Quit is an understandable label for the boundary: the explicit end of my browsing session. This raises the point, though: will sufficiently Android-immersed users have some fuzzy expectation that Firefox will clear data when they're "done browsing", even though those users will hit Home (not Quit) when they're done? User expectations don't have to be rational or achievable, alas. > Another crazy idea: what about clear data on *startup*? I think that poses some challenges -- flicker in the Java UI is one, and doing extra (expensive) work as soon as Gecko starts is another. It also doesn't address concerns of people who want their data gone immediately (again, easier, but actively misleading). Can you explain how you conceive of this working from a user's perspective? The two scenarios I can think of -- hitting Home or choosing Quit -- could both be served by deletion during onFinish, so I think I'm missing what the value is here.

(In reply to Richard Newman [:rnewman] from comment #12) > (In reply to Brian Nicholson (:bnicholson) from comment #11) > > > Calling finish() would basically be the same behavior as Don't Keep > > Activities, which can be pretty buggy (messages sent from Gecko -> Java are > > lost, we end up with weird Context-related bugs, etc). > > That sounds like something we should be working on fixing (e.g., pushing > remaining queued messages back into Gecko in onFinish?). > > Simply not finishing GeckoApp ourselves is a mitigation, but if this is > really a big enough deal that we'd let it affect this decision, we should be > setting up a test case and trying to address it. > > Do you have a bug open for this? Just from some brief testing, I hit bug 872411 (the QuitNow menu didn't come back), bug 1020748 (just filed -- hopefully fixable with a message queue), and bug 964861 (found the missing piece to the STR!). There are probably many more, but I've given up looking for/fixing DKA bugs because new ones keep cropping up. Until we're able to run Robocop tests with DKA enabled, it's going to be difficult to stay reliable. > Seems like a reasonable option for clearing data. I'm not sure how much we > care about those users who are wanting to quit Firefox to get resources back > -- this will be actively misleading those folks, but maybe they should be > trusting Android to allocate resources? Hopefully we'd end up using more appropriate terminology than "quit" if we went with that approach. > This raises the point, though: will sufficiently Android-immersed users have > some fuzzy expectation that Firefox will clear data when they're "done > browsing", even though those users will hit Home (not Quit) when they're > done? I don't think we'd want to clear anything just from hitting home. Imagine receiving a phone call in the middle of your browser session, only to return and have your session wiped! That would pretty frustrating. The ideal trigger (IMO) would be swiping Fennec from the recent apps list. That's the Android way of closing any app, and it's the way we've been promoting since we removed the Quit button. Unfortunately, there's no hook we can use to do a clean shutdown when we're closed from recent apps, which is why we flush our prefs/session/disk cache on every onPause. > > Another crazy idea: what about clear data on *startup*? > > I think that poses some challenges -- flicker in the Java UI is one, and > doing extra (expensive) work as soon as Gecko starts is another. I'm curious about perf too -- since the profile will be pretty empty every time, the delay might not be long. > Can you explain how you conceive of this working from a user's perspective? > The two scenarios I can think of -- hitting Home or choosing Quit -- could > both be served by deletion during onFinish, so I think I'm missing what the > value is here. This would be useful for the scenario I mentioned above, which is closing from the recent apps list. This is the normal way to close Android apps, but we can't do any special handling before we're killed.

(In reply to Richard Newman [:rnewman] from comment #12) > My speculation (with no data to back it up) is that the people who are > asking for clear data on quit want to be able to tell Firefox "OK, I'm done, > and clean up as you leave". Quit is an understandable label for the > boundary: the explicit end of my browsing session. We just want basic privacy. How exactly is an implementation detail. I have made a few suggestions in bug 996028. At least, it would already help if we could start Firefox in private mode, as suggested in bug 909854. > This raises the point, though: will sufficiently Android-immersed users have > some fuzzy expectation that Firefox will clear data when they're "done > browsing", even though those users will hit Home (not Quit) when they're > done? I think people see this (and removing focus from the app in general) more like "minimizing". I think some people would be fine with Brian's recent apps suggestion but some people like using that for freeing memory. Also, people could not be sure when exactly the browsing is "done", and not too easily realize it is the quit menu or recent apps (I think "Clean and quit" is a better label). Considering this and the suggestions in bug 996028, maybe you could add an option like this: Automatically clear [x] When pressing the quit menu browsing data: [x] When removing from recent apps [x] After [N] minutes on background/inactive [x] Which is [N] days old

This adds back the quit button when Clear on exit is enabled. It only clears if you explicitly select Exit. I'd rather just start there than try to do everything at once in this bug, and it feels like a nice way for them to say "I'm done with this data". It uses a special pref type that looks like a checkbox but shows a multi-choice dialog when opened so that you can pick what to clear. A quick survey of competing browsers showed all of them offered a few different data types to clear (for instance dolphin offers "Cache", "History", and "Cookies"). I included everything we can clear here. UX might want to pare that down. This stores the pref in both Java and Gecko. We need it in Java in order to not block when trying to show the menu. Storing it in Gecko was just simple. We could theoretically pass that down when you selected "Exit". I don't have a strong preference either way. I also abstracted the prefs management a bit in GeckoPreferences. We've got a lot of data for a lot of different prefs scattered through there. I'd like to file a follow up to move more of them to use Helpers. This also changes our Sanitizer to return promises from each clear() call. We need to ensure that we've cleared everything before we actually quit, especially since the main thing you likely want to clear, history, is actually an async call to java. Promises make it easy to run all of those clear calls in parallel and only notify Java when they've all finished. Build at: http://people.mozilla.org/~wjohnston/clearOnExit.apk

Comment on attachment 8435333 [details] [diff] [review] Patch v2 >diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js >+// A list of things to clear on exit >+pref("history.clear_on_exit", ""); >\ No newline at end of file Add one >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd >+<!ENTITY pref_clear_private_data_on_exit "Clear private data on exit"> >+<!ENTITY pref_clear_private_data_on_exit_summary "&brandShortName; will clear data when you select Exit in the main menu."> >+<!ENTITY pref_clear_on_exit_title "Data to clear"> Comments: * I am leaning to use "shutdown" instead of exit> "Clear private data on shutdown" * The menu is "Quit" not "Exit", so using a "shutdown" related string might be better: "&brandShortName; will clear data when you shutdown using the menu." ? >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ var prefs = Services.prefs.getCharPref("history.clear_on_exit") || {}; >+ BrowserApp.sanitize(prefs, function() { >+ window.QueryInterface(Ci.nsIDOMChromeWindow).minimize(); >+ window.close(); >+ }); I worry about storing this Gecko and Java. What if an add-on tweaks the Gecko version of the pref. We already know lots of add-ons like to tweak the "clear on exit" functionality on desktop. What's the proposal for sending the data over from Java to Gecko as needed? >+ case "history.clear_on_exit": >+ // Android sends us a serialized Set<String> here. Split it and filter out things >+ // that aren't valid sanitize targets. >+ let prefValues = {}; >+ json.value.split(/[\[,\]]/).forEach((pref) => { >+ if (/^private\.data\./.test(pref)) { >+ pref = pref.trim().replace(/^private\.data\./, ""); >+ prefValues[pref] = true; >+ } >+ }); >+ Services.prefs.setCharPref(json.name, JSON.stringify(prefValues)); See above. Can we look at not doing this?

This looks pretty good Wes! I only tested it with my browsing history but worked as expected. I think we need to fiddle with our Settings a tiny bit though. Now that we have two "clear private data" options we might need to tweak the wording of both so that it's clearer that one is about doing it right now, one is about doing it later. Below is my first stab at that: -------------------------------------- # Clear private data now -------------------------------------- # Clear private data after quitting Firefox will automatically clear your data when you select "Quit" from the main menu. --------------------------------------

or maybe instead of "...when you select..." we say "...whenever you select..."

Splitting this up to help review a bit. I also made this only hold the pref on the Java side of things. This doesn't do that actual clearing though. That's in patch 2.

I know this isn't really your code liuche, but I figured it was nice to keep this all with one person. This does the actual clearing. It passes the rows to clear down in the Browser:Shutdown message.

Comment on attachment 8436105 [details] [diff] [review] Patch 1/2 Review of attachment 8436105 [details] [diff] [review]: ----------------------------------------------------------------- Nice, I like the UI, but I think we should get ibarlow or antlam to take a look at it, and maybe also finalize strings. I'm not sure "Data to clear" is consistent with our other dialog titles, but I can't think of another non-verbose string. I'm happy to r+ after addressing the comment about SharedPreferences.getStringSet for lower versions - everything else is just nits. ::: mobile/android/base/BrowserApp.java @@ +2278,5 @@ > MenuItem desktopMode = aMenu.findItem(R.id.desktop_mode); > MenuItem enterGuestMode = aMenu.findItem(R.id.new_guest_session); > MenuItem exitGuestMode = aMenu.findItem(R.id.exit_guest_session); > > + // Only show the "Quit" menu item on pre-ICS, television devices, of if the user has explicitly enabled the clear on shutdown pref. Typo: "or if the user..." @@ +2283,3 @@ > // In ICS+, it's easy to kill an app through the task switcher. > + SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + Set<String> clear = prefs.getStringSet(GeckoPreferences.ClearOnShutdown.PREF, new HashSet<String>()); final? SharedPreferences.getStringSet() is only present on v11 - I think we still want to track this in versions below v11 :/ (Otherwise, this should probably be wrapped in a check if you are going to use it.) ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +146,5 @@ > <!ENTITY pref_char_encoding_on "Show menu"> > <!ENTITY pref_char_encoding_off "Don\'t show menu"> > <!ENTITY pref_clear_private_data "Clear private data"> > +<!ENTITY pref_clear_private_data_on_exit "Clear private data after quitting"> > +<!ENTITY pref_clear_private_data_on_exit_summary "&brandShortName; will automatically clear your data whenever you select &quot;Quit&quot; from the main menu."> Nit, and maybe check with Ian: period is inconsistent with our Settings strings I think. @@ +147,5 @@ > <!ENTITY pref_char_encoding_off "Don\'t show menu"> > <!ENTITY pref_clear_private_data "Clear private data"> > +<!ENTITY pref_clear_private_data_on_exit "Clear private data after quitting"> > +<!ENTITY pref_clear_private_data_on_exit_summary "&brandShortName; will automatically clear your data whenever you select &quot;Quit&quot; from the main menu."> > +<!ENTITY pref_clear_on_exit_title "Data to clear"> Nit: be more consistent with the naming of the entities (and strings) - can we make them all either pref_clear_on_exit* or pref_clear_private_data_on_exit*? ::: mobile/android/base/preferences/GeckoPreferences.java @@ +927,5 @@ > + public void onChange(Preference pref, Object newValue); > + } > + > + @SuppressWarnings("serial") > + private HashMap<String, PrefHandler> handlers = new HashMap<String, PrefHandler>() {{ Nit: var declaration should be Set. @@ +929,5 @@ > + > + @SuppressWarnings("serial") > + private HashMap<String, PrefHandler> handlers = new HashMap<String, PrefHandler>() {{ > + put(ClearOnShutdown.PREF, new ClearOnShutdown()); > + }}; Cool, I've never seen double brace initialization. ::: mobile/android/base/preferences/ListCheckboxPreference.java @@ +16,5 @@ > +import org.mozilla.gecko.R; > + > +/* This preference shows a checkbox on its left hand side, but will show a menu when clicked. > + Its used for preferences like "Clear on Exit" that have a boolean on-off state, but that represent > + multiple boolean options inside. */ Nit: Use /** and have leading * for each line - it looks cleaner and more consistent, especially with class comments. See something like db/SuggestedSites.java. @@ +23,5 @@ > + private boolean checked; > + > + public ListCheckboxPreference(Context context, AttributeSet attrs) { > + super(context, attrs); > + if (getWidgetLayoutResource() <= 0) { What's the conditional for? ::: mobile/android/base/resources/layout/preference_checkbox.xml @@ +2,5 @@ > +<!-- Copyright (C) 2006 The Android Open Source Project > + Licensed under the Apache License, Version 2.0 (the "License"); > + you may not use this file except in compliance with the License. > + You may obtain a copy of the License at > + ws @@ +4,5 @@ > + you may not use this file except in compliance with the License. > + You may obtain a copy of the License at > + > + http://www.apache.org/licenses/LICENSE-2.0 > + ws @@ +15,5 @@ > + > +<!-- Layout used by CheckBoxPreference for the checkbox style. This is inflated > + inside android.R.layout.preference. --> > +<CheckBox xmlns:android="http://schemas.android.com/apk/res/android" > + android:id="@+id/checkbox" Nit: trailing ws

Ian, does this string work for you? For reference, the string for non-cleared-on-exit private data is just "Clear private data".

Comment on attachment 8436107 [details] [diff] [review] Patch 2/2 Review of attachment 8436107 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but make sure the tests still pass for the clear data/master password tests - I don't think they should, but sometimes the tests expect things. ::: mobile/android/base/GeckoApp.java @@ +467,5 @@ > public boolean onOptionsItemSelected(MenuItem item) { > if (item.getItemId() == R.id.quit) { > if (GeckoThread.checkAndSetLaunchState(GeckoThread.LaunchState.GeckoRunning, GeckoThread.LaunchState.GeckoExiting)) { > + SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + Set<String> clearSet = prefs.getStringSet(GeckoPreferences.ClearOnShutdown.PREF, new HashSet<String>()); Same comment about getStringSet being added in v11. @@ +470,5 @@ > + SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + Set<String> clearSet = prefs.getStringSet(GeckoPreferences.ClearOnShutdown.PREF, new HashSet<String>()); > + JSONObject clearObj = new JSONObject(); > + for (String clear : clearSet) { > + if (!"false".equals(clear)) { Up to you if you want to use this or TextUtils. I guess we know the value we're looking for. @@ +473,5 @@ > + for (String clear : clearSet) { > + if (!"false".equals(clear)) { > + try { > + clearObj.put(clear, true); > + } catch(JSONException ex) { } Log an error. @@ +610,5 @@ > mPrivateBrowsingSession = message.optString("session", null); > > } else if ("Sanitize:ClearHistory".equals(event)) { > handleClearHistory(); > + callback.sendSuccess(true); Nit: newline here ::: mobile/android/chrome/content/browser.js @@ +1336,5 @@ > } > }, > > + sanitize: function (aItems, callback) { > + if (!aItems) { Does this check if the object is empty? It looks like the default value you pass in is {}, which doesn't fail this check. Or maybe I'm not understanding what you're trying to do here... @@ +7020,5 @@ > if (doc && doc.mozFullScreen) { > doc.mozCancelFullScreen(); > } > isForeground = false; > + Nit: unrelated newline?

(In reply to Chenxia Liu [:liuche] from comment #22) > Created attachment 8437359 [details] > Screenshot: "Clear on quit" selection dialog > > Ian, does this string work for you? For reference, the string for > non-cleared-on-exit private data is just "Clear private data". Maybe "Select which data to clear" ?

(In reply to Ian Barlow (:ibarlow) from comment #17) > This looks pretty good Wes! I only tested it with my browsing history but > worked as expected. > > I think we need to fiddle with our Settings a tiny bit though. Now that we > have two "clear private data" options we might need to tweak the wording of > both so that it's clearer that one is about doing it right now, one is about > doing it later. > > Below is my first stab at that: > > -------------------------------------- > # Clear private data now > -------------------------------------- > # Clear private data after quitting > Firefox will automatically clear your data when you select "Quit" from the > main menu. > -------------------------------------- These dual entry points are troublesome. Together like this, they don't look as well as we'd probably like. I have been trying to find a way to combined them together, with "Now" and "On Quit" triggers. It's hard though because if you say "On Quit" the sticky settings for the data can be different than "Now".

I can see that. I wonder if splitting this apart into its own subsection would make this clearer... http://cl.ly/image/283W34262a0q

Thanks for catching the v11 bugs. I started looking into using our MultiChoicePreference instead, but noticed for some reason we made it differ from the Android's MultiSelectListPreference slightly (it treats every row in the dialog as a separate pref rather than as one Set<String> pref). I'm not sure why we did that. Most of the code is copy-paste so I assume it was intentional. Brian do you? I could use it, but it seemed strange to have something just a little different than the v11 widget in the tree (also I was frustrated that rather than just asking for a single pref, I was now going to have to get a list and iterate over a bunch of prefs in order to build a list of things to clear). This brings it back closer in line with the Android widget (and simplifies some array iteration). On older android versions it persists the pref as a JSONArray. I also 1.) renamed some array keys since entryValues is no longer necessarily a list of keys. 2.) Moved some special case code into specific prefs (for instance, we don't want to disable the "OK" button on this "Clear on exit" dialog when nothing is selected). 3.) Made sure listeners are notified before prefs change. 4.) Misc other cleanup.

Updated to use the new code.

Some small changes here to use PrefUtils, but carrying r+

(In reply to Ian Barlow (:ibarlow) from comment #26) > I can see that. I wonder if splitting this apart into its own subsection > would make this clearer... http://cl.ly/image/283W34262a0q I like this approach

(In reply to Wesley Johnston (:wesj) from comment #27) > Thanks for catching the v11 bugs. I started looking into using our > MultiChoicePreference instead, but noticed for some reason we made it differ > from the Android's MultiSelectListPreference slightly (it treats every row > in the dialog as a separate pref rather than as one Set<String> pref). I'm > not sure why we did that. Most of the code is copy-paste so I assume it was > intentional. Brian do you? This code was all written from scratch (no copy-paste). I didn't use persistStringSet because it wasn't exposed in the Preference API (and looks like it's still hidden [1]). But even it was exposed, it uses SharedPreferences.Editor.putStringSet [2], which is API 11 as Chenxia mentioned. If it's a big deal, we could probably write our own (or copy) Set serialization/deserialization for prefs. [1] http://androidxref.com/4.4.3_r1.1/xref/frameworks/base/core/java/android/preference/Preference.java#1445 [2] http://developer.android.com/reference/android/content/SharedPreferences.Editor.html#putStringSet%28java.lang.String,%20java.util.Set%3Cjava.lang.String%3E%29

Comment on attachment 8442660 [details] [diff] [review] Patch 1/3 Review of attachment 8442660 [details] [diff] [review]: ----------------------------------------------------------------- With this patch, won't we lose any old-style selections that were stored individually? I wonder if we should have an upgrade path. ::: mobile/android/base/preferences/AndroidImportPreference.java @@ +99,5 @@ > ); > } > + > + @Override > + public void onClick(DialogInterface dialog, int which, boolean val) { This implementation is exactly the same as PrivateDataPreference. Why not keep it in MultiChoicePreference? ::: mobile/android/base/preferences/MultiChoicePreference.java @@ +151,5 @@ > + if (mValues == null) { > + return values; > + } > + > + synchronized (mValues) { Why is this synchronized block here? getValues() is called only on the UI thread from what I can see (and if it's not, you'll want to synchronize all accesses to mValues -- not just this one). @@ +180,3 @@ > > + @Override > + public void onClick(DialogInterface dialog, int which, boolean val) { As noted elsewhere, you're using the exact same onClick implementation in AndroidImportPreference and PrivateDatapreference. Why not keep that implementation in this file so it's not copied in two places, and so you don't have to expose this onClick method? @@ +216,4 @@ > return false; > } > > + public Set<String> getPersistedStrings(Set<String> defaultVal) { s/public/private/ ::: mobile/android/base/preferences/PrivateDataPreference.java @@ +62,5 @@ > GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Sanitize:ClearData", json.toString())); > } > + > + @Override > + public void onClick(DialogInterface dialog, int which, boolean val) { This implementation is exactly the same as AndroidImportPreference. Why not keep it in MultiChoicePreference?

(In reply to Brian Nicholson (:bnicholson) from comment #32) > This implementation is exactly the same as AndroidImportPreference. Why not > keep it in MultiChoicePreference? The pref we're adding here needs to do the opposite. We could override the opposite way (i.e. the new pref could override onClick), but I was trying to bring this more in line with the default MultiSelect preference, so I opted to make this the non-default behavior. After I uploaded I realized there was no upgrade path either. I can try to add one...

This adds up an upgrade path via an extended MultiChoicePreference type that holds all the backwards compat behaviour.

Put these into their own category. Screenshots coming...

Screenshots look great!

Looks awesome Wes :)

Comment on attachment 8450642 [details] [diff] [review] Patch 1/3 - New pref type Review of attachment 8450642 [details] [diff] [review]: ----------------------------------------------------------------- I'm so, so sorry this took a long time. It looks good, though I'd like to see the MultiPrefMultiChoicePreference file before r+. I also don't see entryKeys from preferences*.xml and arrays.xml used anywhere anymore, but maybe that's in MultiPrefMultiChoicePreference? ::: mobile/android/base/moz.build @@ +339,5 @@ > 'preferences/LinkPreference.java', > 'preferences/LocaleListPreference.java', > 'preferences/ModifiableHintPreference.java', > 'preferences/MultiChoicePreference.java', > + 'preferences/MultiPrefMultiChoicePreference.java', This looks like a new file - I don't see it included in this patch though. ::: mobile/android/base/preferences/MultiChoicePreference.java @@ +18,3 @@ > import android.preference.DialogPreference; > +import android.os.Build; > +import android.text.TextUtils; It doesn't look like Build or TextUtils is used in this file. @@ +23,5 @@ > +import java.util.HashSet; > +import java.util.Set; > + > +import org.json.JSONArray; > +import org.json.JSONException; It doesn't look like these JSON imports are used in this file. @@ +125,1 @@ > * If you want, you could remove this trailing space too. @@ +138,5 @@ > public CharSequence[] getInitialValues() { > return mInitialValues.clone(); > } > > + void setValue(int i, boolean value) { Where is this called? Should it have a modifier? @@ +152,5 @@ > * > * @return The array of values. > */ > + public Set<String> getValues() { > + HashSet<String> values = new HashSet<String>(); Nit: values declaration should be Set<String>. @@ +223,4 @@ > return false; > } > > + protected boolean persist(SharedPreferences.Editor edit) { A quick Javadoc in the style of persistLong, persistBoolean, etc would be nice. ::: mobile/android/base/resources/values/attrs.xml @@ +88,1 @@ > <attr name="entryKeys" format="string"/> Do you still use entryKeys anywhere? ::: mobile/android/base/util/JSONUtils.java @@ +20,1 @@ > import java.util.UUID; Some of these imports are unnecessary now (URLException, URL, UUID), I wouldn't mind if you removed them in this patch. @@ +56,5 @@ > } > + > + // Handles conversions between a JSONArray and a Set<String> > + public static Set<String> parseStringSet(JSONArray json) { > + final HashSet<String> ret = new HashSet<String>(); Nit: Set<String> declaration instead of HashSet<String> ::: mobile/android/base/util/PrefUtils.java @@ +16,5 @@ > +import org.json.JSONException; > + > + > +public class PrefUtils { > + private static final String LOGTAG = "GekcoPrefUtils"; Nit: typo @@ +34,5 @@ > + // If this is Android version >= 11, try to use a Set<String>. > + try { > + return prefs.getStringSet(key, new HashSet<String>()); > + } catch(ClassCastException ex) { > + // A ClassCastException means if we've upgraded from a pre-v11 Android to a new one Nit: extraneous if? @@ +37,5 @@ > + } catch(ClassCastException ex) { > + // A ClassCastException means if we've upgraded from a pre-v11 Android to a new one > + final Set<String> val = getFromJSON(prefs, key); > + SharedPreferences.Editor edit = prefs.edit(); > + putStringSet(edit, key, val).commit(); The commit() is unnecessary, see comment below regarding commit()/apply(). @@ +63,5 @@ > + if (Build.VERSION.SDK_INT < 11) { > + final JSONArray json = new JSONArray(vals); > + edit.putString(key, json.toString()).apply(); > + } else { > + edit.putStringSet(key, vals).apply(); Doesn't calling apply() "commit" the transaction though? So the caller doesn't need to commit it. http://developer.android.com/reference/android/content/SharedPreferences.Editor.html#apply%28%29 : "As SharedPreferences instances are singletons within a process, it's safe to replace any instance of commit() with apply() if you were already ignoring the return value."

Comment on attachment 8450644 [details] [diff] [review] Patch 2/3 Review of attachment 8450644 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, nits. (And some remnant from a merge conflict or something.) ::: mobile/android/base/preferences/ClearOnShutdownPref.java @@ +15,5 @@ > +import android.content.SharedPreferences; > +import android.preference.Preference; > + > +public class ClearOnShutdownPref implements GeckoPreferences.PrefHandler { > + public static final String PREF = GeckoPreferences.NON_PREF_PREFIX + "history.clear_on_exit"; Nit: Do we care about the pref name and the class name matching? @@ +19,5 @@ > + public static final String PREF = GeckoPreferences.NON_PREF_PREFIX + "history.clear_on_exit"; > + > + @Override > + public void setupPref(Context context, Preference pref) { > + // The pref is initialized asynchronously. Read the pref explicitly here to make sure we have the data Nit: Wrap and terminate with a period. ::: mobile/android/base/preferences/ListCheckboxPreference.java @@ +9,5 @@ > +import android.util.AttributeSet; > +import android.view.View; > +import android.widget.Checkable; > + > +import java.util.Set; Nit: Set isn't used. @@ +13,5 @@ > +import java.util.Set; > + > +import org.mozilla.gecko.R; > + > +/** This preference shows a checkbox on its left hand side, but will show a menu when clicked. Nit: One more newline at the top of this comment, to match our other Javadoc style. Nit: "right side" ? @@ +18,5 @@ > + * Its used for preferences like "Clear on Exit" that have a boolean on-off state, but that represent > + * multiple boolean options inside. > + **/ > +class ListCheckboxPreference extends MultiChoicePreference implements Checkable { > + private static final String LOGTAG = "GeckoMultiChoicePreference"; Nit: Logtag doesn't match class ::: mobile/android/base/resources/values/arrays.xml @@ +100,5 @@ > <item>true</item> > <item>true</item> > <item>true</item> > </string-array> > + <string-array name="pref_clear_on_exit_defaults"> Nit: move this below all the pref_private_data sections. ::: mobile/android/base/resources/xml/preferences_privacy.xml @@ +39,5 @@ > + android:persistent="true" > + android:positiveButtonText="@string/button_clear_data" > + gecko:entries="@array/pref_private_data_entries" > + gecko:entryValues="@array/pref_private_data_values" > + gecko:entryKeys="@array/pref_private_data_keys" Still not sure entryKeys is used, but check on that. @@ +57,5 @@ > + android:positiveButtonText="@string/button_set"/> > + > + </PreferenceCategory> > + > +>>>>>>> Nit: remnant of conflict merge maybe

Sorry about that. This has the missing file.

Comment on attachment 8455393 [details] [diff] [review] Patch 1/3 - New pref type Review of attachment 8455393 [details] [diff] [review]: ----------------------------------------------------------------- Nit: I know this is kind of nit picky, but there are a lot of comments in MultiPrefMultiChoicePreference that aren't capitalized or terminated with a period. I think the logic is a little convoluted, but it's okay because this is trying to do a kind of complicated thing with prefs. r+ with nits, and make sure to include the comments from the previous review. ::: mobile/android/base/preferences/MultiChoicePreference.java @@ +182,3 @@ > > + @Override > + public void onClick(DialogInterface dialog, int which, boolean val) { This is an empty function - do you need to override it? If so, nit: {} on the same line. @@ +262,2 @@ > for (int i = 0; i < entryCount; i++) { > + boolean defaultVal = mInitialValues[i].equals("true"); Can you move this boolean defaultVal into the else branch below? We shouldn't need to check it if the pref has already been set. ::: mobile/android/base/preferences/MultiPrefMultiChoicePreference.java @@ +6,5 @@ > +package org.mozilla.gecko.preferences; > + > +import org.mozilla.gecko.R; > +import org.mozilla.gecko.GeckoSharedPrefs; > +import org.mozilla.gecko.util.PrefUtils; Not used. @@ +20,5 @@ > +import android.util.Log; > + > +import java.util.Set; > + > +/* Provides backwards compatbility for some old multi-choice pref types used by Gecko. "compatibility" typo @@ +24,5 @@ > +/* Provides backwards compatbility for some old multi-choice pref types used by Gecko. > + * This will import the old data from the old prefs the first time it is run. > + */ > +class MultiPrefMultiChoicePreference extends MultiChoicePreference { > + private static final String LOGTAG = "GeckoMultiPrefMultiChoicePreference"; Nit: isLoggable will throw, this logtag is too long. http://developer.android.com/reference/android/util/Log.html#isLoggable%28java.lang.String,%20int%29 @@ +31,5 @@ > + > + public MultiPrefMultiChoicePreference(Context context, AttributeSet attrs) { > + super(context, attrs); > + > + final TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.MultiChoicePreference); Shouldn't this technically be a new R.styleable.MultiPrefMultiChoicePreference? With a R.styleable.MultiPrefMultiChoicePreference_entryKeys? @@ +47,5 @@ > + > + return prefs.getBoolean(key, defaultReturnValue); > + } > + > + // Overriden to do a one time import fro the old preference type to the new one Nit: "Overridden", "for" typo. Terminating period. @@ +53,5 @@ > + protected synchronized void loadPersistedValues() { > + // This will load the new pref if it exists. > + super.loadPersistedValues(); > + > + // First check if we've already done the import the old data. If so, nothing to load Nit: "already imported the old data"? @@ +55,5 @@ > + super.loadPersistedValues(); > + > + // First check if we've already done the import the old data. If so, nothing to load > + final SharedPreferences prefs = GeckoSharedPrefs.forApp(getContext()); > + final boolean imported = getPersistedBoolean(prefs, getKey() + IMPORT_SUFFIX, false); Nit: newline @@ +69,5 @@ > + } > + > + final int entryCount = keys.length; > + if (entryCount != entries.length || entryCount != init.length) { > + throw new IllegalStateException("MultiChoicePreference entryKeys and initialValues arrays must be the same length"); Update the exception call to reflect MultiPrefMultiChoicePreference. @@ +78,5 @@ > + ThreadUtils.postToBackgroundThread(new Runnable() { > + @Override > + public void run() { > + try { > + // Use one editor to batch as many chages as we can "changes" typo @@ +85,5 @@ > + boolean initialValue = "true".equals(init[i]); > + boolean val = getPersistedBoolean(prefs, key, initialValue); > + > + // Save the pref and remove the old preference > + setValue(i, val); I wonder if this needs to be a call to the synchronized setValue. This is a protected method that only gets called in the constructor... @@ +91,5 @@ > + } > + > + persist(edit); > + edit.putBoolean(getKey() + IMPORT_SUFFIX, true); > + edit.commit(); edit.apply()?

Make sure to include the comments from the previous two reviews of the other patches!

As soon as I pushed this I remembered we had some tests that depend on these string names. I backed it out. Testing a fix: https://hg.mozilla.org/integration/fx-team/rev/b3fb226d9f9c

Test fixes. Look good on try. https://tbpl.mozilla.org/?tree=Try&pusher=wjohnston@mozilla.com Noticed a bunch of places we could use StringHelper but weren't. There are probably more, but I didn't mean this to be exhaustive...

Comment on attachment 8458804 [details] [diff] [review] Test patch Review of attachment 8458804 [details] [diff] [review]: ----------------------------------------------------------------- If it's green on try, wfm.

Wes, Before you reland this we should look at uses of "commit()" for SharedPreferences. Since we are API 9 and higher now, we should always be using "apply()" which does the commit in the background, not blocking the thread.

Changed the commits to apply(). In this case, most of them were using commit because they knew they would be called in the background, but I didn't see any reason it would hurt to put them even more in the background :) https://hg.mozilla.org/integration/fx-team/rev/00b81ba0835a

Verified as fixed in build 33.0a1 (2014-07-21); Devices: Samsung Galaxy R (Android 2.3.4), Motorola RAZR (Android 4.1.2) and Asus Transformer Tab (Android 4.2.1).

Added to the release notes with the (bad) wording: "Option added to clear data when quitting" If you have a better idea, please let me know!

There might be confusion in that a 'quit' option is only added to the existing browser menu only when the feature is used. It's not worded anywhere.

Based on comment 52 will mark firefox33 as verified.

Test Case added to Moztrap: https://moztrap.mozilla.org/manage/case/14071/ - Always clear Browsing history when quitting