Closed
Bug 558641
Opened 15 years ago
Closed 15 years ago
Port Bug 529674 [Restore windows at the saved position without moving them around on the screen] to SeaMonkey
Categories
(SeaMonkey :: Session Restore, defect)
SeaMonkey
Session Restore
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a2
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(1 file, 3 obsolete files)
4.06 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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•15 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•15 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 4•15 years ago
|
||
As sessionstore gets that stuff from window and saves them in state, then it can safely set it back, right ?
Comment 5•15 years ago
|
||
(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•15 years ago
|
Attachment #438339 -
Flags: review- → review?(neil)
Assignee | ||
Comment 6•15 years ago
|
||
Comment on attachment 438339 [details] [diff] [review] patch I'm requesting review again, based on Paul's comment above.
Comment 7•15 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•15 years ago
|
||
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•15 years ago
|
Attachment #446877 -
Flags: superreview?(neil)
Attachment #446877 -
Flags: superreview-
Attachment #446877 -
Flags: review?(neil)
Comment 9•15 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.
Comment 10•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
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 13•15 years ago
|
||
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•15 years ago
|
Attachment #446954 -
Flags: review?(neil) → review+
Assignee | ||
Comment 14•15 years ago
|
||
Pushed: http://hg.mozilla.org/comm-central/rev/fb73fa07bf9d
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Target Milestone: --- → seamonkey2.1a2
You need to log in
before you can comment on or make changes to this bug.
Description
•