1247335 - wrong window sizing when session restore moves between lo- and hi-dpi screens

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jonathan Kew [:jfkthame]]

AFAIK so far, this is OS X-only...

STR:
Use a retina MacBook with a lo-dpi external monitor attached, so there are two screens, one with 2x devicePixelRatio and one with 1x.

Run Nightly with one window open, and position it on the main monitor.

Ensure prefs are set to NOT immediately restore the previous session when launching, but instead present the home page with the Restore button.

Quit the browser.

Re-launch the browser; the window should re-open in its previous position on the main display.

*BEFORE* clicking the Restore button, move the window to the other display.

Then click Restore; note how the window moves back to the display where it was when the session was saved (correct) but is sized incorrectly (double size if it was moved from hi- to lo-dpi; half size in the opposite case).


This doesn't happen with release Firefox. I haven't specifically bisected, but I'm confident it's a regression from bug 890156.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow and its implementations

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Use desktop pixel coordinates when loading a nsXULWindow position

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch 3 - Check for potential DPI change after moving or resizing nsGlobalWindow

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 8719018 [details] [diff] [review]
patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow and its implementations

Review of attachment 8719018 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +5821,5 @@
> +nsDocShell::SetPositionDesktopPix(int32_t aX, int32_t aY)
> +{
> +  double scale = 1.0;
> +  GetDevicePixelsPerDesktopPixel(&scale);
> +  return SetPosition(NSToIntRound(aX * scale), NSToIntRound(aY * scale));

Is this calculation correct when mixed DPI monitors are present on the system?

::: dom/base/nsGlobalWindow.cpp
@@ +7058,5 @@
> +
> +    screen->GetContentsScaleFactor(&scale);
> +    DesktopPoint deskPos = devPos / DesktopToLayoutDeviceScale(scale);
> +    aError = treeOwnerAsWin->SetPositionDesktopPix(screenLeftDeskPx + deskPos.x,
> +                                                   screenTopDeskPx + deskPos.y);

Why this code changes device pixels to desktop pixels, then changing them back to device pixels (in SetPositionDesktopPix)?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> Comment on attachment 8719018 [details] [diff] [review]
> patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow
> and its implementations
> 
> Review of attachment 8719018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +5821,5 @@
> > +nsDocShell::SetPositionDesktopPix(int32_t aX, int32_t aY)
> > +{
> > +  double scale = 1.0;
> > +  GetDevicePixelsPerDesktopPixel(&scale);
> > +  return SetPosition(NSToIntRound(aX * scale), NSToIntRound(aY * scale));
> 
> Is this calculation correct when mixed DPI monitors are present on the
> system?

I'm not sure, TBH.... but as far as I know this method is not currently called on an actual nsDocShell instance. So this is really just a placeholder implementation, to be reconsidered if we have examples where it is actually needed and can be tested. (I should add a comment to this effect.)

(Another option might be to make this fail with NS_ERROR_NOT_IMPLEMENTED, until/unless we find an actual need for it?)

The same applies to the implementations in nsDocShellTreeOwner and nsWebBrowser. It's the nsChromeTreeOwner and nsContentTreeOwner implementations that matter here, and those forward to nsXULWindow's SetPositionDesktopPix to avoid doing any conversion.


> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +7058,5 @@
> > +
> > +    screen->GetContentsScaleFactor(&scale);
> > +    DesktopPoint deskPos = devPos / DesktopToLayoutDeviceScale(scale);
> > +    aError = treeOwnerAsWin->SetPositionDesktopPix(screenLeftDeskPx + deskPos.x,
> > +                                                   screenTopDeskPx + deskPos.y);
> 
> Why this code changes device pixels to desktop pixels, then changing them
> back to device pixels (in SetPositionDesktopPix)?

In practice, treeOwnerAsWin is an instance of nsChromeTreeOwner or nsContentTreeOwner, which forwards the parameters directly as desktop pixels; this is necessary because converting through device pixels and back on OS X can lead to incorrect behavior, whereas desktop pixels are globally safe.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow and its implementations

Revised version of patch 1, with the unused(?) methods returning unimplemented. Try run with this version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd93e4cb9319.

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/291cf27e6e55e20279fdb53219038b171abc945a
Bug 1247335 - patch 1 - Provide a desktop-pixel variant of SetPosition on nsIBaseWindow and its implementations. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/134d63b3e942785c325d7c1a35d9c46c9775a072
Bug 1247335 - patch 2 - Use desktop pixel coordinates when loading a nsXULWindow position. r=emk

https://hg.mozilla.org/integration/mozilla-inbound/rev/9feea2de5e1d4c39d2f1d0a69b77a493fa09c4a0
Bug 1247335 - patch 3 - Check for potential DPI change after moving or resizing nsGlobalWindow. r=emk

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/291cf27e6e55
https://hg.mozilla.org/mozilla-central/rev/134d63b3e942
https://hg.mozilla.org/mozilla-central/rev/9feea2de5e1d