Closed Bug 896515 Opened 7 years ago Closed 7 years ago

Change session restore pref to have three states

Categories

(Firefox for Android :: General, defect)

24 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: ibarlow, Assigned: bnicholson)

References

Details

Attachments

(1 file, 3 obsolete files)

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
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.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #782626 - Flags: review?(wjohnston)
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.
Attachment #782626 - Flags: review?(wjohnston)
(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.
With suggested changes.
Attachment #782626 - Attachment is obsolete: true
Attachment #784500 - Flags: review?(wjohnston)
Attachment #784500 - Flags: review?(wjohnston) → review+
Be sure the update the tests.
Updated to include tests.
Attachment #784500 - Attachment is obsolete: true
Attachment #784791 - Flags: review+
Updated again to include wesj in commit message.
Attachment #784791 - Attachment is obsolete: true
Attachment #784792 - Flags: review+
Inbound is closed, but ready for checkin assuming try push from comment 6 looks OK.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/607a23f631fe
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/607a23f631fe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Flags: in-moztrap?(fennec)
Blocks: 901903
No longer blocks: 901903
Depends on: 905371
Since the specification changed, this issue will not require a test case in moztrap. Creating test case for Bug 904741.
Flags: in-moztrap?(fennec) → in-moztrap-
You need to log in before you can comment on or make changes to this bug.