Closed
Bug 1446264
Opened 7 years ago
Closed 7 years ago
Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser with the patches for bug 1439875
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | verified |
People
(Reporter: alice0775, Assigned: xidorn)
References
(Depends on 1 open bug)
Details
(Keywords: nightly-community, regression)
Attachments
(5 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
florian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
enndeakin
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
[Tracking Requested - why for this release]:
Reproducible: always
Steps To Re@roduce:
1. Maximize Browser
2. Exit Browser
3. Re-launch Browser
Actual Results:
Browser is opened with normal sizemode instead of maximized sizemode
Expected Results:
Browser should start in maximized sizemode.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cad403cafed9b17d1b801837abcd039a0bce82d5&tochange=990a8eb972cd2a734da04ed8d787564752e8c9c7
Regressed by: Bug 1439875
I was surprised that there was no automated test.
@:emilio
Could you look into this?
Flags: needinfo?(emilio)
Reporter | ||
Updated•7 years ago
|
Component: XUL → DOM
Assignee | ||
Comment 1•7 years ago
|
||
I'm surprised about this too... Probably we don't really have much tests around sessionrestore.
Comment 2•7 years ago
|
||
Another thing I noticed on Linux (previous to Bug 1439875) is that the window shows in the normal mode, then maximizes. This happens only for the first window, but still looks and feels janky. Is it supposed to work like that?
Updated•7 years ago
|
Priority: -- → P1
Summary: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser → stylo: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser
Updated•7 years ago
|
Summary: stylo: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser → Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser
Comment 3•7 years ago
|
||
Further info.
Open the browser - maximize the browse
Using the 'Hamburger' click 'Open New Window' - it opens in small window, not mazimized even though the main window is already.
New Private window same as above, small window.
Comment 4•7 years ago
|
||
What's your OS? STR in comment 3 don't work for me on Linux (new window starts off maximized).
Flags: needinfo?(emilio)
Comment 5•7 years ago
|
||
This is fixed by backout in any case, but I need to look into this before relanding of course...
Summary: Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser → Browser is opened in normal sizemode instead of previous maximized sizemode when restart browser with the patches for bug 1439875
Assignee | ||
Comment 6•7 years ago
|
||
I reproduced it on Windows I think.
Comment 7•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> What's your OS? STR in comment 3 don't work for me on Linux (new window
> starts off maximized).
Sorry, I'm on Windows 10 x64
Comment 8•7 years ago
|
||
Latest Nightly is completely unusable for me (on Linux).
It starts up non-maximized as noted above, but it also doesn't receive
click events at all, so I can't use it for anything. It behaves as if
its transparent to mouse events - when I click on it the terminal window
behind it is raised to the top. Kbd commands appears to work though.
Severity: normal → critical
Comment 9•7 years ago
|
||
Yes, that's bug 1446242. Unfortunately "on Linux" happens to be quite broad and I can't repro on Fedora 27, so I'm trying to get a distro version that repros the problem.
Comment 10•7 years ago
|
||
(Not critical since the offending patches were backed out.)
My distro is Kubuntu 17.10.
Severity: critical → normal
Comment 11•7 years ago
|
||
Patches from bug #1439875 relanded and this bug is back.
Severity: normal → major
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(emilio)
Keywords: nightly-community
Hardware: Unspecified → All
Comment 12•7 years ago
|
||
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #11)
> Patches from bug #1439875 relanded and this bug is back.
Yes, I know, sorry about that. This is a win-only issue, so putting those patches back in the tree seemed worth it to avoid people breaking them in the meantime.
I cannot investigate on my normal laptop, but will investigate on Monday if Xidorn hasn't got around to it.
I suspect this is running earlier than expected on windows now (before mChromeLoaded), and thus we end up not persisting the size mode:
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/xpfe/appshell/nsWebShellWindow.cpp#489
Or perhaps we just need to set mPersistentAttributeMask in BeforeStartLayout:
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/xpfe/appshell/nsXULWindow.cpp#1095
The other thing we can do is landing a partial backout of bug 1439875, by commenting / ifdef'ing out these two lines:
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/xpfe/appshell/nsXULWindow.cpp#2244-2245
But I'm not sure this is severe enough to warrant it.
Assignee | ||
Comment 13•7 years ago
|
||
I'll have a look Monday.
Assignee | ||
Comment 15•7 years ago
|
||
This is because of code in nsXULWindow::SetSize[1] and nsXULWindow::SetPositionAndSize[2] whose comment states that:
> If we're called before the chrome is loaded someone obviously wants this
> window at this size & in the normal size mode (since it is the only mode
> in which setting dimensions makes sense). We don't persist this one-time
> size.
and they set mIgnoreXULSizeMode to true. And later when we try to restore the size mode in nsXULWindow::LoadMiscPersistentAttributesFromXUL we skip it because of that[3].
The comment apparently no longer makes sense because we are always setting size before chrome is loaded since bug 1439875.
I don't have idea immediately how should this be fixed. A quick and dirty way might be adding another flag and set it to true during nsXULWindow::SizeShell, and avoid setting those ignore flags when we are calling into them from SizeShell. I'm not sure whether there is more sensible way to fix this.
[1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/xpfe/appshell/nsXULWindow.cpp#619-627
[2] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/xpfe/appshell/nsXULWindow.cpp#652-659
[3] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/xpfe/appshell/nsXULWindow.cpp#1330-1343
Flags: needinfo?(xidorn+moz)
Comment 16•7 years ago
|
||
So at one point I had a patch that basically replaced those mIgnore* flags for a scoped boolean mSizingShell that was set during SizeShell. It may be worth doing that and using it? That'd catch both sizes, and I think makes sense.
Assignee | ||
Comment 17•7 years ago
|
||
I don't think they should be replaced, though. If someone invokes those methods after BeforeStartLayout but before OnChromeLoaded, we may not want to override that in OnChromeLoaded which calls the various sizing bits again.
Comment 18•7 years ago
|
||
Well, I guess that's pretty much the same idea from comment 15. I don't think there's a more sensible way to do this either.
Comment 19•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17)
> I don't think they should be replaced, though. If someone invokes those
> methods after BeforeStartLayout but before OnChromeLoaded, we may not want
> to override that in OnChromeLoaded which calls the various sizing bits again.
Oh, yeah, I meant converting those bits to something like if (!mSizingShell && !mChromeLoaded)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
Hmmm... the new test times out on Linux... Why GTK widget doesn't trigger resize event when restoring :/
And it causes extra reflows in browser_windowopen_reflows.js on Windows? And unexpected pass in browser_startup_images.js on Linux?
Assignee | ||
Comment 25•7 years ago
|
||
Actually... the failures in browser_windowopen_reflows.js sound very familiar... I think last time when I saw them in try push, they disappeared when it gets landed... Also I cannot reproduce that failure locally...
Comment 26•7 years ago
|
||
When changing this relatively fragile initialization of the sizemode, could you please verify that you are not breaking the case of startup where browser.startup.blankWindow is set to true? It took some hacking to get it to work https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/components/nsBrowserGlue.js#52-65 and bug 1434986.
Comment 27•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #24)
> And unexpected pass in browser_startup_images.js on Linux?
Despite being labelled "unexpected pass" by the test, this sounds like regression.
This test is reporting when we waste time by loading an image that we are not painting. The unexpected pass here means that we are now actually painting this chevron.svg image. This suggests we are now painting the window at a tiny size at which the navigator toolbar overflows, before resizing it to the correct size.
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #26)
> When changing this relatively fragile initialization of the sizemode, could
> you please verify that you are not breaking the case of startup where
> browser.startup.blankWindow is set to true? It took some hacking to get it
> to work
> https://searchfox.org/mozilla-central/rev/
> 3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/components/nsBrowserGlue.
> js#52-65 and bug 1434986.
It doesn't break blankWindow in the sense that the window is opened maximized, but I cannot really tell whether everything works as expected, because it reliably crashes for me locally with or without this patch for some assertion in timestamp...
(In reply to Florian Quèze [:florian] from comment #27)
> Despite being labelled "unexpected pass" by the test, this sounds like
> regression.
I guess we can just mark that as intermittentShown like what we've done in bug 1439875 for Windows.
Assignee | ||
Comment 29•7 years ago
|
||
The Windows failure is pretty weird. Its stack is
> rect@chrome://browser/content/browser-tabsintitlebar.js:106:23
> _update@chrome://browser/content/browser-tabsintitlebar.js:168:28
> updateAppearance@chrome://browser/content/browser-tabsintitlebar.js:61:5
> handleEvent@chrome://browser/content/tabbrowser.xml:761:15
> EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml:127:11
> toolbar_XBL_Constructor@chrome://browser/content/customizableui/toolbar.xml:42:26
> toolbar_XBL_Constructor@chrome://browser/content/customizableui/toolbar.xml:31:13
> init@chrome://browser/content/browser-tabsintitlebar.js:29:16
> onBeforeInitialXULLayout@chrome://browser/content/browser.js:1233:5
> @chrome://browser/content/browser.js:1194:5
> EventListener.handleEvent*@chrome://browser/content/browser.js:1193:3
> @chrome://browser/content/browser.xul:90:3
The event handler calling updateAppearance is for resize event[1], and the resize event seems to be triggered in some random code getting element[2]. That's probably because we flush style for wrapping object for js access, which is needed for XUL elements due to bindings. The code that triggers the toolbar ctor is also just getting element[3].
The most weird thing is that, the whole stack is invoked from onBeforeInitialXULLayout which is actually dispatched before the code path touched here[4], so there should be no chance that it causes issue...
[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/tabbrowser.xml#760
[2] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/components/customizableui/content/toolbar.xml#42
[3] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser-tabsintitlebar.js#29
[4] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/dom/xul/XULDocument.cpp#2747-2752
Assignee | ||
Comment 30•7 years ago
|
||
Another interesting thing is that, in the log, onBeforeInitialXULLayout appears later than the expected reflow from onDOMContentLoaded. Are we calling into onBeforeInitialXULLayout unnecessarily after onDOMContentLoaded?
Codewise, it seems we are definitely dispatching DOMContentLoaded after BeforeInitialXULLayout[1]...
[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/dom/xul/XULDocument.cpp#2779-2781
Assignee | ||
Comment 31•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #30)
> Another interesting thing is that, in the log, onBeforeInitialXULLayout
> appears later than the expected reflow from onDOMContentLoaded. Are we
> calling into onBeforeInitialXULLayout unnecessarily after onDOMContentLoaded?
Oh that's unrelated. The test itself is printing known reflows first and then unexpected. So we would always see unexpected coming in a later place.
Assignee | ||
Comment 32•7 years ago
|
||
Actually... logically this shouldn't happen if onBeforeInitialXULLayout is triggered before onDOMContentLoaded because in that case, TabsInTitlebar._domLoaded should be false, and any call into TabsInTitlebar._update() other than the one from TabsInTitlebar.init() should've been guarded[1].
This makes things more mysterious :/
[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/browser-tabsintitlebar.js#116-118
Assignee | ||
Comment 33•7 years ago
|
||
I put some test code in involving js code, and see the following result:
> onBeforeInitialXULLayout
> TabsInTitlebar._update: domLoaded = false, fromInit = true
> onDOMContentLoaded
> TabsInTitlebar._update: domLoaded = false, fromInit = false
> TabsInTitlebar._update: domLoaded = false, fromInit = false
> TabsInTitlebar.onDOMContentLoaded
> TabsInTitlebar._update: domLoaded = true, fromInit = false
> TabsInTitlebar._update: domLoaded = true, fromInit = false
So TabsInTitlebar._update is invoked after onDOMContentLoaded twice.
In my local machine, I can only see one line of that after TabsInTitlebar.onDOMContentLoaded, so it seems the extra TabsInTitlebar._update is where the problem is.
Assignee | ||
Comment 34•7 years ago
|
||
So... I can reproduce the additional TabsInTitlebar._update call if the window is opened maximized, and that is triggered by a resize event.
The resize event is triggered by two things:
First, nsWindow::SetNonClientMargins invoked from nsXULWindow::SyncAttributesToWidget as part of nsXULWindow::OnChromeLoaded. It calls into UpdateNonClientMargins then ResetLayout. ResetLayout invokes SetWindowPos which triggers OnWindowPosChanged for frame change, for which we call into OnResize to trigger resize event[1]. (Actually this path was added by myself in bug 1443392.
That code path is invoked when the window is not maximized as well, but because the sizemode is normal, the non-client margins aren't actually changed, and consequently resize path is terminated in nsViewManager::DoSetWindowDimensions when it finds the size isn't changed.
Second, nsWindow::Show invoked from nsXULWindow::SetVisibility as part of nsXULWindow::OnChromeLoaded. It calls into ShowWindow which also triggers OnWindowPosChanged but for all of frame change, size and position, because at that moment, the window is still sized as normal, so we actually need a resize.
This is probably unavoidable because we never size the window as maximized before, so when we show it, it has to trigger a resize.
And oh, I just find out that this is a problem indeed because the test on infra opens the window maximized[2] (which I don't think I saw before because not all tasks have screenshot attached), so this is indeed the problem (although I would probably expect it not to be maximized, but that may be a different topic).
So then the question is how can we avoid the extra resize here... We need to make the window maximized before the initial reflow...
[1] https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/widget/windows/nsWindow.cpp#6813
[2] https://taskcluster-artifacts.net/HlGFUbGARKybltnEBh4diQ/0/public/test_info/mozilla-test-fail-screenshot_ajmc_r.png
Comment 35•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34)
> So then the question is how can we avoid the extra resize here... We need to
> make the window maximized before the initial reflow...
Shouldn't that be already the case? I mean... I'm suspicious about OnChromeLoaded causing a resize here. The chromemargin attribute should be set much earlier (or at least that's the point of bug 1445737).
Assignee | ||
Comment 36•7 years ago
|
||
If we can have the window maximized when we call SetSizeMode, both resize triggers would disappear (that's probably because OnWindowPosChanged would be triggered which invokes UpdateNonClientMargins to update the client size, and that resize ends up being terminated in nsViewManager::DoSetWindowDimensions).
I tried to use
> ::ShowWindow(mWnd, SW_HIDE | SW_MAXIMIZE);
in nsWindow::SetSizeMode, and it works in the sense that no extra resize shows up, however the window is not correctly positioned and sized.
Probably windows cannot correctly maximize a hidden window?
Assignee | ||
Comment 37•7 years ago
|
||
Actually... the parameter of ShowWindow doesn't seem to be bit flags, and SW_HIDE is just zero, so that call ends up showing the window directly...
Assignee | ||
Comment 38•7 years ago
|
||
I tried some hacky way to have everything works, but it doesn't seem to work well. What I tried is to add the following code into nsWindow::SetSizeMode:
> if (!mIsVisible) {
> if (aMode == nsSizeMode_Maximized) {
> DWORD style = GetWindowLong(mWnd, GWL_STYLE);
> SetWindowLong(mWnd, GWL_STYLE, style | WS_MAXIMIZE);
> HMONITOR monitor = MonitorFromWindow(mWnd, MONITOR_DEFAULTTONEAREST);
> MONITORINFO monitorInfo;
> monitorInfo.cbSize = sizeof(monitorInfo);
> GetMonitorInfo(monitor, &monitorInfo);
> RECT rect = monitorInfo.rcWork;
> SetWindowPos(mWnd, 0, rect.left, rect.top,
> rect.right - rect.left,
> rect.bottom - rect.top,
> SWP_FRAMECHANGED|SWP_NOACTIVATE|
> SWP_NOOWNERZORDER|SWP_NOZORDER);
> SetWindowLong(mWnd, GWL_STYLE, style);
> }
> }
Setting the window style to WS_MAXIMIZE so that the system is aware of that the window is in the maximized state, and then size the window explicitly with the screen working area rectangle to trigger OnWindowPosChanged so that view manager gets the size we want at the very beginning. And then set the style back, so that the window is able to restore.
The end result of this code is that the window cannot restore to the size we persist.
If the last SetWindowLong is removed, then if user restore the window, it becomes some unusable small size.
I guess there is some super hacky way to make everything just work... but I don't really want to try that route.
Jim, do you have any advice on how can we have the window maximized before showing the window (so that we can run layout with the correct size) while still make it possible to restore to the size we want?
Flags: needinfo?(jmathies)
Assignee | ||
Comment 39•7 years ago
|
||
Another route to fixing the regression is to make the window not maximized... It doesn't seem to me that browser_windowopen_reflows.js is designed to open in maximized mode, so if we make it just open in normal mode, this regression wouldn't show up...
But that also indicates that we would have worse performance depending on the mode of the window, then we may want to add test for that case as well, since always having maximized window seems to be a common practice.
Comment 40•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28)
> (In reply to Florian Quèze [:florian] from comment #26)
> > When changing this relatively fragile initialization of the sizemode, could
> > you please verify that you are not breaking the case of startup where
> > browser.startup.blankWindow is set to true? It took some hacking to get it
> > to work
> > https://searchfox.org/mozilla-central/rev/
> > 3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/browser/components/nsBrowserGlue.
> > js#52-65 and bug 1434986.
>
> It doesn't break blankWindow in the sense that the window is opened
> maximized, but I cannot really tell whether everything works as expected,
> because it reliably crashes for me locally with or without this patch for
> some assertion in timestamp...
I assume this is on a debug build? Is it on Windows or Linux? What's the assertion?
> (In reply to Florian Quèze [:florian] from comment #27)
> > Despite being labelled "unexpected pass" by the test, this sounds like
> > regression.
>
> I guess we can just mark that as intermittentShown like what we've done in
> bug 1439875 for Windows.
It doesn't seem intermittent on your try results so if you want to silence the failure you can just remove 'linux' from the list of platforms at https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/test/performance/browser_startup_images.js#63 But I'm assuming you want to figure out why this regresses, and not just silence the message.
Comment 41•7 years ago
|
||
Could you backout bug 1439875 until this is fixed? Fixing this bug seems to be more complex than expected first.
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8960057 [details]
Bug 1446264 part 4 - Add an approach to remove all persisted values of a document.
https://reviewboard.mozilla.org/r/228828/#review235078
The code seems fine but I'm unclear where you want to use it. Is this just for a test?
Assignee | ||
Comment 43•7 years ago
|
||
(In reply to Neil Deakin from comment #42)
> The code seems fine but I'm unclear where you want to use it. Is this just
> for a test?
Yes, it's just for a test.
Comment 44•7 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #41)
> Could you backout bug 1439875 until this is fixed? Fixing this bug seems to
> be more complex than expected first.
Rather than backing out the patch completely, I'd rather just revert the behavior change (commenting out https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/xpfe/appshell/nsXULWindow.cpp#2264-2265). But I'd really prefer not having to unless completely necessary.
Assignee | ||
Comment 45•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #40)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #28)
> > I guess we can just mark that as intermittentShown like what we've done in
> > bug 1439875 for Windows.
>
> It doesn't seem intermittent on your try results so if you want to silence
> the failure you can just remove 'linux' from the list of platforms at
> https://searchfox.org/mozilla-central/rev/
> 877c99c523a054419ec964d4dfb3f0cadac9d497/browser/base/content/test/
> performance/browser_startup_images.js#63 But I'm assuming you want to figure
> out why this regresses, and not just silence the message.
I have another try push with that removed from the list, but there are failures in the other direction. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d7a4fe25ae8f8f3909b2008f12ebca30eb4838
Actually it's weird... It seems to fail consistently either way, and that happens in different tasks... We are running this test in two tasks with different settings?
Assignee | ||
Comment 46•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #45)
> Actually it's weird... It seems to fail consistently either way, and that
> happens in different tasks... We are running this test in two tasks with
> different settings?
Oh, I see, we do run it twice, one for lodpi, one for hidpi. It seems the hidpi one is regressed somehow.
Updated•7 years ago
|
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8960057 [details]
Bug 1446264 part 4 - Add an approach to remove all persisted values of a document.
https://reviewboard.mozilla.org/r/228828/#review235156
Attachment #8960057 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 49•7 years ago
|
||
If there is no easy way to have window maximized before showing up, I'd suggest we accept the regression on the mochitests, because otherwise we need to backout bug 1439875 which goes a larger step back on performance.
florian, what do you think?
Flags: needinfo?(florian)
Comment 50•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #49)
> If there is no easy way to have window maximized before showing up,
Why is it not easy? Isn't it just a matter of setting the sizemode attribute earlier? It seemed to work well enough for https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/nsBrowserGlue.js#52-56
> I'd suggest we accept the regression on the mochitests,
Are you saying this about the chevron.svg issue on Linux, or the reflows on Windows? Or both?
I think the linux issue is acceptable to whitelist, the Windows one I'm less comfortable.
Flags: needinfo?(florian)
Assignee | ||
Comment 51•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #50)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #49)
> > If there is no easy way to have window maximized before showing up,
>
> Why is it not easy? Isn't it just a matter of setting the sizemode attribute
> earlier? It seemed to work well enough for
> https://searchfox.org/mozilla-central/rev/
> b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/nsBrowserGlue.
> js#52-56
It's... not an issue inside the shared code. It is an issue inside widget code, or more specifically, this is probably limitation from the system.
The problem is that, on Windows, when we invoke nsWindow::SetSizeMode for syncing the sizemode to the window, if the window hasn't been visible, we just record the new sizemode without telling the system anything, and thus the system doesn't trigger the native resize (OnWindowPosChanged) to resize stuff.
The native resize for maximizing is only triggered when we eventually show the window in maximized state in nsWindow::Show via Windows API ShowWindow, but that is too late since we don't want to display window before we finish layout.
A hacky workaround I tried in comment 38 is that we manually set window size to the screen to trigger the native resize forcefully, but that way the system would lose the track of the restored size.
> > I'd suggest we accept the regression on the mochitests,
>
> Are you saying this about the chevron.svg issue on Linux, or the reflows on
> Windows? Or both?
>
> I think the linux issue is acceptable to whitelist, the Windows one I'm less
> comfortable.
I mean both.
Assignee | ||
Comment 52•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #51)
> The problem is that, on Windows, when we invoke nsWindow::SetSizeMode for
> syncing the sizemode to the window, if the window hasn't been visible, we
> just record the new sizemode without telling the system anything, and thus
> the system doesn't trigger the native resize (OnWindowPosChanged) to resize
> stuff.
To be clear, I don't see anything via which we can ask the system to maximize an invisible window.
There is a flag for making window open in maximized state, but setting that flag doesn't trigger the native resize message, so it's not helpful in this case at all.
Assignee | ||
Comment 54•7 years ago
|
||
Alternatively, we can try to have the test not maximize the window so that it wouldn't see the extra resize... I suppose it wasn't designed to catch that at all. It just happens to be opened maximized.
Assignee | ||
Comment 55•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #40)
> I assume this is on a debug build? Is it on Windows or Linux? What's the
> assertion?
Filed bug 1447549 for that, since it isn't quite related to this bug.
Assignee | ||
Comment 56•7 years ago
|
||
Hmm, let's just display the window earlier. It doesn't seem to break other things, and we'll see if that breaks other stuff...
Flags: needinfo?(jmathies)
Flags: needinfo?(florian)
Comment 57•7 years ago
|
||
I mentioned this in one of the related issues, but it might have been overlooked. What's the story on Linux? I'm running Arch Linux with GNOME Shell/Wayland/CSD, and on two computers the Firefox window starts in the restored (normal) state, then gets maximized. This happened even before bug #1439875. Is that the expected behaviour?
Actually, I just tested, and it seems a bit worse: a maximized window flashes as black, then Firefox gets painted in the restored state, and finally it gets maximized again. On another computer, the restored window has its margins flash to black, but I'm not sure about the initial maximized one.
Assignee | ||
Comment 58•7 years ago
|
||
I actually have no idea why Linux can continue to work as expected after bug 1439875... with my patchset (not published in this bug yet because I want to check the try result first), there is a chance that the problem you described on Linux can be resolved as well.
Comment 59•7 years ago
|
||
Thanks. I'll wait until this gets fixed and things stabilize for a bit, then test again and file a new issue.
Assignee | ||
Comment 60•7 years ago
|
||
Hmmm, a little bit better than previous but still have extra reflow: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee72ae566a78374d5d7856a417379f5e2643ebc
This time it's select@chrome://global/content/bindings/textbox.xml
Assignee | ||
Comment 61•7 years ago
|
||
So the new extra reflow happens because my windows patch makes the window show eagerly (to avoid the resize reflow for maximizing), and consequently it steals the focus from the old window.
Before that change, when we want to focus the url bar, the focus event is not dispatched because the window hasn't been displayed at that moment, and thus has no focus. But after that change, the window now has focus, so some more event is dispatched and more code run to trigger an extra reflow.
Assignee | ||
Comment 62•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 70•7 years ago
|
||
mozreview-review |
Comment on attachment 8960933 [details]
Bug 1446264 part 1 - Mark chevron.svg intermittent shown in startup_images test.
https://reviewboard.mozilla.org/r/229668/#review235444
Attachment #8960933 -
Flags: review?(florian) → review+
Comment 71•7 years ago
|
||
mozreview-review |
Comment on attachment 8960934 [details]
Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized.
https://reviewboard.mozilla.org/r/229670/#review235448
::: xpfe/appshell/nsXULWindow.cpp:1371
(Diff revision 1)
> } else {
> - // For maximized windows, ignore the XUL size attributes, as setting the
> - // size would set the window back to the normal sizemode.
> + // For maximized windows, ignore the XUL size and position attributes,
> + // as setting them would set the window back to normal sizemode.
> if (sizeMode == nsSizeMode_Maximized) {
> mIgnoreXULSize = true;
> + mIgnoreXULPosition = true;
Why is this correct? In virtual-desktop setups, can't you have a maximized window with nonzero position?
Comment 72•7 years ago
|
||
mozreview-review |
Comment on attachment 8960056 [details]
Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell().
https://reviewboard.mozilla.org/r/228826/#review235454
r=me
::: commit-message-be113:3
(Diff revision 2)
> +Bug 1446264 part 5 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r?bz
> +
> +When SetSize etc. are called while OnChromeLoaded hasn't been invoked,
"are called before OnChromeLoaded has been invoked"
::: commit-message-be113:10
(Diff revision 2)
> +the window specifically, and thus ignore the values from the <window>
> +element.
> +
> +However, bug 1439875 changes the behavior so that SizeShell is also
> +invoked before OnChromeLoaded, which confuses the functions above, and
> +causes some of the attributes not loaded properly.
"to not be loaded"
::: commit-message-be113:12
(Diff revision 2)
> +
> +However, bug 1439875 changes the behavior so that SizeShell is also
> +invoked before OnChromeLoaded, which confuses the functions above, and
> +causes some of the attributes not loaded properly.
> +
> +This patch add a separate flag to avoid ignoring those attributes when
"adds"
::: xpfe/appshell/nsXULWindow.cpp:582
(Diff revision 2)
>
> NS_IMETHODIMP nsXULWindow::SetPositionDesktopPix(int32_t aX, int32_t aY)
> {
> mWindow->Move(aX, aY);
> + if (mSizingShellFromXUL) {
> + // If we're invoked for sizing from XUL, we want neither ignore anything
"want to neither"
::: xpfe/appshell/nsXULWindow.cpp:626
(Diff revision 2)
>
> DesktopToLayoutDeviceScale scale = mWindow->GetDesktopToDeviceScale();
> DesktopSize size = LayoutDeviceIntSize(aCX, aCY) / scale;
> mWindow->Resize(size.width, size.height, aRepaint);
> + if (mSizingShellFromXUL) {
> + // If we're invoked for sizing from XUL, we want neither ignore anything
"want to neither"
::: xpfe/appshell/nsXULWindow.cpp:664
(Diff revision 2)
> DesktopToLayoutDeviceScale scale = mWindow->GetDesktopToDeviceScale();
> DesktopRect rect = LayoutDeviceIntRect(aX, aY, aCX, aCY) / scale;
> mWindow->Resize(rect.X(), rect.Y(), rect.Width(), rect.Height(),
> !!(aFlags & nsIBaseWindow::eRepaint));
> + if (mSizingShellFromXUL) {
> + // If we're invoked for sizing from XUL, we want neither ignore anything
"want to neither"
Comment 73•7 years ago
|
||
mozreview-review |
Comment on attachment 8960056 [details]
Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell().
https://reviewboard.mozilla.org/r/228826/#review235456
Attachment #8960056 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 74•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8960934 [details]
Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized.
https://reviewboard.mozilla.org/r/229670/#review235448
> Why is this correct? In virtual-desktop setups, can't you have a maximized window with nonzero position?
At least Windows and GTK widget don't agree with you:
* https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/widget/windows/nsWindow.cpp#1793-1796
* https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/widget/gtk/nsWindow.cpp#1190-1193
Comment 75•7 years ago
|
||
mozreview-review |
Comment on attachment 8960058 [details]
Bug 1446264 part 5 - Add test for this bug.
https://reviewboard.mozilla.org/r/228824/#review235462
::: toolkit/content/tests/chrome/test_maximized_persist.xul:15
(Diff revision 2)
> + src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"/>
> +<script class="testbody" type="application/javascript"><![CDATA[
> + SimpleTest.waitForExplicitFinish();
> + const WIDTH = 300;
> + const HEIGHT = 300;
> + const STATE_MAXIMIZED = 1;
I assume you need to hardcode this because this is not a ChromeWindow? If so, please document that.
::: toolkit/content/tests/chrome/test_maximized_persist.xul:39
(Diff revision 2)
> + null, "chrome,dialog=no,all," + features);
> + }
> +
> + function checkWindow(msg, win, sizemode, width, height) {
> + is(win.windowState, sizemode, "sizemode should match " + msg);
> + if (sizemode == STATE_NORMAL) {
Couls you use win.STATE_NORMAL instead of the hardcoded thing?
::: toolkit/content/tests/chrome/test_maximized_persist.xul:74
(Diff revision 2)
> + let win = openWindow();
> + await SimpleTest.promiseFocus(win);
> +
> + // Check the default state.
> + const chrome_url = win.location.href;
> + checkWindow("when open initially", win, STATE_NORMAL, WIDTH, HEIGHT);
And here, can you get STATE_NORMAL from "win"?
::: toolkit/content/tests/chrome/test_maximized_persist.xul:79
(Diff revision 2)
> + checkWindow("when open initially", win, STATE_NORMAL, WIDTH, HEIGHT);
> + const widthDiff = win.outerWidth - win.innerWidth;
> + const heightDiff = win.outerHeight - win.innerHeight;
> + // Maximize the window.
> + await changeSizeMode(() => win.maximize());
> + checkWindow("after maximize window", win, STATE_MAXIMIZED);
And here. And through the rest of the function.
Attachment #8960058 -
Flags: review?(bzbarsky) → review+
Comment 76•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8960934 [details]
Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized.
https://reviewboard.mozilla.org/r/229670/#review235448
> At least Windows and GTK widget don't agree with you:
> * https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/widget/windows/nsWindow.cpp#1793-1796
> * https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/widget/gtk/nsWindow.cpp#1190-1193
Aha. GTK is the one I was worried about. OK, then.
Comment 77•7 years ago
|
||
mozreview-review |
Comment on attachment 8960934 [details]
Bug 1446264 part 2 - Ignore XUL position when sizemode is maximized.
https://reviewboard.mozilla.org/r/229670/#review235470
Attachment #8960934 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 78•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8960058 [details]
Bug 1446264 part 5 - Add test for this bug.
https://reviewboard.mozilla.org/r/228824/#review235462
> Couls you use win.STATE_NORMAL instead of the hardcoded thing?
Oh, that works. Thanks!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (me-too, metoo) |
Comment 83•7 years ago
|
||
mozreview-review |
Comment on attachment 8960935 [details]
Bug 1446264 part 3 - Allow window to show earlier when sizemode is changed before window turned visible.
https://reviewboard.mozilla.org/r/229672/#review235608
lgtm
Attachment #8960935 -
Flags: review?(jmathies) → review+
Comment 84•7 years ago
|
||
mozreview-review |
Comment on attachment 8960936 [details]
Bug 1446264 part 4 - Delay handling of WM_SETFOCUS after we explicit show the window.
https://reviewboard.mozilla.org/r/229674/#review235610
This is risky as it may have unexpected side effects. For example, once we set focus to a window it's assumed the dom state will be in the correct focus state. You're delaying sending the focus event to gecko which could cause odd race issues revolving around focus state. Find with landing htis to see but lets keep an eye out for regression bugs.
Attachment #8960936 -
Flags: review?(jmathies) → review+
Comment 85•7 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38995f6de9df
part 1 - Mark chevron.svg intermittent shown in startup_images test. r=florian
https://hg.mozilla.org/integration/autoland/rev/f9f4a91b3edb
part 2 - Ignore XUL position when sizemode is maximized. r=bz
https://hg.mozilla.org/integration/autoland/rev/8e3dd0825df9
part 3 - Allow window to show earlier when sizemode is changed before window turned visible. r=jimm
https://hg.mozilla.org/integration/autoland/rev/2280d7116066
part 4 - Delay handling of WM_SETFOCUS after we explicit show the window. r=jimm
https://hg.mozilla.org/integration/autoland/rev/27efbd3b9218
part 5 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r=bz
https://hg.mozilla.org/integration/autoland/rev/af69031d49a0
part 6 - Add an approach to remove all persisted values of a document. r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/7b9a6f304c95
part 7 - Add test for this bug. r=bz
Comment 86•7 years ago
|
||
Backed out 7 changesets (bug 1446264) for wpt failures at html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html
Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7b9a6f304c95ba963b76e15bf6e05d26a390e23f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=169562742&repo=autoland&lineNumber=1841
Backout: https://hg.mozilla.org/integration/autoland/rev/22e482785671fb41d2e97ca29d328c2a01c5c715
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 87•7 years ago
|
||
So... part 3 does cause problem. We cannot show the window earlier, because we may restore its sizemode to normal before we start painting due to e.g. window.open with top/left/width/height, and that could lead to some flicking, which should be considered a regression itself. Whether it should affect the wpt is different question, though.
The new test added should have also caught that, but probably an empty window is rendered too fast so it doesn't look that bad, but browser.xul is much heavier.
I guess it is probably impossible to workaround the reflow for maximizing on Windows, then...
Assignee | ||
Comment 88•7 years ago
|
||
On the other hand, the fact that the resizing for window feature happens after the initial layout also indicates we are wasting a resize reflow for that case as well. It's probably something we should improve as well.
Comment 89•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/38995f6de9df
https://hg.mozilla.org/mozilla-central/rev/f9f4a91b3edb
https://hg.mozilla.org/mozilla-central/rev/8e3dd0825df9
https://hg.mozilla.org/mozilla-central/rev/2280d7116066
https://hg.mozilla.org/mozilla-central/rev/27efbd3b9218
https://hg.mozilla.org/mozilla-central/rev/af69031d49a0
https://hg.mozilla.org/mozilla-central/rev/7b9a6f304c95
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 90•7 years ago
|
||
The patches were backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•7 years ago
|
Target Milestone: mozilla61 → ---
Reporter | ||
Comment 91•7 years ago
|
||
FYI, after landing Bug 1447719, first window is opened in maximized mode as expected, but second new window is still opened in normal mode. If setting browser.startup.blankWindow=fasle, the first window is still opened in normal mode.
Assignee | ||
Comment 92•7 years ago
|
||
So currently we are in a state that even backing out the XUL sizing patch is non-trivial... Nothing is trivial forward... The window open path is so complicated :(
Assignee | ||
Comment 93•7 years ago
|
||
So, per discussion with emilio and florian, I think the best way forward currently is to land this patch with the extra reflows whitelisted. We can probably fix bug 1447864 then land the two windows widget patches to remove those extra reflows.
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8960935 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8960936 -
Attachment is obsolete: true
Assignee | ||
Comment 99•7 years ago
|
||
With the extra reflows whitelisted, it looks pretty good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d348bca42ae9c277131554e378959b563093a41
Comment 100•7 years ago
|
||
mozreview-review |
Comment on attachment 8960056 [details]
Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell().
https://reviewboard.mozilla.org/r/228826/#review236098
Bug 1443578 renamed all the browser_*_reflows.js tests as they now also cover flicker.
::: browser/base/content/test/performance/browser_windowopen_reflows.js:33
(Diff revision 4)
> "onDOMContentLoaded@chrome://browser/content/browser.js",
> ],
> maxCount: 2, // This number should only ever go down - never up.
> },
> +
> + // The reflows below are triggered by maximizing the window after
How much effort would it take to push these 2 whitelist entries to the list of exceptions only when the test window is in the maximized state? I think it should be straightforward.
Of course a better solution would be to duplicate the test to have one instance of the test always opening maximized windows, and one instance of the test always opening non-maximized windows. But I don't want to block this bug on that test enhancement.
::: browser/base/content/test/performance/browser_windowopen_reflows.js:41
(Diff revision 4)
> + stack: [
> + "rect@chrome://browser/content/browser-tabsintitlebar.js",
> + "_update@chrome://browser/content/browser-tabsintitlebar.js",
> + "updateAppearance@chrome://browser/content/browser-tabsintitlebar.js",
> + "handleEvent@chrome://browser/content/tabbrowser.xml",
> + "EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml",
This line containing the '*' character won't exist in the stack on beta and later, so your patch here will make this test fail when 61 will merge to beta.
Please remove this line and stop the stack after the "handleEvent@chrome://browser/content/tabbrowser.xml" line.
::: browser/base/content/test/performance/browser_windowopen_reflows.js:51
(Diff revision 4)
> + stack: [
> + "verticalMargins@chrome://browser/content/browser-tabsintitlebar.js",
> + "_update@chrome://browser/content/browser-tabsintitlebar.js",
> + "updateAppearance@chrome://browser/content/browser-tabsintitlebar.js",
> + "handleEvent@chrome://browser/content/tabbrowser.xml",
> + "EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml",
Same here.
Attachment #8960056 -
Flags: review?(florian) → review-
Assignee | ||
Comment 101•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8960056 [details]
Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell().
https://reviewboard.mozilla.org/r/228826/#review236098
> How much effort would it take to push these 2 whitelist entries to the list of exceptions only when the test window is in the maximized state? I think it should be straightforward.
>
> Of course a better solution would be to duplicate the test to have one instance of the test always opening maximized windows, and one instance of the test always opening non-maximized windows. But I don't want to block this bug on that test enhancement.
I doubt how straightforward that could be, because it really depends on when exactly do we maximize the window. It could happen inside openDialog, or it could happen after. Since I don't have a screen with such small resolution to force the window enter maximized state, I have no idea when that happens.
If it happens after openDialog, there is no way in the current setup that we can push it into the list, because the `with*Observer` function wants us to pass the list in before the window opens.
Even if we know, I suspect the timing could actually be very sensitive to code change in the open window path, and depending on that may make the test even more fragile.
Given that we may be able to remove them soonish, I don't want to waste much time on investigating this.
Comment 102•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #101)
> I doubt how straightforward that could be
I was thinking you would just check for the sizemode attribute near the end of the test.
> the `with*Observer` function wants us to
> pass the list in before the window opens.
It's only a reference to the list, you can still change it until the end of the test (since bug 1438233 where I changed reflow tests to collect all stacks during the test and analyze them at the end of the test).
Assignee | ||
Comment 103•7 years ago
|
||
OK, I cannot make it today, then. I don't think I want to wait for another hour for the build to finish and test whether that setting works near 1am.
Comment 104•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #103)
> OK, I cannot make it today, then. I don't think I want to wait for another
> hour for the build to finish and test whether that setting works near 1am.
Even if we add the whitelist entries unconditionally with the assumption that you'll be removing them soon anyway, the tests have changed enough recently that it would be safer to do another try run before landing.
Assignee | ||
Comment 105•7 years ago
|
||
Sure. I just wanted to test it locally before pushing to try run in case there is any silly small mistake (which I usually make especially in late night).
(And at this point, I don't want to touch my dev laptop this weekend, so let's wait for Monday.)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 111•7 years ago
|
||
Comment 112•7 years ago
|
||
mozreview-review |
Comment on attachment 8960056 [details]
Bug 1446264 part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell().
https://reviewboard.mozilla.org/r/228826/#review236538
Thanks for restricting this to maximized windows only :-).
::: browser/base/content/test/performance/browser_windowopen.js:118
(Diff revision 5)
> subject => subject == win);
>
> await BrowserTestUtils.firstBrowserLoaded(win, false);
> await BrowserTestUtils.browserStopped(win.gBrowser.selectedBrowser, "about:home");
>
> + if (Services.appinfo.OS == "WINNT" && win.windowState == win.STATE_MAXIMIZED) {
Nit: we usually use AppConstants.platform == "win" rather than Services.appinfo.OS.
Attachment #8960056 -
Flags: review?(florian) → review+
Comment 113•7 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #112)
> > + if (Services.appinfo.OS == "WINNT" && win.windowState == win.STATE_MAXIMIZED) {
>
> Nit: we usually use AppConstants.platform == "win" rather than
> Services.appinfo.OS.
Sorry, I just noticed this file is already full of Services.appinfo.OS uses. Feel free to ignore this comment, I'll cleanup in a follow-up.
Comment 114•7 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58de007dbd7d
part 1 - Mark chevron.svg intermittent shown in startup_images test. r=florian
https://hg.mozilla.org/integration/autoland/rev/78fa8ca081e0
part 2 - Ignore XUL position when sizemode is maximized. r=bz
https://hg.mozilla.org/integration/autoland/rev/6e74eff6b09d
part 3 - Avoid ignoring and persisting size/position/sizemode when we are setting them from SizeShell(). r=bz,florian
https://hg.mozilla.org/integration/autoland/rev/6acd877b6f28
part 4 - Add an approach to remove all persisted values of a document. r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/6dbe66440bd2
part 5 - Add test for this bug. r=bz
Comment 116•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/58de007dbd7d
https://hg.mozilla.org/mozilla-central/rev/78fa8ca081e0
https://hg.mozilla.org/mozilla-central/rev/6e74eff6b09d
https://hg.mozilla.org/mozilla-central/rev/6acd877b6f28
https://hg.mozilla.org/mozilla-central/rev/6dbe66440bd2
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 118•7 years ago
|
||
Not fixed yet on 61.0a1 (2018-03-26) (64-bit)
Assignee | ||
Comment 119•7 years ago
|
||
I tested locally on 61.0a1 (2018-03-26) (64-bit) on macOS and it seems to me this has been fixed.
Note that you need to maximize the window once to have it work, otherwise the normal state from last close may have been persisted.
If the issue is still there, mind telling what OS are you using?
Comment 120•7 years ago
|
||
61.0a1 (2018-03-26) (64-bit) Win
Ctrl + shift + J to open Browser console
it is small
Maximise
Close
Ctrl + shift + J to open Browser console
it is still small
F12 to open Browser console
it is small
Maximise
Close
F12 to open Browser console
it is still small
History opens up maximised
Comment 121•7 years ago
|
||
> 61.0a1 (2018-03-26) (64-bit) Win
That build presumably predates this fix being merged (which happened on the afternoon of the 26th, Pacific time).
To make sure, please say what the revision id in about:buildconfig is.
Comment 122•7 years ago
|
||
Assignee | ||
Comment 123•7 years ago
|
||
Yep, I can reproduce the problem with browser and web console on Windows (but not macOS), while the browser itself can be opened in maximized correctly.
That's probably worth a separate bug as this bug is mainly for browser window.
Comment 124•7 years ago
|
||
> Built from https://hg.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af
OK, that does have the changes from this bug.
Comment hidden (typo) |
Reporter | ||
Comment 126•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #124)
> > Built from https://hg.mozilla.org/mozilla-central/rev/de32269720d056972b85f4eec5f0a8286de6e3af
>
> OK, that does have the changes from this bug.
alreay filed bug 1448760
Comment 127•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #119)
> I tested locally on 61.0a1 (2018-03-26) (64-bit) on macOS and it seems to me
> this has been fixed.
>
> Note that you need to maximize the window once to have it work, otherwise
> the normal state from last close may have been persisted.
>
> If the issue is still there, mind telling what OS are you using?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #123)
> Yep, I can reproduce the problem with browser and web console on Windows
> (but not macOS), while the browser itself can be opened in maximized
> correctly.
>
> That's probably worth a separate bug as this bug is mainly for browser
> window.
On OS X, still reproducing with browser windows, but only private navigation windows. Normal windows correctly open up maximized.
And before this fix, private windows were opening in a very smallsize, now it's at maximum height, but close to maximum width: there's still some margin/space on the left.
Assignee | ||
Comment 128•7 years ago
|
||
(In reply to Clément Lefèvre from comment #127)
> On OS X, still reproducing with browser windows, but only private navigation
> windows. Normal windows correctly open up maximized.
> And before this fix, private windows were opening in a very smallsize, now
> it's at maximum height, but close to maximum width: there's still some
> margin/space on the left.
Sounds like it's worth a separate bug as well.
Comment 129•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #128)
> (In reply to Clément Lefèvre from comment #127)
> > On OS X, still reproducing with browser windows, but only private navigation
> > windows. Normal windows correctly open up maximized.
> > And before this fix, private windows were opening in a very smallsize, now
> > it's at maximum height, but close to maximum width: there's still some
> > margin/space on the left.
>
> Sounds like it's worth a separate bug as well.
I was currently adding it as a comment on bug 1448760, why doesn't this fit? Btw, people restricted bug 1448760 to Windows while it still looks like it affect other OS.
Assignee | ||
Comment 130•7 years ago
|
||
(In reply to Clément Lefèvre from comment #129)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #128)
> > (In reply to Clément Lefèvre from comment #127)
> > > On OS X, still reproducing with browser windows, but only private navigation
> > > windows. Normal windows correctly open up maximized.
> > > And before this fix, private windows were opening in a very smallsize, now
> > > it's at maximum height, but close to maximum width: there's still some
> > > margin/space on the left.
> >
> > Sounds like it's worth a separate bug as well.
>
> I was currently adding it as a comment on bug 1448760, why doesn't this fit?
> Btw, people restricted bug 1448760 to Windows while it still looks like it
> affect other OS.
That's about the browser console and web console, not about the browser window itself. And I cannot reproduce that issue on browser console or web console on macOS, so I believe that's an issue specific to Windows.
What you described sounds like something related to the browser window itself, and it happens on macOS, so I'd suggest they are different issues unless you can prove they are the same. We can always duplicate them if we find they are actually the same.
Comment 131•7 years ago
|
||
(In reply to Clément Lefèvre from comment #129)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #128)
> > (In reply to Clément Lefèvre from comment #127)
> > > On OS X, still reproducing with browser windows, but only private navigation
> > > windows. Normal windows correctly open up maximized.
> > > And before this fix, private windows were opening in a very smallsize, now
> > > it's at maximum height, but close to maximum width: there's still some
> > > margin/space on the left.
> >
> > Sounds like it's worth a separate bug as well.
>
> I was currently adding it as a comment on bug 1448760, why doesn't this fit?
> Btw, people restricted bug 1448760 to Windows while it still looks like it
> affect other OS.
Filed follow-up bug as bug 1449111. I used same component as this one, but I'm kinda unsure about this one.
Comment 133•7 years ago
|
||
Bug 1458860 can be a result of this one. I'm not sure and adding as dependency.
Please remove it if you think that's completely unrelated.
Depends on: 1458860
Comment 134•7 years ago
|
||
Hello, in Nighlty this problem back again one or two days ago, have you any test to catch that bug?
Updated•7 years ago
|
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 135•7 years ago
|
||
That's bug 1460639 regressed by bug 1453788.
Flags: needinfo?(xidorn+moz)
Updated•7 years ago
|
Flags: qe-verify+
Comment 136•7 years ago
|
||
I managed to reproduce the initial issues on 61.0a1 (2018-03-24). I can confirm that 61.0b6 build1 (20180517141400) is verified fixed across platforms (Windows 10 x64, Ubuntu 16.04 x64, macOS 10.13.4): the maximized sizemode is preserved between different windows and different sessions.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•