956368 - [session store] Tab Groups lost after switching from desktop to Metro and back

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Components, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1) In desktop Firefox, create multiple tab groups in a single window.
2) From that window, relaunch in Metro Firefox.
3) After Metro Firefox launches, relaunch back to desktop.

Expected results: The tab groups are unchanged.
Actual results: All the tabs from the window are now in one group.

From a UX perspective, I think we should solve this just like bug 950159.  Only the visible tabs from the current window should open in the Metro sesssion.  "Background" tab groups, just like background windows, should be restored unchanged when switching back to desktop.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: v1: Keep track of tab groups when switching to Metro and only switch over tabs from the current group

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8356826 [details] [diff] [review]
v1: Keep track of tab groups when switching to Metro and only switch over tabs from the current group

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

I want to investigate and at least fully document the JSON issue before hacking around it.

::: browser/metro/components/SessionStore.js
@@ +538,5 @@
>  
>      aBrowser.__SS_data = tabData;
>    },
>  
> +  _saveTabData: function(tabList, winData) {

Nit: aTabList, aWinData.

@@ +539,5 @@
>      aBrowser.__SS_data = tabData;
>    },
>  
> +  _saveTabData: function(tabList, winData) {
> +    for (let i = 0; i < tabList.length; i++) {

While we're touching this code... I like "for (let tab of aTabList)" just for conciseness/clarity.

@@ +779,5 @@
>          let windowIndex = this._selectedWindow - 1;
>          let tabs = data.windows[windowIndex].tabs;
>          let selected = data.windows[windowIndex].selected;
>  
> +        // Desktop gives us malformed JSON so we do some extra work to access our tab group ID

Could you file a bug about the malformed JSON and add the bug number to this comment, please?  I'd like to remove this workaround once that's fixed.

@@ +783,5 @@
> +        // Desktop gives us malformed JSON so we do some extra work to access our tab group ID
> +        let currentGroupId;
> +        try {
> +          let extData = data.windows[windowIndex].extData["tabview-groups"];
> +          currentGroupId = JSON.parse(JSON.stringify(eval("(" + extData + ")"))).activeGroupId;

Why the extra parse/stringify instead of just "eval(...).activeGroupId"?

What sort of invalid JSON are we getting?  Desktop's session store and tabview code seems to use a plain JSON.parse to read this property.

Comment 4

[Marina Samuel [:emtwo]]

Attachment: v2: Keep track of tab groups when switching to Metro and only switch over tabs from the current group

Oops, I got thrown off by the extData property which seems to have been stored with JSON.stringify() called twice so it looks different than the rest of the sessionstore.js file, but it works to just parse it a second time like you said and is not malformed.

I don't think we need to file a bug for desktop in this case. Sorry about the confusion.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8357176 [details] [diff] [review]
v2: Keep track of tab groups when switching to Metro and only switch over tabs from the current group

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

Looks good, thanks!

::: browser/metro/components/SessionStore.js
@@ +803,5 @@
>            selected = 1;
>  
>          for (let i=0; i<tabs.length; i++) {
>            let tabData = tabs[i];
> +          let tabGroupId = currentGroupId ?

Maybe change the test to (typeof currentGroupId == "number"), in case zero is ever used as a valid ID.

(Not necessary with the current code, just defensive coding against future changes.)

Comment 6

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/6c4862a2b752

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c4862a2b752

Comment 8

[Marina Samuel [:emtwo]]

Comment on attachment 8357176 [details] [diff] [review]
v2: Keep track of tab groups when switching to Metro and only switch over tabs from the current group

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 924886

User impact if declined: Tab groups are lost when switching from Metro back to desktop.

Testing completed (on m-c, etc.): on m-c since 01-08

Risk to taking this patch (and alternatives if risky): Low-risk, Metro-only.

String or IDL/UUID changes made by this patch: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f6eaab92cb9

Comment 10

[u279076]

Kamil, please verify this has been fixed in Firefox Nightly and Aurora.

Comment 11

[Cornel Ionce [:noni] [Hubs QA]]

Build ID Nightly: 20140122030521
Build ID Aurora: 20140123004002

I can confirm this is now fixed in latest Nightly and Aurora using the STR from the description.