The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla25

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mbrubeck, Assigned: kats)

Tracking

(Depends on: 1 bug)

18 Branch
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.)
(Reporter)

Comment 1

5 years ago
I think this would also let us remove the special-case workaround for mobile that I added in bug 779527.
(Reporter)

Comment 2

4 years ago
Created attachment 707708 [details] [diff] [review]
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?
Attachment #707708 - Flags: feedback?(bugmail.mozilla)
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
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 on attachment 707708 [details] [diff] [review]
WIP

Clearing feedback flag for now.
Attachment #707708 - Flags: feedback?(bugmail.mozilla)
(Reporter)

Updated

4 years ago
Blocks: 840916
(Reporter)

Updated

4 years ago
Blocks: 874295
Depends on: 883646
Created attachment 764949 [details] [diff] [review]
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.
Attachment #707708 - Attachment is obsolete: true
Attachment #764949 - Flags: feedback?(mbrubeck)
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
Created attachment 769134 [details] [diff] [review]
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
Attachment #764949 - Attachment is obsolete: true
Attachment #764949 - Flags: feedback?(mbrubeck)
Attachment #769134 - Flags: feedback?(mbrubeck)
(Reporter)

Comment 9

4 years ago
Comment on attachment 769134 [details] [diff] [review]
WIP v2

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #769134 - Flags: feedback?(mbrubeck) → feedback+
(Reporter)

Comment 10

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Matt Brubeck (:mbrubeck) [away until July 3] from comment #9)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Created attachment 770892 [details] [diff] [review]
Make Fennec use widget scaling properly

My push to try is hanging, will post the new try push URL here once it completes.
Attachment #769134 - Attachment is obsolete: true
Attachment #770892 - Flags: review?(mbrubeck)
Assignee: nobody → bugmail.mozilla
https://tbpl.mozilla.org/?tree=Try&rev=dc281c2e3c59
(Reporter)

Comment 14

4 years ago
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!
Attachment #770892 - Flags: review?(mbrubeck) → review+
Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa23d6bd3fa

I also filed bug 890253 for changing the GetDefaultScaleInternal implementation as discussed.
https://hg.mozilla.org/mozilla-central/rev/efa23d6bd3fa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 891336

Updated

4 years ago
Depends on: 891175
Depends on: 892267
Duplicate of this bug: 794057
Blocks: 745871
Depends on: 902888

Updated

4 years ago
Depends on: 910098
Depends on: 903327
(Reporter)

Updated

4 years ago
Depends on: 911695
Depends on: 924791
Depends on: 900592
Depends on: 985188
You need to log in before you can comment on or make changes to this bug.