The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.1a2

Status

SeaMonkey
Session Restore
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Trunk
seamonkey2.1a2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
Attachment #438339 - Flags: superreview?(neil)
Attachment #438339 - Flags: review?(neil)

Comment 1

7 years ago
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.
Attachment #438339 - Flags: superreview?(neil)
Attachment #438339 - Flags: review?(neil)
Attachment #438339 - Flags: review-

Comment 2

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

Comment 3

7 years ago
Paul, can You comment or advice ?
(Assignee)

Comment 4

7 years ago
As sessionstore gets that stuff from window and saves them in state, then it can safely set it back, right ?
(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...
(Assignee)

Updated

7 years ago
Attachment #438339 - Flags: review- → review?(neil)
(Assignee)

Comment 6

7 years ago
Comment on attachment 438339 [details] [diff] [review]
patch

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

Comment 7

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

Comment 8

7 years ago
Created attachment 446877 [details] [diff] [review]
use outerWidth and outerHeight attribute

Not sure that I'm doing it right ...
Attachment #438339 - Attachment is obsolete: true
Attachment #446877 - Flags: superreview?(neil)
Attachment #446877 - Flags: review?(neil)
Attachment #438339 - Flags: review?(neil)

Updated

7 years ago
Attachment #446877 - Flags: superreview?(neil)
Attachment #446877 - Flags: superreview-
Attachment #446877 - Flags: review?(neil)

Comment 9

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

Comment 12

7 years ago
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.
Attachment #446877 - Attachment is obsolete: true
Attachment #446904 - Attachment is obsolete: true
Attachment #446954 - Flags: superreview?(neil)
Attachment #446954 - Flags: review?(neil)
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+ ;-)
Attachment #446954 - Flags: superreview?(neil) → superreview+

Updated

7 years ago
Attachment #446954 - Flags: review?(neil) → review+
(Assignee)

Comment 14

7 years ago
Pushed: http://hg.mozilla.org/comm-central/rev/fb73fa07bf9d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Target Milestone: --- → seamonkey2.1a2
You need to log in before you can comment on or make changes to this bug.