1020246 - Add support for HiDPI devices

Status: RESOLVED FIXED

(Testing Graveyard :: Eideticker, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

It would appear that the interactions are offset when running against the Flame device. I'm unable to launch the Contacts or Messages applications from their dock icons, and instead of Music, the Marketplace app is launched.

Interestingly, the Camera app is successfully launched. This icon is located in the first row/column on the homescreen.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

It seems that if I multiple the co-ordinates by 1.5 then it works. I think this is due to the Flame being HDPI, so we probably need to add this as an attribute to the device properties and perform the multiplication before sending the events to Orangutan. Alternatively, I suppose we could enhance Orangutan?

Comment 2

[William Lachance (:wlach)]

As discussed on irc, I'm pretty sure the pixel values being returned by marionette are downscaled by the hidpi factor, which is causing this problem. I think adding an attribute to device properties is the way to go in the short term, so we can get something working. Longer term, we should be able to somehow detect the scaling factor programmatically.

Jonathan, do you know who we might ask about how to properly detect hidpi scaling inside gecko/gaia?

Comment 3

[Jonathan Griffin (:jgriffin)]

Dave, do you know how we can programmatically detect the hidpi scaling factor that's used to transparently enlarge the viewport for the Flame?

Comment 4

[Dave Hylands [:dhylands]]

I don't, but mwwu probably does.

Comment 5

[Michael Wu [:mwu]]

nsWindow::GetDefaultScale will get you that value. From JS, you might want to look for unscaledDevicePixelsPerCSSPixel on the window.

Comment 6

[William Lachance (:wlach)]

I'll take this one, starting off with just a manual setting for the pixel scaling factor, then using marionette to get it dynamically if possible.

Dave, is the above something we could integrate with gaiatest? I couldn't figure out how to get this value using marionette in my testing, but I didn't try very hard and am less familiar with this stuff than you.

Comment 7

[Jonathan Griffin (:jgriffin)]

Theoretically it looks like you could do something like:

scaling_factor = m.execute_script("return window.wrappedJSObject.unscaledDevicePixelsPerCSSPixel")

...but, I haven't tried this, and we don't use this attribute anywhere in m-c.

Comment 8

[William Lachance (:wlach)]

(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> Theoretically it looks like you could do something like:
> 
> scaling_factor = m.execute_script("return
> window.wrappedJSObject.unscaledDevicePixelsPerCSSPixel")
> 
> ...but, I haven't tried this, and we don't use this attribute anywhere in
> m-c.


This doesn't work. :/ Do I need to do something more than:

m = marionette.Marionette()
m.start_session()
scaling_factor = m.execute_script("return window.wrappedJSObject.unscaledDevicePixelsPerCSSPixel")

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> Theoretically it looks like you could do something like:
> 
> scaling_factor = m.execute_script("return
> window.wrappedJSObject.unscaledDevicePixelsPerCSSPixel")
> 
> ...but, I haven't tried this, and we don't use this attribute anywhere in
> m-c.

That doesn't return anything for me. I tried in both chrome and content contexts, as well as from an app frame.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Perhaps Jonathan Kew can help, I see he worked on bug 794038 that introduced this, although it was specifically for OS X and not B2G.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Nevermind, this works:

m.execute_script('return devicePixelRatio')

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Here's a full working example:

from marionette import Marionette
m = Marionette()
m.start_session()
ratio = m.execute_script('return window.wrappedJSObject.devicePixelRatio;')

On Flame it returns 1.5, on Hamachi it returns 1.

Comment 13

[William Lachance (:wlach)]

Attachment: Use hidpi scaling factor

I believe this should work even on versions of FirefoxOS where the devicePixelRatio property is undefined.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8435319 [details] [diff] [review]
Use hidpi scaling factor

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

r- because I have an outstanding question. You're much more familiar with this part of the codebase than I am, so I might be oversimplifying it.

::: src/eideticker/eideticker/device.py
@@ +244,5 @@
> +                      int(coords[0]))
> +
> +        if not prescaled:
> +            coords = (int(float(coords[0]) * self.devicePixelRatio),
> +                      int(float(coords[1]) * self.devicePixelRatio))

Are there cases where devicePixelRatio would be > 1 but prescaled would be False? I guess I was expecting us to just multiply all coordinates by devicePixelRatio. If we want to avoid the unnecessary calculation then we could do this when devicePixelRation != 1.

@@ +556,5 @@
>          self.startB2G()
>  
> +    @property
> +    def devicePixelRatio(self):
> +        if not self._dpRatio:

Could we call this _devicePixelRatio for consistency and clarity?

Comment 15

[William Lachance (:wlach)]

(In reply to Dave Hunt (:davehunt) from comment #14)
> Comment on attachment 8435319 [details] [diff] [review]
> Use hidpi scaling factor
> 
> Review of attachment 8435319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because I have an outstanding question. You're much more familiar with
> this part of the codebase than I am, so I might be oversimplifying it.
> 
> ::: src/eideticker/eideticker/device.py
> @@ +244,5 @@
> > +                      int(coords[0]))
> > +
> > +        if not prescaled:
> > +            coords = (int(float(coords[0]) * self.devicePixelRatio),
> > +                      int(float(coords[1]) * self.devicePixelRatio))
> 
> Are there cases where devicePixelRatio would be > 1 but prescaled would be
> False? I guess I was expecting us to just multiply all coordinates by
> devicePixelRatio. If we want to avoid the unnecessary calculation then we
> could do this when devicePixelRation != 1.

Yes, sometimes we perform actions (e.g. scroll_down) on Android which are calculated based on all the screen pixels, not the scaled pixels we get from gecko. So we do need something like this to get the right behavior there. If that makes sense, could I get an r+? :)

> @@ +556,5 @@
> >          self.startB2G()
> >  
> > +    @property
> > +    def devicePixelRatio(self):
> > +        if not self._dpRatio:
> 
> Could we call this _devicePixelRatio for consistency and clarity?

Yup, np.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8435319 [details] [diff] [review]
Use hidpi scaling factor

It only vaguely makes sense to me so far, but I don't want to block this on my limited capacity to understand it. :)

Comment 17

[William Lachance (:wlach)]

Pushed with suggested renaming:

https://github.com/mozilla/eideticker/commit/8733ca1240e056e71cc3fb5d826a4d8c6f27816f