Closed Bug 950159 Opened 6 years ago Closed 6 years ago

Keep tabs from other windows in desktop when switching to metro

Categories

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

x86_64
Windows 8
defect

Tracking

(firefox28 verified, firefox29 verified)

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

People

(Reporter: emtwo, Assigned: emtwo)

References

Details

(Whiteboard: [beta28] p=1)

Attachments

(1 file, 1 obsolete file)

Following up from bug 924886 comment 12 point 4. We want to not lose tab data from other windows when we switch to the single-window metrofx. A couple of temporary solutions until we have support for multiple windows:

1) Move all the tabs from all the desktop windows into the single metrofx window. Note that in this case switching back to desktop would keep all the tabs in one window. 

2) Only move over the tabs from the window where 'switch to metro' was chosen but when switching back, restore the other windows that were previously open on desktop. Note that the user would then not have access to all their tabs from desktop on Metro.

It's a bit of a trade off, but I prefer option 2 since as far as I know, desktop is capable of handling many tabs but a large number of tabs might cause problems on metro.
Whiteboard: [triage]
Hi Yuan, do you have suggestions/preference for this? Which one sounds better for the user?
Depends on: 924886
Flags: needinfo?(ywang)
Blocks: 924886
No longer depends on: 924886
I prefer 2) option. It's not disruptive. People can still restore tabs in all windows when they switch back from Metro.

My hypothesis is that people won't switch modes until their workflow or context get changed. Their goals on metro and classic also vary depending on the context. An approach that is different and more context appropriate should be expected and acceptable.
Flags: needinfo?(ywang)
With this change, switching back to desktop from Metro will restore all the windows that were previously opened.
Attachment #8348184 - Flags: review?(mbrubeck)
Assignee: nobody → msamuel
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [triage] → [beta28] p=1
Comment on attachment 8348184 [details] [diff] [review]
v1: Save state of other desktop windows to be restored when switching back from Metro

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

r=mbrubeck with one minor change.  There's also some code I think is fine, but I had a question about it (below)...

::: browser/metro/components/SessionStore.js
@@ +730,4 @@
>  
> +        // Move all window data from sessionstore.js to this._windows.
> +        for (let i = 0; i < data.windows.length; i++) {
> +          let SSID = (i == currWindow) ? window.__SSID : "window" + Date.now();

We shouldn't use Date.now() to generate an ID here, since it might return the same result for two different windows.  (I see we already do that elsewhere, but it's especially dangerous here in a loop.)  See bug 927038 for the fix.

@@ +730,5 @@
>  
> +        // Move all window data from sessionstore.js to this._windows.
> +        for (let i = 0; i < data.windows.length; i++) {
> +          let SSID = (i == currWindow) ? window.__SSID : "window" + Date.now();
> +          this._windows[SSID] = data.windows[i];

For the selected window, this will overwrite any data that's already in this._windows[SSID].  Is this safe, or is there any possibility that a tab has already been added to the this._windows[SSID].tabs at this point?  Would it be better just to skip the current window here?

I guess it'll get fixed the next time _collectWindowData runs...
Attachment #8348184 - Flags: review?(mbrubeck) → review+
I noticed that window ordering when switching back to desktop was off and the selected window was incorrect so I made changes to address that.

In terms this._windows[SSID] being overwritten, I think this would only happen if someone opens tabs between the call to 'onWindowOpen()' and 'restoreLastSession()' which is unlikely and probably wouldn't cause issues like you said but I made a change to be on the safe side with that too.
Attachment #8348184 - Attachment is obsolete: true
Attachment #8348909 - Flags: review?(mbrubeck)
Attachment #8348909 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/integration/fx-team/rev/2de2620d564a
Whiteboard: [beta28] p=1 → [fixed-in-fx-team][beta28] p=1
https://hg.mozilla.org/mozilla-central/rev/2de2620d564a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][beta28] p=1 → [beta28] p=1
Target Milestone: --- → Firefox 29
Comment on attachment 8348909 [details] [diff] [review]
v2: Save state of other desktop windows to be restored when switching back from Metro

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

User impact if declined: Users will lose tabs in windows other than the one they switched over to metro when they switch back
 
Testing completed (on m-c, etc.): Tested and works on mc

Risk to taking this patch (and alternatives if risky): Low risk. Only Metro code is affected.
 
String or IDL/UUID changes made by this patch: None
Attachment #8348909 - Flags: approval-mozilla-aurora?
Attachment #8348909 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 956368
Verified as fixed for iteration #21, with latest Nightly and Aurora on Win 8 64-bit: in Metro I can see the moving over of the tabs from the window where 'switch to metro' was chosen (so in Desktop), and when switching back, the other windows that were previously open on Desktop are restored.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.