896515 - Change session restore pref to have three states

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

The current session restore pref isn't quite what I had in mind when we were discussing it in bug 801412. We should provide users with three options:

# Tabs 
- Always restore
- Restore after a crash [default state]
- Never restore

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Change session restore pref to have three states

Changes the checkbox pref to a ListPreference, which requires a bit of extra work (mostly to make sure the summary is shown). I rearranged the onPreferenceChange() code since we want to set the summary even if we don't set the pref in Gecko, and those operations were lumped together before.

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 782626 [details] [diff] [review]
Change session restore pref to have three states

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

A couple questions, that I bet you have answers for.

Also, it would help reviewing if you'd put unified = 8 in your .hgrc so there was a little more context.

::: mobile/android/base/GeckoApp.java
@@ +1738,5 @@
>              restoreMode = RESTORE_NORMAL;
> +        } else {
> +            String restorePref = PreferenceManager.getDefaultSharedPreferences(this)
> +                                                  .getString(GeckoPreferences.PREFS_RESTORE_SESSION, "crash");
> +            if ("always".equals(restorePref)) {

I'm a bit torn between making these constants and having them live around uselessly forever, and doing this during startup which I assume contributes to some of our GC churn (does it?). This seems fine I guess.

::: mobile/android/base/GeckoPreferences.java
@@ +307,4 @@
>                      preferences.removePreference(pref);
>                      i--;
>                      continue;
> +                } else if (key != null && key.equals(PREFS_RESTORE_SESSION)) {

Dont' we do PREFS_RESTORE_SESSION.equals(key) everywhere here to avoid this? Also, why do we need to do this? The other list prefs don't...

@@ +487,1 @@
>          if (!TextUtils.isEmpty(prefName)) {

Is there some reason we do this even if the pref is an android.not_a_pref pref? I notice health reporter reading some of these, but its using the SharedPrefsObserver stuff and then sending the prefs to some remote web content (i.e. it makes me kinda sick to read...).

Then you could just remove the if statement you had above, and we'd be fine. I kinda like having the very specific individual pref handlers above the generic handlers, as they specific ones might want to override default behavior.

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #2)
> Also, it would help reviewing if you'd put unified = 8 in your .hgrc so
> there was a little more context.

Sorry, that happens when using my git-to-hg scripts :D I'll see if I can adjust it...

> ::: mobile/android/base/GeckoApp.java
> @@ +1738,5 @@
> >              restoreMode = RESTORE_NORMAL;
> > +        } else {
> > +            String restorePref = PreferenceManager.getDefaultSharedPreferences(this)
> > +                                                  .getString(GeckoPreferences.PREFS_RESTORE_SESSION, "crash");
> > +            if ("always".equals(restorePref)) {
> 
> I'm a bit torn between making these constants and having them live around
> uselessly forever, and doing this during startup which I assume contributes
> to some of our GC churn (does it?). This seems fine I guess.

I'm not sure what you're saying here. What do you mean by making them constants that live forever?

> ::: mobile/android/base/GeckoPreferences.java
> @@ +307,4 @@
> >                      preferences.removePreference(pref);
> >                      i--;
> >                      continue;
> > +                } else if (key != null && key.equals(PREFS_RESTORE_SESSION)) {
> 
> Dont' we do PREFS_RESTORE_SESSION.equals(key) everywhere here to avoid this?
> Also, why do we need to do this? The other list prefs don't...

Good point, I'll drop the null check here. We need this here because all other prefs go through Gecko, and their summaries are set once Gecko has responded with the current value and we know which summary to use. This pref is special as it's one of the only ones that lives on the native UI side that Gecko doesn't even know about.

> @@ +487,1 @@
> >          if (!TextUtils.isEmpty(prefName)) {
> 
> Is there some reason we do this even if the pref is an android.not_a_pref
> pref? I notice health reporter reading some of these, but its using the
> SharedPrefsObserver stuff and then sending the prefs to some remote web
> content (i.e. it makes me kinda sick to read...).

I didn't think any not_a_pref prefs were making it that far; I assumed they were all handled above in the special case section and they would all return early. But I agree that checking for not_a_pref here would be better anyway, so I'll do that.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Change session restore pref to have three states, v2

With suggested changes.

Comment 5

[Chenxia Liu [:liuche]]

Be sure the update the tests.

Comment 6

[Brian Nicholson (:bnicholson)]

Try push with updated tests: https://tbpl.mozilla.org/?tree=Try&rev=ccc660d57ae9

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Change session restore pref to have three states. r=wesj

Updated to include tests.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Change session restore pref to have three states. r=wesj

Updated again to include wesj in commit message.

Comment 9

[Brian Nicholson (:bnicholson)]

Inbound is closed, but ready for checkin assuming try push from comment 6 looks OK.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/607a23f631fe

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/607a23f631fe

Comment 12

[u421692]

Since the specification changed, this issue will not require a test case in moztrap. Creating test case for Bug 904741.