656778 - Enable session restore when Panorama usage is detected

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

When TabView.firstUseExperienced is set to true enable session restore (if not already) and notify the user with a little infobar at the bottom. No interaction with the infobar is needed or wanted. It just fades out after some seconds.

We need to enable session restore for people that already have (TabView.firstUseExperienced == true) when they re-enter Panorama the first time after updating.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 532192 [details] [diff] [review]
patch v1

Dunno if that banner "design" is sufficient. The banner text is also quite temporary ;)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

It passes try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=097501d1d155

But one test fails permanently:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/preferences/tests/browser_bug567487.js | Sanity check - Got 3, expected 1

This is because some tests trigger Panorama's new behavior and change the browser.startup.page value. How do we deal with that? Should we just reset the user-value of browser.startup.page in browser_bug567487.js?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> This is because some tests trigger Panorama's new behavior and change the
> browser.startup.page value. How do we deal with that? Should we just reset
> the user-value of browser.startup.page in browser_bug567487.js?

Yea, just do that. Ideally the other test would be cleaning up after itself, but you want this test to start from a known state so make sure it's in that state.

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 532192 [details] [diff] [review]
patch v1

Tested the tryserver build, looking good. Some minor changes:

* I'd show the bar at the bottom of the screen instead of the top, it's just informational, and there's more likelihood of it covering up real groups if it's at the top. Since we're not worried about people missing the info, it should be as non-intrusive as possible.

* Change the text that appears to: 

"Tabs and groups will automatically be restored the next time you start $appname."

ui-r+ with those changes. :)

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #5)
> * I'd show the bar at the bottom of the screen instead of the top, it's just
> informational, and there's more likelihood of it covering up real groups if
> it's at the top. Since we're not worried about people missing the info, it
> should be as non-intrusive as possible.

Fixed.

> * Change the text that appears to: 
> 
> "Tabs and groups will automatically be restored the next time you start
> $appname."

Fixed.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Great. Pushed to try:

http://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]&rev=60f501b5b01e

Crashes on every platform (libxul.so!js::ctypes::ConvertToJS):

http://tinderbox.mozilla.org/showlog.cgi?log=Try/1305373663.1305377038.12420.gz&fulltext=1#err0

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 532424 [details] [diff] [review]
patch v2

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

So I think this looks good. I'm not super wild about the banner behavior though. I sort of wish it were more like our other bannerish notifications that stayed until dismissed (with an button to revert). Limi doesn't seem to agree with me though, so UX team prevails this time.

Poking Shawn for extra review since I'm not technically a reviewer here.

