558641 - Port Bug 529674 [Restore windows at the saved position without moving them around on the screen] to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

Attachment: patch

From parent bug:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b4pre)
Gecko/20091118 Namoroka/3.6b4pre ID:20091118034701

When a couple of windows are open and you start/stop the PB mode or restart
Firefox all windows are restored at the position of the first window and moved
to their target (saved) position afterward. That doesn't look nice. Can we
directly restore the windows at their saved position?

Steps:
1. Open a couple of windows
2. Start/Stop the Private Browsing mode

Once you stop the Private Browsing mode you will see the mentioned behavior
from above.

I ran test on my Fedora 12, no failed test seen.

Comment 1

[[e-mail]]

Comment on attachment 438339 [details] [diff] [review]
patch

>+    // Build feature string
>+    let features = "chrome,dialog=no,all";
>+    let winState = aState.windows[0];
>+    WINDOW_ATTRIBUTES.forEach(function(aFeature) {
>+      // Use !isNaN as an easy way to ignore sizemode and check for numbers
>+      if (aFeature in winState && !isNaN(winState[aFeature]))
>+        features += "," + aFeature + "=" + winState[aFeature];
>+    });
This doesn't work for the width and height: in a feature string they are the inner width and height but the persisted values are the outer width and height.

Comment 2

[[e-mail]]

Looks like chrome window inner and outer sizes are the same on Linux, so you wouldn't have noticed. But they are definitely different on Windows.

Comment 3

[Misak Khachatryan]

Paul, can You comment or advice ?

Comment 4

[Misak Khachatryan]

As sessionstore gets that stuff from window and saves them in state, then it can safely set it back, right ?

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> Looks like chrome window inner and outer sizes are the same on Linux, so you
> wouldn't have noticed. But they are definitely different on Windows.

FWIW, without doing anything thorough, it appears to be working (in Minefield). I just open a (2nd) window & resize it. It reopens to the same size. This was on Windows 7. DomI supports this...

That said, it does look like outer(Width|Height) is what _should_ be specified in the feature string...

Comment 6

[Misak Khachatryan]

Comment on attachment 438339 [details] [diff] [review]
patch

I'm requesting review again, based on Paul's comment above.

Comment 7

[[e-mail]]

(In reply to comment #5)
> That said, it does look like outer(Width|Height) is what _should_ be specified
> in the feature string...
Agreed.

Comment 8

[Misak Khachatryan]

Attachment: use outerWidth and outerHeight attribute

Not sure that I'm doing it right ...

Comment 9

[[e-mail]]

Comment on attachment 446877 [details] [diff] [review]
use outerWidth and outerHeight attribute

>-const WINDOW_ATTRIBUTES = ["width", "height", "screenX", "screenY", "sizemode"];
>+const WINDOW_ATTRIBUTES = ["outerWidth", "outerHeight", "screenX", "screenY", "sizemode"];
Won't this cause compatibility problems with previous builds?

>-    case "width":
>+    case "outerWidth":
>       dimension = aWindow.outerWidth;
>       break;
>-    case "height":
>+    case "outerHeight":
>       dimension = aWindow.outerHeight;
>       break;
>     default:
>       dimension = aAttribute in aWindow ? aWindow[aAttribute] : "";
>       break;
>     }
> 
>     if (aWindow.windowState == aWindow.STATE_NORMAL) {
What you can't see here is the call to getAttribute(aAttribute) which won't work for outerWidth/Height.

Also restoreDimensions would have to be changed for this to work.

Comment 10

[[e-mail]]

Attachment: Proof of concept

This is completely untested, but I wrote it because the previous patch made me realise that we already had width/outerWidth conversion code and rather than duplicating it it might be possible to centralise it.

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #9)
> (From update of attachment 446877 [details] [diff] [review])
> >-const WINDOW_ATTRIBUTES = ["width", "height", "screenX", "screenY", "sizemode"];
> >+const WINDOW_ATTRIBUTES = ["outerWidth", "outerHeight", "screenX", "screenY", "sizemode"];
> Won't this cause compatibility problems with previous builds?

Yes.

(In reply to comment #10)
> Created an attachment (id=446904) [details]
> Proof of concept
> 
> This is completely untested, but I wrote it because the previous patch made me
> realise that we already had width/outerWidth conversion code and rather than
> duplicating it it might be possible to centralise it.

I kind of like this and would probably take this back in Firefox.

Comment 12

[Misak Khachatryan]

Attachment: based on Neil's proposal

I basically didn't changed anything from Your proposed patch, added one missed line maybe. I ran tests and ensured none is failing. About compatibility - as I understand for the first run where used outerWidth and outerHeight the position will be reset to default and then will be saved and restored. Don't think that we should implement migration code for that. Paul, i You wish, as I get r+ and sr+ from Neil, I'll prepare patch for Firefox.

Comment 13

[[e-mail]]

Comment on attachment 446954 [details] [diff] [review]
based on Neil's proposal

That was a pretty important oversight! I'm surprised I didn't miss anything else... I guess I should test locally before r+ ;-)

Comment 14

[Misak Khachatryan]

Pushed: http://hg.mozilla.org/comm-central/rev/fb73fa07bf9d