950159 - Keep tabs from other windows in desktop when switching to metro

Status: VERIFIED FIXED

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

Description

[Marina Samuel [:emtwo]]

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.

Comment 1

[Marina Samuel [:emtwo]]

Hi Yuan, do you have suggestions/preference for this? Which one sounds better for the user?

Comment 2

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

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.

Comment 3

[Marina Samuel [:emtwo]]

Attachment: v1: Save state of other desktop windows to be restored when switching back from Metro

With this change, switching back to desktop from Metro will restore all the windows that were previously opened.

Comment 4

[Matt Brubeck (:mbrubeck)]

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...

Comment 5

[Marina Samuel [:emtwo]]

Attachment: v2: Save state of other desktop windows to be restored when switching back from Metro

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.

Comment 6

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/2de2620d564a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2de2620d564a

Comment 8

[Marina Samuel [:emtwo]]

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

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0c13a12ef1c

Comment 10

[Manuela Muntean [Away]]

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.