Splitting this off from bug 674373 because it is excessively long already. The initial HiDPI patches in bug 674373 should work for single-screen Retina-display machines, but don't fully work across multiple screens with mixed resolutions. So HiDPI support will initially be preffed-off for such systems. In this bug, we need to handle window/widget management properly across multiple screens with different device-pixel to CSS-pixel and Cocoa-point ratios (backing scale factor), and deal with dynamic changes to backing-store resolution when a window is moved between screens.

Posting some work-in-progress here; also started a tryserver build at https://tbpl.mozilla.org/?tree=Try&rev=0e501c665ff7 that incorporates bug 674373, bug 785667 and these patches. Known issues at this point include: 1. When moving a window between HiDPI and non-HiDPI displays (or vice versa), the transition is visually untidy: the content appears incorrectly scaled at first, and then after a moment it will redraw with the proper scaling. 2. Various "pop-up" elements (tooltips, combo-box dropdowns, autocomplete lists, etc) behave incorrectly after a window has been moved between screens with differing backing resolutions.

I originally posted this to bug 674373. Using the latest nightly, 18.0a1 (2012-10-03): I had an external monitor plugged into my retina MBP. I unplugged the monitor and then put my laptop to sleep. The firefox windows looked ok. I then woke the laptop with no external displays and (I think) the windows still looked ok, but they were not in the foreground. I Cmd-Tab'd Firefox into the foreground and all of its windows look like the attached image.

Rebased patches to current trunk. Tryserver build at https://tbpl.mozilla.org/?tree=Try&rev=eed996b2274e. Issues mentioned above will still be present; this is unfinished work-in-progress.

Here's a patch, on top of Jonathan's patches, that seems to work quite well. Among other things, you can switch modes while a plugin is "playing". There are still some (temporary) artifacts, but I suspect those are caused by bad inputs to my plugin-mode-switching code. Next week I'll dig further into this. I'm doing a tryserver build of all this bug's patches (Jonathan's plus mine). It should be available in a few hours.

> There are still some (temporary) artifacts, but I suspect those are > caused by bad inputs to my plugin-mode-switching code. Next week > I'll dig further into this. One reason I think this is that the artifacts are just as bad when the plugin is running in process (as Java does by default) as they are when it's running out-of-process (as Flash and Silverlight do by default).

