Closed Bug 1125040 Opened 5 years ago Closed 5 years ago

Use LayoutDeviceIntPoint in more places

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(11 files, 3 obsolete files)

37.10 KB, patch
botond
: review+
Details | Diff | Splinter Review
7.61 KB, patch
botond
: review+
Details | Diff | Splinter Review
64.04 KB, patch
botond
: review+
Details | Diff | Splinter Review
4.66 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.13 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.00 KB, patch
botond
: review+
Details | Diff | Splinter Review
IME
29.35 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.92 KB, patch
botond
: review+
Details | Diff | Splinter Review
27.04 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.37 KB, patch
botond
: review+
Details | Diff | Splinter Review
10.59 KB, patch
botond
: review+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — Splinter Review
No description provided.
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
Attachment #8553586 - Attachment is patch: true
Attachment #8553586 - Attachment mime type: message/rfc822 → text/plain
Attachment #8553586 - Flags: review?(botond)
Attachment #8553586 - Flags: review?(botond)
Attached patch layoutdeviceintpoint (obsolete) — Splinter Review
Cleaned up the previous patch.
Assignee: nobody → dzbarsky
Attachment #8553586 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8553998 - Flags: review?(botond)
Summary: Use LayoutDeviceIntPoint for nsLayoutUtils::GetEventCoordinatesRelativeTo and Point::mRefPoint → Use LayoutDeviceIntPoint for nsLayoutUtils::GetEventCoordinatesRelativeTo and Touch::mRefPoint
Rebased to master and passing try: https://tbpl.mozilla.org/?tree=Try&rev=d1667e6790ac
Attachment #8553998 - Attachment is obsolete: true
Attachment #8553998 - Flags: review?(botond)
Attachment #8554192 - Flags: review?(botond)
Summary: Use LayoutDeviceIntPoint for nsLayoutUtils::GetEventCoordinatesRelativeTo and Touch::mRefPoint → Use LayoutDeviceIntPoint in more places
Attachment #8554192 - Attachment description: Patch → for nsLayoutUtils::GetEventCoordinatesRelativeTo and Touch::mRefPoint
Attached patch GetEventPointSplinter Review
Attachment #8554310 - Flags: review?(botond)
Attached patch WidgetToScreenSplinter Review
Attachment #8554311 - Flags: review?(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
Attachment #8554192 - Flags: review?(botond) → review+
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?)
Attachment #8554310 - Flags: review?(botond) → review+
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.
Attachment #8554311 - Flags: review?(botond) → review+
(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
Whiteboard: [leave open]
Attached patch XULPopupManagerSplinter Review
Attachment #8557657 - Flags: review?(botond)
Attached patch nsTitleBarFrameSplinter Review
Attachment #8557658 - Flags: review?(botond)
Attached patch FramesetFrameSplinter Review
Attachment #8557659 - Flags: review?(botond)
Attached patch IMESplinter Review
Attachment #8557660 - Flags: review?(botond)
Attachment #8557657 - Flags: review?(botond) → review+
Attachment #8557658 - Flags: review?(botond) → review+
Attachment #8557659 - Flags: review?(botond) → review+
Attachment #8557660 - Flags: review?(botond) → review+
This is the last set of patches I have
Attachment #8563040 - Flags: review?(botond)
Attachment #8563042 - Flags: review?(botond)
Attached patch GetScreenCoords (obsolete) — Splinter Review
Attachment #8563043 - Flags: review?(botond)
Attached patch mMouseDownRectSplinter Review
Attachment #8563044 - Flags: review?(botond)
Attached patch GetScreenCoordsSplinter Review
Attachment #8563043 - Attachment is obsolete: true
Attachment #8563043 - Flags: review?(botond)
Attachment #8563084 - Flags: review?(botond)
Attachment #8563040 - Flags: review?(botond) → review+
Attachment #8563044 - Flags: review?(botond) → review+
Attachment #8563084 - Flags: review?(botond) → review+
Attachment #8563042 - Flags: review?(botond) → review+
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 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
(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!
(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.
Blocks: 1122804
You need to log in before you can comment on or make changes to this bug.