874295 - mozmm should reflect actual device resolution of B2G phones

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

0. If you are looking at a Geeksphone, make sure it does not run the stocked version but the open source build.
1. Go to an website with mozmm, like http://jsfiddle.net/timdream/ewJZZ/ (the little signal button besides [Run] will give you a short URL for mobile)
2. Find a physical metre ruler, measure the 10mozmm bar, or, if you live in the US,
2'. Find a physical inch ruler, measure the 25.4mozmm bar

Expected:

1. It should comes with physical size of 1 cm
1'. It should comes with physical size of 1 inch

Actual:

It doesn't.

Note:

My understand is that this is a similar problem like bug 845182, where in Gonk failed to let Gecko figure out the value of |layout.css.dpi| on its own.

Note 2:

I didn't actually test this; this is reported on the mailing list
https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.b2g/wsemoryVQuM

Note 3:

:roc, as promised, here is a needinfo? for you :)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For this test to work, PuppetWidget::GetDPI must return the true value, and there must be no zooming. Testing  this in an app would help, since there's no browser zooming there.

Tim, if you find me we can debug this together if you like :-).

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Test case

OK, I was too naive with jsfiddle ... here is a standalone test case w/ <viewport>.

Comment 3

[Michael Wu [:mwu]]

The Peak should have everything configured to report dpi correctly on the Gonk level. On other devices, the default is 160 dpi on QC devices if it isn't configured, which it often is not.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hum, the length of mozmm is correct on Unagi.

Gonna try with a device-px-ratio > 1 setting before closing this bug as INVALID.

Comment 5

