Last Comment Bug 558641 - Port Bug 529674 [Restore windows at the saved position without moving them around on the screen] to SeaMonkey
: Port Bug 529674 [Restore windows at the saved position without moving them ar...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a2
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 529674
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-11 03:14 PDT by Misak Khachatryan
Modified: 2010-05-23 11:31 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.29 KB, patch)
2010-04-11 03:14 PDT, Misak Khachatryan
no flags Details | Diff | Review
use outerWidth and outerHeight attribute (2.62 KB, patch)
2010-05-22 04:26 PDT, Misak Khachatryan
neil: superreview-
Details | Diff | Review
Proof of concept (2.61 KB, patch)
2010-05-22 11:59 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Review
based on Neil's proposal (4.06 KB, patch)
2010-05-23 05:59 PDT, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Review

Description Misak Khachatryan 2010-04-11 03:14:07 PDT
Created attachment 438339 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-04-11 04:52:16 PDT
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 neil@parkwaycc.co.uk 2010-04-11 04:56:18 PDT
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 2010-04-11 22:21:53 PDT
Paul, can You comment or advice ?
Comment 4 Misak Khachatryan 2010-04-20 09:44:48 PDT
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 reading much bugmail, email directly) 2010-05-05 15:30:04 PDT
(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 2010-05-13 22:16:16 PDT
Comment on attachment 438339 [details] [diff] [review]
patch

I'm requesting review again, based on Paul's comment above.
Comment 7 neil@parkwaycc.co.uk 2010-05-14 01:43:39 PDT
(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 2010-05-22 04:26:01 PDT
Created attachment 446877 [details] [diff] [review]
use outerWidth and outerHeight attribute

Not sure that I'm doing it right ...
Comment 9 neil@parkwaycc.co.uk 2010-05-22 11:44:36 PDT
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 neil@parkwaycc.co.uk 2010-05-22 11:59:42 PDT
Created attachment 446904 [details] [diff] [review]
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 reading much bugmail, email directly) 2010-05-23 02:13:00 PDT
(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 2010-05-23 05:59:11 PDT
Created attachment 446954 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-05-23 09:53:51 PDT
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 2010-05-23 11:30:53 PDT
Pushed: http://hg.mozilla.org/comm-central/rev/fb73fa07bf9d

Note You need to log in before you can comment on or make changes to this bug.