Closed Bug 956368 Opened 10 years ago Closed 10 years ago

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

Categories

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

28 Branch
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: mbrubeck, Assigned: emtwo)

References

Details

(Keywords: dataloss, Whiteboard: [beta28] [defect] p=2)

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [defect] [triage] p=0 → [beta28] [defect] p=0
Assignee: nobody → msamuel
Blocks: metrov1it22
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=2
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.
Attachment #8356826 - Flags: review?(mbrubeck) → review-
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.
Attachment #8356826 - Attachment is obsolete: true
Attachment #8357176 - Flags: feedback?(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.)
Attachment #8357176 - Flags: feedback?(mbrubeck) → review+
https://hg.mozilla.org/integration/fx-team/rev/6c4862a2b752
Whiteboard: [beta28] [defect] p=2 → [fixed-in-fx-team][beta28] [defect] p=2
https://hg.mozilla.org/mozilla-central/rev/6c4862a2b752
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][beta28] [defect] p=2 → [beta28] [defect] p=2
Target Milestone: --- → Firefox 29
Depends on: 958166
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
Attachment #8357176 - Flags: approval-mozilla-aurora?
Attachment #8357176 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Kamil, please verify this has been fixed in Firefox Nightly and Aurora.
Flags: needinfo?(kamiljoz)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.