[Michael Wu [:mwu]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #2)
> Created attachment 751966 [details]
> Test case
> 
> OK, I was too naive with jsfiddle ... here is a standalone test case w/
> <viewport>.

This test passes on my Peak. The kernel that was shipped with the Peak doesn't report the right DPI, but building from source will get you a kernel with correct screen size/dpi reporting.

Comment 6

[Michael Wu [:mwu]]

Note that these devices are not currently expected to be configured correctly: Keon, Buri, Inari/Ikura, Unagi.

However, since they're HVGA and ~3.5".. the DPI just happens to be pretty close to the default fallback of 160 dpi. We can try to get these devices closer if we want though.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Galaxy S2 screenshot

We fail on SGS2.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't have any information. Come by and we can debug it :-)

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #2)
> Created attachment 751966 [details]
> Test case

The test case now comes with short url, http://bit.ly/bug874295test

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Update:

Galaxy S2 is not really a HIDPI devices according to math in bug 845182. Gaia interfaces was being scale up NOT by |layout.css.devPixelsPerPx|, but with responsive.css, which is due to remove in bug 870318.

The webpage as screenshot shown, within the browser app was being scale up by 1.5x; we somehow decided that's the default scale on this phone. (I cannot link where the code is because it was :roc who show me no his screen :-/).

As the apps will not be set with mozasyncpanzoom, the page will not be scale up there.

Fortunately, if I manually change the calculation in bug 845182 and make SGS2 a ratio=1.5 device (attempt to overwrite it from Gaia with user.js failed with unknown reason), the page looks correct on both browser tabs and apps.

The next thing we should do would be make sure the code :roc mentioned and the numbers in GetDefaultScaleInternal() is in sync, presumably concentrate the calculation.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #10)
> Galaxy S2 is not really a HIDPI devices according to math in bug 845182.
> Gaia interfaces was being scale up NOT by |layout.css.devPixelsPerPx|, but
> with responsive.css, which is due to remove in bug 870318.

BTW, this also means I am not good at math...

I'll ping :roc later to find out where is the calculate on viewport default scale. He is in a meeting.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The code in question is

https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#5079

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Tentative patch

This patch copy and paste the calculation from 

nsContentUtils::GetDevicePixelsPerMetaViewportPixel()

to

nsWindow::GetDefaultScaleInternal()

so they would be sync. I've verify this would work on SGS2.

I tried to do |return nsContentUtils::GetDevicePixelsPerMetaViewportPixel(gFocusedWindow)| but got segfault, presumably because |gFocusedWindow| is still null at the time GetDefaultScaleInternal() is called.

By doing so, we kind of killed two birds with one stone: we would make sure FxOS classify device DPIs with the same way AOSP does, and we would get the length of |mozmm| right within <iframe mozasyncpanzoom> frame.

Feedback anyone? This is me writing a C++ patch without actually writing anything.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 753092 [details] [diff] [review]
Tentative patch

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

I don't really have an opinion on this patch.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Matt, could we make GetDevicePixelsPerMetaViewportPixel call nsIWidget::GetDefaultScale, and move the logic that's currently there into the Android nsWindow? I think we should...

Comment 16

[Michael Wu [:mwu]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> Created attachment 751992 [details]
> Galaxy S2 screenshot
> 
> We fail on SGS2.

SGS2 is probably not configured correctly then. It requires proper per-device configuration and some devices aren't configured. We'll be requiring proper configuration for any non-HVGA FFOS device.

Comment 17

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Matt, could we make GetDevicePixelsPerMetaViewportPixel call
> nsIWidget::GetDefaultScale, and move the logic that's currently there into
> the Android nsWindow? I think we should...

Yes, we should do that, and I even put a patch on bug 803207 that basically does it, but it caused bugs in the Android UI that we haven't had yet figured out.  See the comments there for details.  But we should really get back to that because it's also the right way to fix bug 794056 and bug 840916.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about we call GetDefaultScale instead of GetDevicePixelsPerMetaViewportPixel, #ifdefed for non-Android, to fix this bug for B2G while you work on the Android issues?

Comment 19

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> How about we call GetDefaultScale instead of
> GetDevicePixelsPerMetaViewportPixel, #ifdefed for non-Android, to fix this
> bug for B2G while you work on the Android issues?

Sounds good to me.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Tim, it would be helpful if you could test this on your device.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Created attachment 754085 [details] [diff] [review]
> fix
> 
> Tim, it would be helpful if you could test this on your device.

I can verify this fixed the bug on SGS2, whether or not GetDefaultScaleInternal() return 1 or 1.5.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e2bfbb427d

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/441d2b9cecb4

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This got backed out due to test failures :-(

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Failing tests:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23575597&tree=Mozilla-Inbound&full=1

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

3:17:03 INFO - 42135 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport1.html | initial zoom is 150% - got 1, expected 1.5
23:17:03 INFO - 42136 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport1.html | width equals displayWidth/1.5 - got 900, expected 600
23:17:03 INFO - 42137 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport1.html | height equals displayHeight/1.5 - got 600, expected 400
23:17:03 INFO - 42147 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport2.html | initial zoom is 150% - got 1, expected 1.5
23:17:03 INFO - 42148 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport2.html | width equals displayWidth/1.5 - got 900, expected 600
23:17:03 INFO - 42149 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport2.html | height equals displayHeight/1.5 - got 600, expected 400
23:17:03 INFO - 42173 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport4.html | initial zoom is adjusted for device pixel ratio - got 1, expected 1.5
23:17:03 INFO - 42174 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport4.html | width fits the initial zoom - got 800, expected 533
23:17:03 INFO - 42175 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport4.html | height fits the initial zoom - got 480, expected 320
23:17:03 INFO - 42192 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport6.html | minumum scale is converted to device pixel scale - got 0.75, expected 1.125
23:17:03 INFO - 42193 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport6.html | initial scale is bounded by the minimum scale - got 0.75, expected 1.125
23:17:03 INFO - 42194 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_meta_viewport6.html | maximum scale defaults to the absolute maximum - got 10, expected 15

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This test fails because it sets browser.viewport.scaleRatio and expects that to to do something. Of course, after this patch, it doesn't. On non-Android the test needs to set layout.css.devPixelsPerPx instead.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: Fix reftests

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

forget the patch summary there :-)

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d19f2381c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a995e362a3f

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53d19f2381c4
https://hg.mozilla.org/mozilla-central/rev/1a995e362a3f

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/44179aebdbe1
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/db2bceea18cf

Comment 33

[Al Tsai [:atsai]]

[2013/10/21 Helix Testing]
Gaia:     c829a2042594b6c3a4899ee27979799a0f301534
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/f7c657f6d019
BuildID   20131015042201
Version   18.0

Comment 34

[pancake]

This bug has appeared again in current git (2 weeks ago or so). It works fine on fxos 1.2 and 1.3, but in 1.4 the mozmm is broken again.

You can verify this by installing the Ruler app from the marketplace.

 https://marketplace.firefox.com/app/ruler-1

Here's a tweet about that:

 https://twitter.com/trufae/status/415424762530508800

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please file a new bug about that and mark it blocking1.4?

Comment 36

[pancake]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> Please file a new bug about that and mark it blocking1.4?

Already did that yesterday

https://bugzilla.mozilla.org/show_bug.cgi?id=956665