803207 - Make nsIWidget::GetDefaultScale aware of high-DPI displays on Android and other platforms

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Brubeck (:mbrubeck)]

As discussed in 716575 comment 33 and 34, the "device pixels per CSS pixel" logic from nsContentUtils::GetDevicePixelsPerMetaViewportPixel should be moved into nsIWidget::GetDefaultScale, so that the default scale will automatically be set >1 on high-density displays on all platforms.

This would make window.devicePixelRatio return the correct value on those displays (fixing bug 794056) and could simplify the <meta name=viewport> code from bug 716575.

(Some platforms would still want to provide their own platform-specific logic, as Mac OS X already did in bug 674373.  For example, the Android widget implementation might want to implement GetDefaultScale in terms of DisplayMetrics.density.)

Comment 1

[Matt Brubeck (:mbrubeck)]

I think this would also let us remove the special-case workaround for mobile that I added in bug 779527.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

This patch breaks scrolling and clicking in Fennec on non-MDPI screens, as mentioned in bug 716575 comment 34.  It looks like scroll offsets (and possibly other coordinates) are no longer converted correctly between device pixels and CSS pixels.  However, I haven't managed to track down where to fix this, mostly because I don't know this code well enough.  Any suggestions?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

To be honest, I'm not sure what's going on here. I looked at the patch and also tried it out. I can reproduce the wonky behaviour. As far as I can tell, the only "external" effect the patch has is that other callers to GetDefaultScale() will now get 2.0 (on my XHDPI galaxy nexus) instead of 1.0 that they used to before. The only such piece of code is is gfx/src/nsDeviceContext.cpp, and it ends up setting mAppUnitsPerDevPixel to 30 instead of 60. I believe this is the desired behaviour of your patch (since it is actually how many app units get mapped to each device pixel). I've verified that if I hard-code a 1.0 at [1] then everything works fine, so it is in fact the nsDeviceContext values that are throwing things off.

Fennec's Java and JS code is all oblivious to the exact mapping between app units and device pixels; as far as I know only gfx code cares about this value. However the exact place in the code where this value is screwing things up is still a mystery to me. nsDeviceContext gets used in a lot of places and I guess one of them is making an invalid assumption about the relationship between device pixels and app units or CSS pixels.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.cpp#341

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I thought about this some more after our discussion on IRC. I'm fairly sure that the values flowing through Java (i.e. in the ImmutableViewportMetrics) should remain unchanged. The ImmutableViewportMetrics stores the page size in physical device pixels (pageRectXXX), the page size in CSS pixels (cssPageRectXXX), the viewport size and location in physical device pixels relative to the page (viewportRectXXX), and the zoom factor (conversion between physical device pixels and CSS pixels). None of these values are changed by your patch so these values should remain unchanged.

The values passed to gecko from browser.js (i.e. arguments to setDisplayPortForElement, setCriticalDisplayPortForElement, scrollTo, setScrollPositionClampingScrollPortSize, setCSSViewport) are also all in CSS pixels and therefore seem like they should be unchanged. The one I'm unsure about is the call to DOMWindowUtils.setResolution [1]; that may need to be adjusted. The documentation for that method [2] also refers to a setViewportScale function which I've never seen before and may be relevant.

Most of the layout code deals in app units, and the conversion between app units and CSS pixels hasn't changed, so the values flowing through there that bubble into browser.js (e.g. via getRootBounds, window.scrollY) should all also be unchanged.

All the above deals with "content" rather than "chrome". Since content can be scaled arbitrarily anyway, I'm fairly certain that no code relating to that makes invalid assumptions that would break here. However with chrome (e.g. XUL windows) that do not get scaled during normal operation, there might be such assumptions. For example, do the window.outer(Width|Height) values change with this patch? If so, then the Window:Resize handler at [3] may need to be updated so that gScreenWidth and gScreenHeight (viewport size in physical device pixels) remain unchanged.

