Closed Bug 900111 Opened 7 years ago Closed 7 years ago

Fix CSSIntPoints and LayoutDeviceIntPoint usage in APZC Metro implementation

Categories

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

25 Branch
x86_64
Windows 8
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [preview])

Attachments

(1 file, 1 obsolete file)

13:45:21] <Ms2ger> bbondy, http://hg.mozilla.org/mozilla-central/rev/6a8926881a44  has CSSIntPoints all over that aren't CSSIntPoints but LayoutDeviceIntPoint
Summary: Fix CSSIntPoints and LayoutDeviceIntPoint usage in APZC implementation → Fix CSSIntPoints and LayoutDeviceIntPoint usage in APZC Metro implementation
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?
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
Attached patch Patch v1. (obsolete) — Splinter Review
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 :)
Attachment #783994 - Flags: review?(bugmail.mozilla)
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.
Attachment #783994 - Flags: review?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Attached patch Patch v2Splinter Review
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.
Attachment #783994 - Attachment is obsolete: true
Attachment #784144 - Flags: review?(bugmail.mozilla)
Attachment #784144 - Flags: review?(bugmail.mozilla) → review+
(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.
ok I guess we'll handle that in a different bug, thanks.
https://hg.mozilla.org/integration/fx-team/rev/c4fcf8be2bd2
Target Milestone: --- → Firefox 25
Whiteboard: [preview]
https://hg.mozilla.org/mozilla-central/rev/c4fcf8be2bd2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 898841
You need to log in before you can comment on or make changes to this bug.