|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
Here's the stack: ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm:4265:7 ssi_updateWindowFeatures/<@resource:///modules/sessionstore/SessionStore.jsm:3023:24 ssi_updateWindowFeatures@resource:///modules/sessionstore/SessionStore.jsm:3022:5 ssi_collectWindowData@resource:///modules/sessionstore/SessionStore.jsm:3213:5 getCurrentState/<@resource:///modules/sessionstore/SessionStore.jsm:3061:11 ssi_forEachBrowserWindow@resource:///modules/sessionstore/SessionStore.jsm:4138:9 getCurrentState@resource:///modules/sessionstore/SessionStore.jsm:3057:7 getCurrentState@resource:///modules/sessionstore/SessionStore.jsm:362:12 _saveState@resource:///modules/sessionstore/SessionSaver.jsm:246:17 _saveStateAsync@resource:///modules/sessionstore/SessionSaver.jsm:309:5 runDelayed/this._timeoutID<@resource:///modules/sessionstore/SessionSaver.jsm:184:40 setTimeout_timer@resource://gre/modules/Timer.jsm:30:5
This is: dimension = aWindow.outerWidth; at http://searchfox.org/mozilla-central/rev/f225dbcb15ca2e38f7d434a9278a41d2340e7cf3/browser/components/sessionstore/SessionStore.jsm#4265 I wonder if this could use lazyWidth instead (http://searchfox.org/mozilla-central/rev/f225dbcb15ca2e38f7d434a9278a41d2340e7cf3/dom/base/nsIFrameLoader.idl#246)
Component: Untriaged → Session Restore
It's not clear to why outerWidth on a top-level window would flush layout. Feels like this should be avoidable like it was for mozInnerScreenX (bug 1307134).
Otherwise I agree that we could use lazyWidth here.
Assignee: nobody → dao+bmo
Component: Session Restore → DOM: Core & HTML
Product: Firefox → Core
nsGlobalWindow::CheckSecurityLeftAndTop is using the same pattern and should probably get the same treatment, but I'm not quite sure what the purpose of that function is.
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8860693 [details] Bug 1358809 - Remove unnecessary layout flush for outerWidth and outerHeight. https://reviewboard.mozilla.org/r/132640/#review135988 This is the outersize, it has nothing to do with the current document (unless the current document happens to be the root document). And I'm not even sure a flush could change that value. So _if_ we need to flush here it seems like we are flushing the right thing.
Attachment #8860693 - Flags: review?(tnikkel) → review-
Bug 26673 added the layout flush in a bulk patch with no particular explanation why outerWidth/Height would need this.
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
I'm not familiar with where the size of a docshell tree owner comes from, so I can't say if we need a flush or not to get an up to date value. So I tried to forward the review to bz (or anyone else who might know).
The size comes from the bounds of the nsIWidget for the nsXULWindow. So as long as we don't have cases where the nsXULWindow auto-resizes to its content (as opposed to explicit sizeToContent calls), not flushing here should be OK. I _think_ we never do that, but if we do I'm hoping Dão would actually know about it...
Comment on attachment 8860693 [details] Bug 1358809 - Remove unnecessary layout flush for outerWidth and outerHeight. https://reviewboard.mozilla.org/r/132640/#review137002 r=me
Attachment #8860693 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #11) > The size comes from the bounds of the nsIWidget for the nsXULWindow. > > So as long as we don't have cases where the nsXULWindow auto-resizes to its > content (as opposed to explicit sizeToContent calls), not flushing here > should be OK. I _think_ we never do that, but if we do I'm hoping Dão would > actually know about it... As far as I know, we don't do that.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/1be01419533e Remove unnecessary layout flush for outerWidth and outerHeight. r=bz
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 892154
from bug 892154 comment 1: > When this bug is fixed, you should update browser_windowopen_reflows.js to > not mention the sync reflow coming from Session Store.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43 followup: update browser_windowopen_reflows.js to account for the reflow in ssi_getWindowDimension having been removed (no review)
For QA: For testing this, I'd have multiple windows open, and ensure that we properly save their heights and widths when closing those windows. When restored from the History menu, those closed windows should always have the same dimensions as when they closed.
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf][photon-performance][qa-commented]
Environment: Ubuntu 16.04 x64, Windows 10 Pro x64, Mac OsX 10.12 57.0a1 20170822171841 55.0.2 20170814072924 With bug 1393013 identified as not a regression caused by this bug and also by taking in account the bugs blocking bug 1330633, I will mark this bug as verified from the point of view that it hasn't introduced any new regression.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox57: --- → verified
You need to log in before you can comment on or make changes to this bug.