1125040 - Use LayoutDeviceIntPoint in more places

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[David Zbarsky (:dzbarsky)]

Comment on attachment 8553586 [details] [diff] [review]
Patch

I guessed on the reviewer here, feel free to bounce to someone else.

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fcc784b526c

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: layoutdeviceintpoint

Cleaned up the previous patch.

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: for nsLayoutUtils::GetEventCoordinatesRelativeTo and Touch::mRefPoint

Rebased to master and passing try: https://tbpl.mozilla.org/?tree=Try&rev=d1667e6790ac

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: GetEventPoint

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: WidgetToScreen

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8554192 [details] [diff] [review]
for nsLayoutUtils::GetEventCoordinatesRelativeTo and Touch::mRefPoint

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

LGTM, and thanks for spreading strongly-typed coordinates!

::: layout/base/nsLayoutUtils.h
@@ -674,5 @@
>     * @param aPoint the point to get the coordinates relative to
>     * @param aFrame the frame to make coordinates relative to
>     * @return the point, or (NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE) if
>     * for some reason the coordinates for the mouse are not known (e.g.,
> -   * the event is not a GUI event).

Stray line removal, please add back.

@@ +678,4 @@
>     */
>    static nsPoint GetEventCoordinatesRelativeTo(
>                     const mozilla::WidgetEvent* aEvent,
> +                   const mozilla::LayoutDeviceIntPoint aPoint,

Let's take |aPoint| by const-reference (for consistency with other functions that take points as arguments), since we're changing the signature anyways.

@@ +691,5 @@
>     * for some reason the coordinates for the mouse are not known (e.g.,
>     * the event is not a GUI event).
>     */
>    static nsPoint GetEventCoordinatesRelativeTo(nsIWidget* aWidget,
> +                                               const mozilla::LayoutDeviceIntPoint aPoint,

Likewise here (take by const-reference).

@@ +715,5 @@
>     * @return the point in the view's coordinates
>     */
>    static nsPoint TranslateWidgetToView(nsPresContext* aPresContext,
> +                                       nsIWidget* aWidget,
> +                                       mozilla::LayoutDeviceIntPoint aPt,

And here.

::: widget/android/nsWindow.cpp
@@ +1144,1 @@
>                  ae->Points()[0].y);

nit: adjust spacing so this continues lining up

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8554310 [details] [diff] [review]
GetEventPoint

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

::: layout/xul/nsResizerFrame.h
@@ +66,5 @@
>    static void RestoreOriginalSize(nsIContent* aContent);
>  
>  protected:
>  	nsIntRect mMouseDownRect;
> +  LayoutDeviceIntPoint mMouseDownPoint;

Indentation (or is it just bugzilla messing it up?)

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8554311 [details] [diff] [review]
WidgetToScreen

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

::: widget/nsIWidget.h
@@ +1637,5 @@
>       * @return screen coordinates stored in the x,y members
>       */
>  
> +    virtual mozilla::LayoutDeviceIntPoint WidgetToScreenOffset() = 0;
> +    virtual nsIntPoint WidgetToScreenOffsetUntyped() {

Please add a comment saying that this function is to help transition from untyped to typed coordinates, and will be removed once the transition is complete.

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to Botond Ballo [:botond] from comment #7)
> Comment on attachment 8554310 [details] [diff] [review]
> GetEventPoint
> 
> Review of attachment 8554310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/xul/nsResizerFrame.h
> @@ +66,5 @@
> >    static void RestoreOriginalSize(nsIContent* aContent);
> >  
> >  protected:
> >  	nsIntRect mMouseDownRect;
> > +  LayoutDeviceIntPoint mMouseDownPoint;
> 
> Indentation (or is it just bugzilla messing it up?)

There was a tab on the nsIntRect line.

Thanks for the reviews!  I'll send some more patches your way :)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c36a026244de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9dbdaf7ab14d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5167196c4b98

Comment 10

[David Zbarsky (:dzbarsky)]

Attachment: XULPopupManager

Comment 11

[David Zbarsky (:dzbarsky)]

Attachment: nsTitleBarFrame

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: FramesetFrame

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: IME

Comment 14

[Phil Ringnalda (:philor)]

Backed out the WidgetToScreen hunk in https://hg.mozilla.org/integration/mozilla-inbound/rev/5af5c02018a2 for not compiling on Linux, https://treeherder.mozilla.org/logviewer.html#?job_id=6147608&repo=mozilla-inbound

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c36a026244de
https://hg.mozilla.org/mozilla-central/rev/9dbdaf7ab14d

Comment 16

[David Zbarsky (:dzbarsky)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/52de461bce37
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c9c91426b3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e853e7c487
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/345796581183
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de5837c6e80b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52de461bce37
https://hg.mozilla.org/mozilla-central/rev/b4c9c91426b3
https://hg.mozilla.org/mozilla-central/rev/b3e853e7c487
https://hg.mozilla.org/mozilla-central/rev/345796581183
https://hg.mozilla.org/mozilla-central/rev/de5837c6e80b

Comment 18

[David Zbarsky (:dzbarsky)]

Attachment: GetChildProcessOffset

This is the last set of patches I have

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: Widget native events

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: GetScreenCoords

Comment 21

[David Zbarsky (:dzbarsky)]

Attachment: mMouseDownRect

Comment 22

[David Zbarsky (:dzbarsky)]

Attachment: GetScreenCoords

Comment 23

[David Zbarsky (:dzbarsky)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f451fd1326
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/157e840119de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b736b6fb4d1b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e0060a3e4ed8

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8563084 [details] [diff] [review]
GetScreenCoords

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

::: dom/events/Event.cpp
@@ +905,5 @@
>    LayoutDeviceIntPoint offset = aPoint + guiEvent->widget->WidgetToScreenOffset();
>    nscoord factor =
>      aPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
> +  return LayoutDeviceIntPoint(nsPresContext::AppUnitsToIntCSSPixels(offset.x * factor),
> +                              nsPresContext::AppUnitsToIntCSSPixels(offset.y * factor));

This looks suspicious to me...

::: dom/events/UIEvent.cpp
@@ +362,4 @@
>      Event::GetScreenCoords(mPresContext, mEvent, mEvent->refPoint);
>    nsresult rv = Event::DuplicatePrivateData();
>    if (NS_SUCCEEDED(rv)) {
> +    mEvent->refPoint = screenPoint;

ISTR that refPoint could be in several coordinate systems.

Comment 25

[David Zbarsky (:dzbarsky)]

Comment on attachment 8563084 [details] [diff] [review]
GetScreenCoords

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

::: dom/events/UIEvent.cpp
@@ +362,4 @@
>      Event::GetScreenCoords(mPresContext, mEvent, mEvent->refPoint);
>    nsresult rv = Event::DuplicatePrivateData();
>    if (NS_SUCCEEDED(rv)) {
> +    mEvent->refPoint = screenPoint;

From what I've seen, refPoint can sometimes be in Screen coordinates in addition to LayoutDevice coordinates.  This is pretty much the only place where it seems like it may be in CSS coordinates.

Also, the comment claims GetScreenPoint converts refPoint to the right coordinates, but the only GetScreenPoint I saw is in dom/events/WheelHandlingHelper.cpp, which just adds the child process offset.

So I'm not sure how this is supposed to work, but I'd rather avoid mucking aroundf too much and breaking everything

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f6f451fd1326
https://hg.mozilla.org/mozilla-central/rev/157e840119de
https://hg.mozilla.org/mozilla-central/rev/b736b6fb4d1b
https://hg.mozilla.org/mozilla-central/rev/e0060a3e4ed8

Comment 27

[Botond Ballo [:botond]]

(In reply to :Ms2ger from comment #24)
> Comment on attachment 8563084 [details] [diff] [review]
> GetScreenCoords
> 
> Review of attachment 8563084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/events/Event.cpp
> @@ +905,5 @@
> >    LayoutDeviceIntPoint offset = aPoint + guiEvent->widget->WidgetToScreenOffset();
> >    nscoord factor =
> >      aPresContext->DeviceContext()->AppUnitsPerDevPixelAtUnitFullZoom();
> > +  return LayoutDeviceIntPoint(nsPresContext::AppUnitsToIntCSSPixels(offset.x * factor),
> > +                              nsPresContext::AppUnitsToIntCSSPixels(offset.y * factor));
> 
> This looks suspicious to me...

Indeed, this is wrong. The type of Touch::mScreenPoint should be CSSIntPoint, not LayoutDeviceIntPoint. Apologies for missing this comment originally!

Comment 28

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #27)
> Indeed, this is wrong. The type of Touch::mScreenPoint should be
> CSSIntPoint, not LayoutDeviceIntPoint. Apologies for missing this comment
> originally!

Randall is touching this code in bug 1122804 (it was a discussion about that bug that that prompted me to look at this code in more detail), this will be fixed up there.