Open
Bug 936065
Opened 11 years ago
Updated 2 years ago
[Session Restore] setWindowState() will open and restore additional windows passed in aState
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
REOPENED
People
(Reporter: smacleod, Unassigned)
References
Details
(Keywords: addon-compat)
Attachments
(1 file)
7.26 KB,
patch
|
smacleod
:
review+
|
Details | Diff | Splinter Review |
Since |setWindowState()| directly calls |restoreWindow()| with the entire state it is passed, it is possible to make |setWindowState()| open and restore multiple windows. We should remove this functionality, so that |setWindowState()| will only modify the state of the window it is passed. There may be code which is abusing |setWindowState()| to open and restore multiple windows already, which should be investigated.
Comment 1•11 years ago
|
||
Reporter | ||
Comment 2•11 years ago
|
||
Comment on attachment 8341338 [details] [diff] [review] 0001-Bug-936065-Ignore-every-but-the-first-window-state-p.patch Review of attachment 8341338 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! ::: browser/components/sessionstore/src/SessionStore.jsm @@ +1508,4 @@ > throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG); > }, > > setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) { Maybe we should document this method while we're here changing the behavior? @@ +1518,5 @@ > + throw Components.Exception("Invalid state string: not JSON", Cr.NS_ERROR_INVALID_ARG); > + } > + > + if (!winState.windows || !winState.windows[0]) { > + throw Components.Exception("Invalid window state passed", Cr.NS_ERROR_INVALID_ARG); This appears to change the behavior of setWindowState to throw an exception if there is no window state, where before it would have just not restored. I think this new behavior makes more sense, but could cause breakages with callers. We should keep an eye out for this as well as breakages from the change to only restore a single window.
Attachment #8341338 -
Flags: review?(smacleod) → review+
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 3•11 years ago
|
||
(In reply to Steven MacLeod [:smacleod] from comment #2) > > throw Components.Exception("Window is not tracked", Cr.NS_ERROR_INVALID_ARG); > > }, > > > > setWindowState: function ssi_setWindowState(aWindow, aState, aOverwrite) { > > Maybe we should document this method while we're here changing the behavior? Sure. > This appears to change the behavior of setWindowState to throw an exception > if there is no window state, where before it would have just not restored. > > I think this new behavior makes more sense, but could cause breakages with > callers. We should keep an eye out for this as well as breakages from the > change to only restore a single window. I don't really expect any big breakage but we should definitely watch out for any incompatibility that might result from this change.
Keywords: addon-compat
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7b156069de68
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Comment 6•10 years ago
|
||
This was backed out from 28/29 in bug 950320.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 28 → ---
Comment 7•10 years ago
|
||
Thank you, Ryan. This got lost somewhere...
Updated•10 years ago
|
Assignee: ttaubert → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•