The other bit that I'm not sure about is where the gfx code does conversions from app units to device pixels before passing them on to Java via the AndroidBridge::(setFirstPaintViewport|setPageRect) methods. The app unit values should be unchanged, and the values passed to Java should remain unchanged, but if something in that conversion was relying on values that have now changed (e.g. GetDefaultScale(), nsDeviceContext things) then that conversion will now be wrong, and it will be sending changed values to Java. I think I saw this happening in one of my builds but I didn't look too closely at the time. I think this is most likely area for tracking down this problem - looking at the values coming into Java from setFirstPaintViewport, seeing if they are different, and if they are, tracking down where the difference originates.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3281
[2] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#161
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5429

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 707708 [details] [diff] [review]
WIP

Clearing feedback flag for now.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

So with the 7 patches from bug 883646 applied, this patch results in sane visual behaviour on my Nexus 4, which uses a 2.0 widget scale. However, I haven't run it through try yet or tested whether things like CSS media queries are affected.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 764949 [details] [diff] [review]
WIP

Actually the touch event behaviour is still messed up. But still, I'd like to get your feedback on some aspects of this patch. Specifically, should the browser.viewport.scaleRatio pref be abolished entirely in favour of layout.css.devPixelsPerPx? Also, is there a way to avoid duplicating the getDefaultScale() widget code into getScaleRatio() in browser.js? I didn't see any way to basically get that value into browser.js and I think it is still needed there. Maybe not though; I don't know where all the CSS-viewport code lives now and whether more stuff can be ripped out of browser.js

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP v2

This one fixes up the issues with the touch events, I think.

Try build in progress at https://tbpl.mozilla.org/?tree=Try&rev=08771203bba7

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 769134 [details] [diff] [review]
WIP v2

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Specifically, should the browser.viewport.scaleRatio pref be abolished
> entirely in favour of layout.css.devPixelsPerPx?

Yes, I think so.

> Also, is there a way to avoid duplicating the getDefaultScale() widget
> code into getScaleRatio() in browser.js?

I think we can remove the JS code, and instead use window.devicePixelRatio or other JS/CSS APIs to get this value in browser.js.

::: widget/android/nsWindow.cpp
@@ +345,5 @@
> +    if (dpi < 200) { // includes desktop displays, LDPI, and MDPI Android devices
> +        return 1.0;
> +    }
> +    if (dpi < 300) { // includes Nokia N900, HDPI Android devices
> +        return 1.5;

I think we should get rid of these heuristics, and just return the Android system's "DisplayMetrics.density" value as suggested in comment 0, for simplicity and consistency with other Android apps.  That could be done in a separate bug if you want.

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> So with the 7 patches from bug 883646 applied, this patch results in sane
> visual behaviour on my Nexus 4, which uses a 2.0 widget scale. However, I
> haven't run it through try yet or tested whether things like CSS media
> queries are affected.

I did some brief tests on an HDPI device (1.5 device pixel per px).  The latest Try build is working fine for me, and now seems to return correct values for the resolution media query.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Matt Brubeck (:mbrubeck) [away until July 3] from comment #9)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> > Specifically, should the browser.viewport.scaleRatio pref be abolished
> > entirely in favour of layout.css.devPixelsPerPx?
> 
> Yes, I think so.

A look through MXR shows that b2g's prefs file and metro code still uses this. I can remove all the fennec uses of this pref though.

> > Also, is there a way to avoid duplicating the getDefaultScale() widget
> > code into getScaleRatio() in browser.js?
> 
> I think we can remove the JS code, and instead use window.devicePixelRatio
> or other JS/CSS APIs to get this value in browser.js.

For some reason when I tried window.devicePixelRatio earlier it didn't seem to work. Worked when I tried it just now though. I've updated the patch to use this and got rid of all the old scaleRatio stuff.

> I think we should get rid of these heuristics, and just return the Android
> system's "DisplayMetrics.density" value as suggested in comment 0, for
> simplicity and consistency with other Android apps.  That could be done in a
> separate bug if you want.

I'm fine with doing this, but would prefer a follow-up bug for it.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make Fennec use widget scaling properly

My push to try is hanging, will post the new try push URL here once it completes.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://tbpl.mozilla.org/?tree=Try&rev=dc281c2e3c59

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 770892 [details] [diff] [review]
Make Fennec use widget scaling properly

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

\o/  Thanks so much for doing this!

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa23d6bd3fa

I also filed bug 890253 for changing the GetDefaultScaleInternal implementation as discussed.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/efa23d6bd3fa