900111 - Fix CSSIntPoints and LayoutDeviceIntPoint usage in APZC Metro implementation

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

Description

[Brian R. Bondy [:bbondy]]

13:45:21] <Ms2ger> bbondy, http://hg.mozilla.org/mozilla-central/rev/6a8926881a44  has CSSIntPoints all over that aren't CSSIntPoints but LayoutDeviceIntPoint

Comment 1

[Brian R. Bondy [:bbondy]]

Kats is this what needs to be done here?

Convert these from device pixels to CSSIntPixels properly:
http://hg.mozilla.org/mozilla-central/rev/6a8926881a44#l2.216
http://hg.mozilla.org/mozilla-central/rev/6a8926881a44#l2.242

Convert these (and similar code) from CSSIntPoint pixels:
http://hg.mozilla.org/mozilla-central/rev/6a8926881a44#l2.336

If so, how do I do this conversion? Do I need to take into account the current scale or only the dpi value?

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

After talking to kats on irc and reading https://staktrace.com/spout/entry.php?id=800 over again i think this is correct. But please school me if not :)

Comment 3

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

Comment on attachment 783994 [details] [diff] [review]
Patch v1.

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

Looks ok, except for the rounding. A LayoutDeviceIntPoint stores x and y as integers, and a LayoutDevicePoint stores x and y as floats. So when your conversion function creates the LayoutDeviceIntPoint, it's going to be applying an implicit floor or something, I'm not really sure. Regardless the places that you're calling round() won't do anything because it will already be rounded. I suspect you want to move the rounding into the conversion function and keep the return value as LayoutDeviceIntPoint.

Also I'm assuming that LogToPhys returns something in LayoutDevice coordinates, but I don't really know what windows hands you and what the conversion is supposed to do so I can't confirm that.

Comment 4

[Brian R. Bondy [:bbondy]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 783994 [details] [diff] [review]
> Patch v1.
> 
> Review of attachment 783994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks ok, except for the rounding. A LayoutDeviceIntPoint stores x and y as
> integers, and a LayoutDevicePoint stores x and y as floats. So when your
> conversion function creates the LayoutDeviceIntPoint, it's going to be
> applying an implicit floor or something, I'm not really sure. Regardless the
> places that you're calling round() won't do anything because it will already
> be rounded. I suspect you want to move the rounding into the conversion
> function and keep the return value as LayoutDeviceIntPoint.
> 

Oops ya hence the name Int :)

> Also I'm assuming that LogToPhys returns something in LayoutDevice
> coordinates, but I don't really know what windows hands you and what the
> conversion is supposed to do so I can't confirm that.

Yes I think it's correct, it multiplies it by:

http://msdn.microsoft.com/en-us/library/windows/apps/windows.graphics.display.displayproperties.logicaldpi.aspx

which is described here:
http://msdn.microsoft.com/en-us/library/windows/apps/ff684173.aspx

It's also the same as wht we used to pass into the refPoints

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

This fixes the review comments, but I noticed a problem.  I'm using a normal 100% dpi machine, when I zoom in, the HandleSingleTap is offet too much.
For example when I go to my website and zoom in, I have a button bar at the top of the page

[   link 1  ] [ link 2 ] [ link 3] 
When I zoom in a lot and then tap on link 2, it actually loads link1. 

When I'm not zoomed I don't have that problem.

This was a problem before this patch too though, is it possible this is not something I'm doing wrong but a problem in APZC somewhere? I remember you mentioning something about a similar problem on Android before.

Comment 6

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

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> This was a problem before this patch too though, is it possible this is not
> something I'm doing wrong but a problem in APZC somewhere? I remember you
> mentioning something about a similar problem on Android before.

Link clicking while zoomed was working on Fennec and works in B2G, so I'm not sure if this is a problem in APZC. It might be, I just can't say without debugging.

Comment 7

[Brian R. Bondy [:bbondy]]

ok I guess we'll handle that in a different bug, thanks.

Comment 8

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/fx-team/rev/c4fcf8be2bd2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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