Convert nsEventStateManager::GetChildProcessOffset to return a LayoutDeviceIntPoint instead of an nsIntPoint

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Events
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: botond)

Tracking

25 Branch
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

nsEventStateManager::GetChildProcessOffset returns a generic nsIntPoint that is really in the LayoutDevice coordinate space. The nsIntPoint should be changed to a LayoutDeviceIntPoint and propagated as much as is reasonable.

As part of this, we'll probably want to add a helper function to the LayoutDevicePixel struct in layout/base/Units.h with the signature "static LayoutDeviceIntPoint FromAppUnitsToNearest(const nsPoint& aPoint, int32_t appUnitsPerDevPixel)" and use it when converting from app units to the LayoutDevice coordinate space.

It probably also makes sense to convert the first argument to nsEventStateManager::MapEventCoordinatesForChildProcess in this bug as well, but leave nsEvent::refPoint as an nsIntPoint for now.
Version: 24 Branch → 25 Branch
(Assignee)

Comment 1

4 years ago
Created attachment 772841 [details] [diff] [review]
patch
Comment on attachment 772841 [details] [diff] [review]
patch

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

Looks fine to me, requesting smaug to review this since I'm not actually a peer for this code.

::: content/events/src/nsEventStateManager.cpp
@@ +1539,5 @@
>  
>    // Find out how far we're offset from the nearest widget.
>    nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(&aEvent,
>                                                              targetFrame);
> +  return LayoutDevicePixel::FromAppUnitsToNearest(pt, presContext->AppUnitsPerDevPixel());

This works fine but for consistency with other pieces of similar code I've written, I would use "LayoutDeviceIntPoint::FromAppUnitsToNearest" here instead of "LayoutDevicePixel::FromAppUnitsToNearest".
Attachment #772841 - Flags: review?(bugs)
Attachment #772841 - Flags: feedback+
(Assignee)

Comment 3

4 years ago
Created attachment 772854 [details] [diff] [review]
updated patch
Attachment #772841 - Attachment is obsolete: true
Attachment #772841 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #772854 - Flags: review?(bugs)
(Assignee)

Comment 4

4 years ago
> ::: content/events/src/nsEventStateManager.cpp
> @@ +1539,5 @@
> >  
> >    // Find out how far we're offset from the nearest widget.
> >    nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(&aEvent,
> >                                                              targetFrame);
> > +  return LayoutDevicePixel::FromAppUnitsToNearest(pt, presContext->AppUnitsPerDevPixel());
> 
> This works fine but for consistency with other pieces of similar code I've
> written, I would use "LayoutDeviceIntPoint::FromAppUnitsToNearest" here
> instead of "LayoutDevicePixel::FromAppUnitsToNearest".

Changed in updated patch,
(Assignee)

Comment 5

4 years ago
Created attachment 772879 [details] [diff] [review]
updated patch

Updated patch to fix a compiler error.
Attachment #772854 - Attachment is obsolete: true
Attachment #772854 - Flags: review?(bugs)
Attachment #772879 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #772879 - Flags: review? → review?(bugs)

Updated

4 years ago
Attachment #772879 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

4 years ago
Try results: https://tbpl.mozilla.org/?tree=Try&rev=4264d54cc63e
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14404dc5c68
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a14404dc5c68
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.