Here's a tryserver build made with all this bug's current patches (including my patch from comment #9): http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-c971f279cf45/try-macosx64/firefox-18.0a1.en-US.mac.dmg

This is just some preliminary cleanup.

We need to use global "display" or CSS pixels to manage window positioning and sizing across mixed-resolution screens. Rather than adding parallel display-pixel APIs everywhere, this seems like the less intrusive way to get there - existing device-pixel APIs are unchanged, but when necessary we can find out how to convert them to global units.

The main preparation needed for mixed-resolution support is to manage XUL windows using global display pixels rather than screen-specific device pixels.

nsWindowWatcher also needs to use display pixels, to avoid ugly two-stage sizing behavior when a new window is opened on a secondary display.

The ScreenForRect method expects global CSS pixels, so we need to fix the popup-constraining code accordingly.

nsCocoaWindow::Resize has code to constrain the resized window to its current screen, but in a mixed-resolution situation this can cause tooltip windows to remain constrained to the screen on which they were first displayed, after the main window moves to a different screen. The constraint should only be used when we're using Resize strictly to do a resize, not when we're doing a complete re-position-and-size of the window.

This is the patch that actually handles resolution-change notifications and ensures we repaint the window properly on its new screen. See below for known issues still remaining...

Window size constraints operate in device pixels, so we need to scale the stored values when the device resolution changes. An alternative would be to store the constraints in display pixels, but that seems like a more intrusive change as it would require checking and updating all users of the Set/GetSizeConstraints APIs.

Tryserver build with the latest patch queue here, including Steven's plugins patch, is in progress at https://tbpl.mozilla.org/?tree=Try&rev=67a436f16420. Known issues at this point: * somewhat ugly transition when dragging a window between screens of differing resolution - there's a brief flash of wrongly-scaled content. * tooltips may sometimes get "stuck" at the wrong size when they've been displayed on one screen, then the window is moved to a different screen and the tooltip is re-displayed. * when a window is straddling two screens of different resolutions, any tooltips displayed on the screen containing the smaller part of the window will be incorrectly scaled/positioned.

Well, that burned on tryserver :( ... looking into it, fresh attempt coming soon...

The failed non-Mac builds were due to a minor issue in the plugins patch; this update should resolve that problem. New tryserver job is https://tbpl.mozilla.org/?tree=Try&rev=bd8346dfca36. There'll be an M4 failure in test_window_open_units.html; looking into that. Still, I expect the build should be pretty usable.

(Steven, the change in the plugins patch is just to move nsPluginInstanceOwner::ContentsScaleFactorChanged out of the #ifdef XP_MACOSX conditionals, so that it is present for all platforms.)

It turns out there's an error in the test_window_open_units.html test, such that it doesn't check the height properly; it should have been reporting two failures, not just one. This patch fixes the test so that it now fails in both dimensions.

On the bright side, tryserver indicates that the patches here will fix bug 602745, so we can remove the Windows-specific "todo" from this test.

Comment on attachment 670753 [details] [diff] [review] pt 0.9 - provide a defaultScale attribute on nsIBaseWindow to expose the default device-pix/css-pix ratio Review of attachment 670753 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsDocShell.cpp @@ +4946,5 @@ > NS_IMETHODIMP > +nsDocShell::GetDefaultScale(double *aScale) > +{ > + *aScale = mParentWidget ? mParentWidget->GetDefaultScale() : 1.0; > + return NS_OK; Seems to me this should return something for non-root docshells, so we should delegate this to the parent docshell if there's no mParentWidget. ::: widget/nsIBaseWindow.idl @@ +187,5 @@ > + The default scale factor used by the window. This is the default number of > + device pixels per CSS pixel. Note that this may change if the window is > + moved between screens with differing resolutions. > + */ > + readonly attribute double defaultScale; Fix indent. This needs to explain more clearly what the "default" scale is. In particular it should say that this is the GetDefaultScale() of the underlying widget, and that this is the number of device pixels per CSS pixel for the window's screen at the default zoom level.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > Seems to me this should return something for non-root docshells, so we > should delegate this to the parent docshell if there's no mParentWidget. I guess by QI'ing mParent to nsIDocShellTreeOwner and then nsIBaseWindow.

Comment on attachment 670753 [details] [diff] [review] pt 0.9 - provide a defaultScale attribute on nsIBaseWindow to expose the default device-pix/css-pix ratio Review of attachment 670753 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsIBaseWindow.idl @@ +187,5 @@ > + The default scale factor used by the window. This is the default number of > + device pixels per CSS pixel. Note that this may change if the window is > + moved between screens with differing resolutions. > + */ > + readonly attribute double defaultScale; Actually I think this deserves a better name too. How about screenDevicePixelsPerCSSPixel?

Comment on attachment 670754 [details] [diff] [review] pt 1.0 - use global display pixel coordinates to persist and restore XUL window position and size Review of attachment 670754 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/nsIScreen.idl @@ +35,5 @@ > void GetRect(out long left, out long top, out long width, out long height); > void GetAvailRect(out long left, out long top, out long width, out long height); > > /** > + * And these report in global display pixels Can we refer to these as "screen CSS pixels" instead? I think that's a bit clearer. What do you think?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > > Seems to me this should return something for non-root docshells, so we > > should delegate this to the parent docshell if there's no mParentWidget. > > I guess by QI'ing mParent to nsIDocShellTreeOwner and then nsIBaseWindow. nsDocShell has an mTreeOwner pointer - would it be reasonable to use this (with QI to nsIBaseWindow) rather than going via mParent, or is that something entirely different?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Comment on attachment 670753 [details] [diff] [review] > pt 0.9 - provide a defaultScale attribute on nsIBaseWindow to expose the > default device-pix/css-pix ratio > > Review of attachment 670753 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/nsIBaseWindow.idl > @@ +187,5 @@ > > + The default scale factor used by the window. This is the default number of > > + device pixels per CSS pixel. Note that this may change if the window is > > + moved between screens with differing resolutions. > > + */ > > + readonly attribute double defaultScale; > > Actually I think this deserves a better name too. > > How about screenDevicePixelsPerCSSPixel? That seems misleading to me, because it sounds like it would change when the page is zoomed. I think we need to include "default" or "unscaled" or something like that in the name, to clarify that this is independent of zoom. And "screen" doesn't add anything useful here AFAICS. How about unscaledDevicePixelsPerCSSPixel?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > Comment on attachment 670754 [details] [diff] [review] > pt 1.0 - use global display pixel coordinates to persist and restore XUL > window position and size > > Review of attachment 670754 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/nsIScreen.idl > @@ +35,5 @@ > > void GetRect(out long left, out long top, out long width, out long height); > > void GetAvailRect(out long left, out long top, out long width, out long height); > > > > /** > > + * And these report in global display pixels > > Can we refer to these as "screen CSS pixels" instead? I think that's a bit > clearer. What do you think? I borrowed "display pixels" from https://bugzilla.mozilla.org/show_bug.cgi?id=785667#c68, and I'd be inclined to stay with it. The trouble with "CSS pixels" is that they respond to page zoom within any given browser window, whereas these "display pixels" are meant to be a single, consistent coordinate system across all the displays that make up the user's desktop. ISTM that "screen CSS pixels" is more confusing, in that they may be *different* (due to zoom) from the "CSS pixels" we have within a window.

Corrected version of this patch, so it now accounts for zoom and no longer fails the test_window_open_units.html testcase. New tryserver builds with the full patch set at https://tbpl.mozilla.org/?tree=Try&rev=fa725f6b12af and https://tbpl.mozilla.org/?tree=Try&rev=abf4c6aafc68.

Hmm, now the Windows mochitest failures in test_position_on_resize.xul (which "unexpectedly passed" last time) came back. We could simply wallpaper that by dropping patch 1.5 here, as that test is currently failing anyway (and marked "todo"), but I'd like to understand better what's going on.

Comment on attachment 671407 [details] [diff] [review] pt 1.1 - update nsWindowWatcher::SizeOpenedDocShellItem to use global display pixels for window manipulation Jonathan: GetUnscaledDevicePixelsPerCSSPixel() isn't yet defined in the tree or any of your other patches, so this patch fails to build (when built together with all the other patches).

Comment on attachment 670757 [details] [diff] [review] pt 1.3 - nsCocoaWindow::Resize should only constrain the window to its current screen if no new position was passed This seems a bit hackish. As I remember it, there's exactly one "popup window" that's used (and re-used) for all tooltips (and perhaps for all "popup windows" of any kind). Maybe it's better to destroy this window every time the HiDPI mode changes (so that it'll get recreated with the proper size and constraints when it's needed). Let me dig through the code to confirm or deny this.

Changed defaultScale unscaledDevicePixelsPerCSSPixel to nsIBaseWindow and elsewhere; is that a reasonable name to use here?

Updated for name change in the preceding patch.

(In reply to Steven Michaud from comment #36) > Comment on attachment 671407 [details] [diff] [review] > pt 1.1 - update nsWindowWatcher::SizeOpenedDocShellItem to use global > display pixels for window manipulation > > Jonathan: GetUnscaledDevicePixelsPerCSSPixel() isn't yet defined in the > tree or any of your other patches, so this patch fails to build (when built > together with all the other patches). Sorry, I was waiting to see if Roc was happy with the name change (see comments 29 & 32). I've just uploaded fresh copies of the preceding patches, so I think the full set should be buildable again now.

(In reply to Steven Michaud from comment #37) > Comment on attachment 670757 [details] [diff] [review] > pt 1.3 - nsCocoaWindow::Resize should only constrain the window to its > current screen if no new position was passed > > This seems a bit hackish. > > As I remember it, there's exactly one "popup window" that's used (and > re-used) for all tooltips (and perhaps for all "popup windows" of any kind). > Maybe it's better to destroy this window every time the HiDPI mode changes > (so that it'll get recreated with the proper size and constraints when it's > needed). I think that might be beneficial for other reasons, but I think the change to Resize here actually does make sense. What's hackish, if anything, is that we share the actual implementation between Resize(width, height), which really is just a "resize", and Resize(x, y, width, height), which is actually a combination of Move and Resize. And we don't want Move operations to be constrained to the window's current screen; a window can be Moved anywhere on the (multi-screen) desktop. At present, we get away with the constraint on Move+Resize because we don't actually use that API to move the tooltip window from one screen to another when the browser window is moved; as we don't have mixed-resolution support, that becomes simply a Move operation (unconstrained). But with multi-res, it may become Move+Resize, and so we call the Resize(x,y,w,h) method, and hit the (unwanted) constraint. So the refactoring here is intended to allow us to explicitly choose when to apply the current-screen constraint; we want it when calling Resize(w, h), but not when calling Resize(x, y, w, h).

As Jonathan noticed, my previous patch won't build on other platforms than the Mac. Here's what I think is a better way to fix the problem than what Jonathan describes in comment #24 (which doesn't make my patch's NS_PLUGIN_RESOLUTION_CHANGED event infrastructure available on all platforms). The only change from my v1.0 patch (besides updating it to current trunk code) is to wrap the call from nsObjectFrame::HandleEvent() to nsPluginInstanceOwner::ContentsScaleFactorChanged() in an XP_MACOSX ifdef. There's no reason (as best I can tell) why the NS_PLUGIN_RESOLUTION_CHANGED event can't be used on other platforms. But I'm leaving that for others to implement.

(In reply to Jonathan Kew (:jfkthame) from comment #31) > nsDocShell has an mTreeOwner pointer - would it be reasonable to use this > (with QI to nsIBaseWindow) rather than going via mParent, or is that > something entirely different? I think that would work, but I'm not 100% sure about those relationships. Just test that non-root docshells return the right value.

(In reply to Jonathan Kew (:jfkthame) from comment #33) > I borrowed "display pixels" from > https://bugzilla.mozilla.org/show_bug.cgi?id=785667#c68, and I'd be inclined > to stay with it. Fair enough. (In reply to Jonathan Kew (:jfkthame) from comment #32) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > > How about screenDevicePixelsPerCSSPixel? > > That seems misleading to me, because it sounds like it would change when the > page is zoomed. I think we need to include "default" or "unscaled" or > something like that in the name, to clarify that this is independent of > zoom. And "screen" doesn't add anything useful here AFAICS. How about > unscaledDevicePixelsPerCSSPixel? OK.

Comment on attachment 670757 [details] [diff] [review] pt 1.3 - nsCocoaWindow::Resize should only constrain the window to its current screen if no new position was passed I hadn't read you patch carefully enough. Even your new Resize(x, y, width, height) ("resize and move") still constrains the size appropriately for the "destination screen". I hadn't noticed this. I somehow thought it no longer did any constraining whatsoever -- which seemed hacky. Maybe at some point we can add code to delete the single popup window whenever the HiDPI mode changes (if there still is just one popup window). But doing that would probably be riskier than your current patch.

Comment on attachment 671488 [details] [diff] [review] pt 1.0 - use global display pixel coordinates to persist and restore XUL window position and size Review of attachment 671488 [details] [diff] [review]: ----------------------------------------------------------------- Usually we snap rectangle edges instead of width/height, but I think here it's more important to keep window heights and widths constant as the window moves than to ensure window edges align, so this seems good.

We have code that watches for changes to the hidpi preference and the system's display configuration, to enable or disable HiDPI support on the fly, but this doesn't work right; windows end up in a "mixed" state until browser restart. I've looked into fixing this, but one key stumbling-block is that we have no way (short of forcibly resizing the window) to tell existing windows to update their OpenGL surface if we decide to switch modes. As all this is only a temporary hack for while HiDPI support is evolving, I think we should simply remove the observing code and accept that pref/config changes require a browser restart before the new setting takes effect, rather than spend time trying to improve live-switching.

(In reply to Jonathan Kew (:jfkthame) from comment #47) > As all this is only a temporary hack for while HiDPI support is > evolving, I think we should simply remove the observing code and accept that > pref/config changes require a browser restart before the new setting takes > effect, rather than spend time trying to improve live-switching. Definitely!

Comment on attachment 670844 [details] [diff] [review] pt 1.5 - remove the Windows "todo" in test_position_on_resize.xul, as it now passes correctly Obsoleting part 1.5, as the latest version of the nsWindowWatcher patch no longer causes this test to start "unexpectedly" passing on Windows. So back to bug 602745 for this one; maybe the patches here will at least help provide a clue.

Pushed the patches here (including your plugins patch, Steven) to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a2ad2cfb50 https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec67c1dd2e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/4120e702e25b https://hg.mozilla.org/integration/mozilla-inbound/rev/b0be2ef452df https://hg.mozilla.org/integration/mozilla-inbound/rev/8b64501cb566 https://hg.mozilla.org/integration/mozilla-inbound/rev/303f9562beeb https://hg.mozilla.org/integration/mozilla-inbound/rev/4915cfb82b42 https://hg.mozilla.org/integration/mozilla-inbound/rev/430a6d608f0f https://hg.mozilla.org/integration/mozilla-inbound/rev/1ee9ac5c2a0a https://hg.mozilla.org/integration/mozilla-inbound/rev/0cedf8847b2d https://hg.mozilla.org/integration/mozilla-inbound/rev/eaccb5bb50c0 Provided these stick, I'm intending to resolve this bug as "fixed", and file separate followups for the remaining known issues (as per comment 21).

https://hg.mozilla.org/mozilla-central/rev/a2a2ad2cfb50 https://hg.mozilla.org/mozilla-central/rev/5ec67c1dd2e1 https://hg.mozilla.org/mozilla-central/rev/4120e702e25b https://hg.mozilla.org/mozilla-central/rev/b0be2ef452df https://hg.mozilla.org/mozilla-central/rev/8b64501cb566 https://hg.mozilla.org/mozilla-central/rev/303f9562beeb https://hg.mozilla.org/mozilla-central/rev/4915cfb82b42 https://hg.mozilla.org/mozilla-central/rev/430a6d608f0f https://hg.mozilla.org/mozilla-central/rev/1ee9ac5c2a0a https://hg.mozilla.org/mozilla-central/rev/0cedf8847b2d https://hg.mozilla.org/mozilla-central/rev/eaccb5bb50c0

Turns out this gave a compile error when configured with --disable-accessibility, as nsChildView was relying on an indirect #include of nsIPresShell.h. Pushed https://hg.mozilla.org/mozilla-central/rev/366998e4e1dc as a followup to fix this.

I *think* this caused a regression on Windows with non-default DPI settings - see bug 804062.

I'm still experiencing this on the Aurora channel - is this just because the changes aren't that far downstream yet? When plugging in / unplugging a non-HiDPI monitor into a Retina MBP, The windows I had open render as in the screenshot above. Closing the window and re-opening without exiting the process fixes the issue reliably.

This hasn't yet landed on the 18 branch (currently the aurora branch).

> This hasn't yet landed on the 18 branch (currently the aurora branch). And I'm not sure it's ever intended to.

Rollup of all the patches here, for consideration for Aurora.

Comment on attachment 681240 [details] [diff] [review] rollup patch for Aurora 18 [Approval Request Comment] Bug caused by (feature/regressing bug #): HiDPI support (674373) User impact if declined: The initial retina-display support landed in bug 674373 does not handle multiple screens with mixed resolutions, or dynamic changes in the display's hidpi/lodpi mode. If we don't take this bug as well, FF18 users on Retina MacBooks who dynamically connect or disconnect an external display while Firefox is open will experience ugly, mis-scaled windows, which can only be corrected by closing and re-opening the browser. (See the screenshot in attachment 667535 [details].) Testing completed (on m-c, etc.): On m-c for nearly a month now, working ok. Caused one known regression (bug 804062), so if this is uplifted, we'll also want to nominate that to go with it. Risk to taking this patch (and alternatives if risky): Reasonably low risk - although the patch looks large, much of this is plumbing that has no net effect on behavior for platforms where the ratio of CSS to screen pixels is 1:1. String or UUID changes made by this patch: Patch 0.9 changed the UUID of nsIBaseWindow, because a new method was added to this interface.

Adding QA to help verify this bug on Aurora. Thanks!

Comment on attachment 681240 [details] [diff] [review] rollup patch for Aurora 18 Approving the patch on aurora as the Firefox experience seems bad for people using an external display on HiDPI.Also HiDPI support is a new feature in FF18

Jonathan: Juan asked me to verify this - is there is anything specific I have to have set in the display options to test this other than plugging/unplugging an external monitor - for example resolution (retina vs scaled), mirroring, etc. I can try all combinations but don't see anything specific in the bug when people were hitting it.

A couple of scenarios that require the window to switch between retina and non-retina rendering: - With no external display, launch Firefox, so that it's running on the MBPro's built-in retina screen. Then connect an external (non-retina) display (not configured for mirroring, so you get an "extended" desktop area); then drag the Firefox window to the other display. - Start with the Macbook in "clamshell" mode with only the external display, and launch Firefox. Then sleep the machine, detach the external display and open the laptop's own (retina) display, and wake it up. You'll generally see a (rather ugly) flash of incorrectly-scaled content for a moment as the window makes the transition, but then it should display correctly for the new screen.

Verified fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/18.0 Firefox/18.0. I performed the two scenarios that Jonathan described in Comment 64 and did not see any issues other than in Scenario 2 when I woke the machine up the Mac login screen was slightly off center, but soon rectified itself. I did not see any issues with ugly, mis-scaled windows during testing.

Turns out the patches here missed at least one important aspect of the issue - popups may be opened incorrectly placed/scaled on multi-screen systems. See bug 814434 for details.

(In reply to Jonathan Kew (:jfkthame) from comment #4) > Known issues at this point include: > > 1. When moving a window between HiDPI and non-HiDPI displays (or vice > versa), the transition is visually untidy: the content appears incorrectly > scaled at first, and then after a moment it will redraw with the proper > scaling. This issue remains in the Firefox 18 beta, and I can't find a specific bug to track that problem. Should I open a new bug, or this just a failure of my search-fu? I've also seen (difficult-to-reproduce) situations where this visual untidyness happens for the current window, but clicking on the "back" button brings up old (presumably cached) pages drawn at the old DPI. So, for example, starting on a standard DPI screen, go to a first page and then a second page. Drag the window onto a high DPI monitor. The text gets tiny and then normal sized. Click the back button. This page is, on occasion, rendered in a tiny, tiny font. There's a related behavior where the scroll position on the page can be moved around in unexpected ways when moving between screens with different DPIs, but I suspect it's tied up in the visual glitches described above.