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

VERIFIED FIXED in Firefox 28

Status

P2
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mbrubeck, Assigned: emtwo)

Tracking

({dataloss})

28 Branch
Firefox 29
dataloss
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified, firefox29 verified)

Details

(Whiteboard: [beta28] [defect] p=2)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Whiteboard: [defect] [triage] p=0 → [beta28] [defect] p=0

Updated

5 years ago
Assignee: nobody → msamuel
Blocks: 955892
No longer blocks: 838081
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=2
(Reporter)

Updated

5 years ago
Duplicate of this bug: 950935
(Assignee)

Comment 2

5 years ago
Created attachment 8356826 [details] [diff] [review]
v1: Keep track of tab groups when switching to Metro and only switch over tabs from the current group
Attachment #8356826 - Flags: review?(mbrubeck)
(Reporter)

Comment 3

5 years ago
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-
(Assignee)

Comment 4

5 years ago
Created attachment 8357176 [details] [diff] [review]
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.
Attachment #8356826 - Attachment is obsolete: true
Attachment #8357176 - Flags: feedback?(mbrubeck)
(Reporter)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][beta28] [defect] p=2 → [beta28] [defect] p=2
Target Milestone: --- → Firefox 29
(Reporter)

Updated

5 years ago
Depends on: 958166
(Assignee)

Comment 8

5 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f6eaab92cb9
status-firefox28: --- → fixed
status-firefox29: --- → fixed
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
status-firefox28: fixed → verified
status-firefox29: fixed → verified
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.