::: browser/base/content/browser-tabview.js
@@ +127,5 @@
>    // ----------
>    // Observes topic changes.
>    observe: function TabView_observe(subject, topic, data) {
> +    if (topic != "nsPref:changed")
> +      return;

There shouldn't be observers that you don't know about right? (It's ok to check but seems like we shouldn't need to).

Comment 9

[Alex Limi (:limi) — Firefox UX Team]

We might want to not show the banner in v1 so we don't have any string impact, to get it landed more easily, and have a shot at getting this into FF5 at release manager's discretion.

That means we need to get it landed on central ASAP, though.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3 (without banner)

(In reply to comment #8)
> There shouldn't be observers that you don't know about right? (It's ok to
> check but seems like we shouldn't need to).

True, removed.

(In reply to comment #9)
> We might want to not show the banner in v1 so we don't have any string
> impact, to get it landed more easily, and have a shot at getting this into
> FF5 at release manager's discretion.

Removed all banner-specific stuff. Now the only thing this patch does is flip the preference.

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 532783 [details] [diff] [review]
patch v3 (without banner)

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

[Stealing from sdwilsh since he's out for the day.]

::: browser/base/content/browser-tabview.js
@@ +72,5 @@
> +  },
> +
> +  // ----------
> +  get sessionRestoreActivatedOnce() {
> +    return this._sessionRestoreActivatedOnce;

Just make this a Services.getBoolPref() [with default if not set], and remove the init() code that fetches it. This doesn't seem to be in any hot path that merits the extra complexity of caching in a property.

That removes worry about somehow getting the cached value (in the property) and actual pref out of sync.

Not to scope-creep (much ;), but I'd suggest doing this for firstUseExperienced for the same reasons.

@@ +78,5 @@
> +
> +  // ----------
> +  set sessionRestoreActivatedOnce(val) {
> +    if (val != this._sessionRestoreActivatedOnce)
> +      Services.prefs.setBoolPref(this.PREF_RESTORE_ACTIVATED_ONCE, val);

Similarly, just unconditionally set the pref here.

@@ +85,5 @@
>    // ----------
>    init: function TabView_init() {
> +    if (!Services.prefs.prefHasUserValue(this.PREF_FIRST_RUN) ||
> +        !Services.prefs.getBoolPref(this.PREF_FIRST_RUN)) {
> +      Services.prefs.addObserver(this.PREF_FIRST_RUN, this, false);

You can add a pref observer for an entire pref branch, I think that would simplify things (especially as you inevitably add more prefs ;).

EG, something like:
  Services.prefs.addObserver("browser.panorama.", this, false);
  observer : function (s,t,d) {
    switch (d) {
      case "experienced_first_run":
      case "session_restore_activated_once":
    }
  }

Just unconditionally add the observer in init(), and remove it in uninit().

@@ +126,5 @@
>  
>    // ----------
>    // Observes topic changes.
>    observe: function TabView_observe(subject, topic, data) {
> +    Services.prefs.removeObserver(data, this);

This (existing) code isn't really great, because it's making the first time the pref is changed behave specially. Remove this in favor of simply controlling the observer's lifespan via init()/uninit().

@@ +137,5 @@
> +        break;
> +      case this.PREF_RESTORE_ACTIVATED_ONCE:
> +        this._activateSessionRestoreOnce();
> +        this._sessionRestoreActivatedOnce = true;
> +        break;

It's a rather weird to indirectly be performing actions based on pref changes, more on this below.

At least add Services.getBoolPref() checks here so these are no-ops if the pref value is being changed to |false|. I already see one test that setting the value to false, so I hope there's not already some magical behavior being assumed...

::: browser/base/content/tabview/ui.js
@@ +553,5 @@
>        TabItems.resumePainting();
>      }
> +
> +    if (gTabView.firstUseExperienced)
> +      gTabView.sessionRestoreActivatedOnce = true;

This is confusing. Someone reading this code is unlikely to expect this to actually _do_ something (it looks like a trivial property change). And if they go looking, they have to understand this may trigger an pref change notification, and go looking for the call to _activateSessionRestoreOnce() there.

This should just be:

  if (conditions)
    gTabView.activateSessionRestore();

Or just always call activateSessionRestore() and let it figure out if it's going to actually do anything. Putting the conditions here seems to make sense, though, since it's the only place it's called.

Now that I've read through this all, though, I'm really confused about exactly what the session_restore_activated_once pref is supposed to be doing. I was expecting this to just be a flag to prevent accidentally restoring the session multiple times per launch. So I'm not sure why it's a preference at all.

One of us is misunderstanding what this bug/patch is trying to do. Not sure who, yet. ;-)

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #11)
> Now that I've read through this all, though, I'm really confused about
> exactly what the session_restore_activated_once pref is supposed to be
> doing. I was expecting this to just be a flag to prevent accidentally
> restoring the session multiple times per launch. So I'm not sure why it's a
> preference at all.

Phrased a different way:

The first time a user enters Panorama and triggers the session-restore stuff added here, the sessionRestoreActivatedOnce pref gets set to true. Nothing seems to ever change it to false. So the next time the user runs the browser and enters Panorama, the session won't be restored because the pref is still true.

Did I miss something, or is that intentional?

Comment 13

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #12)
> The first time a user enters Panorama and triggers the session-restore stuff
> added here, the sessionRestoreActivatedOnce pref gets set to true. Nothing
> seems to ever change it to false. So the next time the user runs the browser
> and enters Panorama, the session won't be restored because the pref is still
> true.
> 
> Did I miss something, or is that intentional?

I assume it's just checking since it flips the pref once, so if the user turned session restore off again, we don't turn it back on yet again. But I can't read the code, so I'll let Tim comment on the specifics — that's the behavior we want, at least.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #13)
> I assume it's just checking since it flips the pref once, so if the user
> turned session restore off again, we don't turn it back on yet again. But I
> can't read the code, so I'll let Tim comment on the specifics — that's the
> behavior we want, at least.

True. We turn on session restore only once. If the user turns it off afterwards we won't do it again.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

(In reply to comment #11)
> Just make this a Services.getBoolPref() [with default if not set], and
> remove the init() code that fetches it. This doesn't seem to be in any hot
> path that merits the extra complexity of caching in a property.
> 
> That removes worry about somehow getting the cached value (in the property)
> and actual pref out of sync.
> 
> Not to scope-creep (much ;), but I'd suggest doing this for
> firstUseExperienced for the same reasons.

Fixed.

> Similarly, just unconditionally set the pref here.

Fixed.

> You can add a pref observer for an entire pref branch, I think that would
> simplify things (especially as you inevitably add more prefs ;).
> 
> Just unconditionally add the observer in init(), and remove it in uninit().

Fixed.

> This (existing) code isn't really great, because it's making the first time
> the pref is changed behave specially. Remove this in favor of simply
> controlling the observer's lifespan via init()/uninit().

Fixed.

> It's a rather weird to indirectly be performing actions based on pref
> changes, more on this below.
> 
> At least add Services.getBoolPref() checks here so these are no-ops if the
> pref value is being changed to |false|. I already see one test that setting
> the value to false, so I hope there's not already some magical behavior
> being assumed...

That's true but we need this in case multiple Panorama instances are running. Though not for the session restore activation.

> This is confusing. Someone reading this code is unlikely to expect this to
> actually _do_ something (it looks like a trivial property change). And if
> they go looking, they have to understand this may trigger an pref change
> notification, and go looking for the call to _activateSessionRestoreOnce()
> there.

Fixed.

Comment 16

[Justin Dolske [:Dolske]]

(In reply to comment #11)

> One of us is misunderstanding what this bug/patch is trying to do. Not sure
> who, yet. ;-)

That would be me. Wow, enormous brainfart. I get it now.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 532923 [details] [diff] [review]
patch v4

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

General question: Do we (UX) really want to enable session restore simply by having entered panorama? This basically means any new Firefox user will get session restore enabled by default if they simply activate Panorama to see what it is (or if they accidentally trigger it).

AIUI this issue was brought up because of dataloss-ish concerns -- people who try panorama, meticulously organize things into groups, restart, and then find Panorama is empty (because they haven't restored their session). What about only enabling session restore when there's a second panorama group created? EG, then someone who uses Panorama just as a tab-switcher (no groups) wouldn't see any change from Firefox 4. Alternatively, add a button in Panorama for "Restore Previous Session" (akin to the "Undo Close Group" button Panorama shows when you close a group).

I mainly just raise this general question to make sure we've considered alternatives to changing a user's pref, since it a little confusing (semi-magical), and some people get grumpy about such things.

Other than that (and the review nitpicks here), the patch looks fine.

::: browser/base/content/browser-tabview.js
@@ +384,5 @@
> +  // ----------
> +  // Function: activateSessionRestore
> +  // Activates automatic session restore when the browser is started. Does
> +  // nothing if we already did that once in the past.
> +  activateSessionRestore: function UI_activateSessionRestore() {

Can we call this "enableSessionRestore" instead of "activate"? Maybe it's just me, but "activate" implies it's being triggered right then.

@@ +400,5 @@
> +    if (startupPage == this.PREF_VALUE_RESTORE_SESSION)
> +      return;
> +
> +    Services.prefs.setIntPref(this.PREF_STARTUP_PAGE,
> +      this.PREF_VALUE_RESTORE_SESSION);

There's no harm from setting a pref to its existing value, so no need to check it first. I'd also get rid of PREF_VALUE_RESTORE_SESSION, just put "3" here with a comment explaining it.

::: browser/base/content/tabview/ui.js
@@ +553,5 @@
>        TabItems.resumePainting();
>      }
> +
> +    if (gTabView.firstUseExperienced)
> +      gTabView.activateSessionRestore();

Can we actually get here with .firstUseExperienced == false? It certainly _seems_ like it should already be true (or will become true momentarily). But it seems like .firstUseExperienced is being set from some odd places, so maybe there's something subtle going on here that I don't get?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #17)
> General question: Do we (UX) really want to enable session restore simply by
> having entered panorama? This basically means any new Firefox user will get
> session restore enabled by default if they simply activate Panorama to see
> what it is (or if they accidentally trigger it).
> 
> AIUI this issue was brought up because of dataloss-ish concerns -- people
> who try panorama, meticulously organize things into groups, restart, and
> then find Panorama is empty (because they haven't restored their session).
> What about only enabling session restore when there's a second panorama
> group created? EG, then someone who uses Panorama just as a tab-switcher (no
> groups) wouldn't see any change from Firefox 4. Alternatively, add a button
> in Panorama for "Restore Previous Session" (akin to the "Undo Close Group"
> button Panorama shows when you close a group).

We're just doing that right now. Session restore is only enabled when the user actually _uses_ Panorama not when just opening it. See bug 626791 comment #33 that implements .firstUseExperienced.

> ::: browser/base/content/tabview/ui.js
> @@ +553,5 @@
> >        TabItems.resumePainting();
> >      }
> > +
> > +    if (gTabView.firstUseExperienced)
> > +      gTabView.activateSessionRestore();
> 
> Can we actually get here with .firstUseExperienced == false? It certainly
> _seems_ like it should already be true (or will become true momentarily).
> But it seems like .firstUseExperienced is being set from some odd places, so
> maybe there's something subtle going on here that I don't get?

Yeah we can get here with (.firstUseExperienced == false) when Panorama is opened but not really used (see above).

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

(In reply to comment #17)
> Can we call this "enableSessionRestore" instead of "activate"? Maybe it's
> just me, but "activate" implies it's being triggered right then.

Yeah, I actually wanted to do that, too. :) Fixed.

> @@ +400,5 @@
> > +    if (startupPage == this.PREF_VALUE_RESTORE_SESSION)
> > +      return;
> > +
> > +    Services.prefs.setIntPref(this.PREF_STARTUP_PAGE,
> > +      this.PREF_VALUE_RESTORE_SESSION);
> 
> There's no harm from setting a pref to its existing value, so no need to
> check it first. I'd also get rid of PREF_VALUE_RESTORE_SESSION, just put "3"
> here with a comment explaining it.

Fixed.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to comment #18)

> We're just doing that right now. Session restore is only enabled when the
> user actually _uses_ Panorama not when just opening it. See bug 626791
> comment #33 that implements .firstUseExperienced.

Ohhhhhh. That explains why it was being set in odd places.

I wish the pref had been renamed as was discussed in that bug, it's a bit misleading as it's used now.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 22

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/2daba4fe422e

Comment 23

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 533086 [details] [diff] [review]
patch for checkin

Asking for approval to land this simple fix to turn on session restore once the user starts using Panorama.

Right now, it's incredibly easy to lose your groups the first time you use Panorama and restart, which leads to (perceived) data loss — we have the session saved, but it's not automatically restored in the default setup. It should be.

This is a simple workaround to make sure we protect people's Panorama sessions until we can fix things properly.

The patch only affects Panorama users, and uses the same code path we already use for deciding whether to show the Panorama button in the main UI. It also takes care to not turn on session restore again if you turned it off manually afterwards. 

We also made sure to implement it in a way that involves no string changes.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 533086 [details] [diff] [review]
patch for checkin

Please land this change on both Aurora and Beta. (In the future, getting changes in during Aurora will save you this extra step.)

Comment 25

[Girish Sharma [:Optimizer]]

This has landed in nightly , but I didn't get it . Can someone please explain what this feature do ? 
Please. In plain english.
Thanks in advance

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/67c9bddd1be8 and http://hg.mozilla.org/releases/mozilla-beta/rev/33c9c89df4db

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #26)
> Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/67c9bddd1be8 and
> http://hg.mozilla.org/releases/mozilla-beta/rev/33c9c89df4db

Followed by http://hg.mozilla.org/releases/mozilla-aurora/rev/e500342a882f and http://hg.mozilla.org/releases/mozilla-beta/rev/a7608e3a3b31 because I missed a conflict when applying the patch.

Comment 28

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Well, that didn't go well. Backed out from Aurora and Beta.

Tim: Please attach a new patch that applies cleanly and passes tests (you should just need to fix uses of clearUserPref to use a try+catch or check hasUserValue since that can still throw on Firefox 5). See http://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=e500342a882f for the latest state.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

(In reply to comment #28)
> Tim: Please attach a new patch that applies cleanly and passes tests (you
> should just need to fix uses of clearUserPref to use a try+catch or check
> hasUserValue since that can still throw on Firefox 5).

Done. Both failing tests do now pass locally.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/releases/mozilla-beta/rev/3a4b24e3dac1

Comment 31

[George Carstoiu]

Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0

Verified issue using the following steps:
1. Open Fx (set option When Firefox starts: Show my home page)
2. Go into Panorama and create a second group and populate it with one tab
3. Go Back to Options and verify that option When Firefox starts is set to Show my windows and tabs from last time

Verified on WinXP, Mac OS X 10.6, Win7 x86, Ubuntu 11.04 x86.