Last Comment Bug 891468 - Convert nsEventStateManager::GetChildProcessOffset to return a LayoutDeviceIntPoint instead of an nsIntPoint
: Convert nsEventStateManager::GetChildProcessOffset to return a LayoutDeviceIn...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: 25 Branch
: All All
: -- normal (vote)
: mozilla25
Assigned To: Botond Ballo [:botond]
:
:
Mentors:
Depends on: 865735
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-09 11:33 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2013-07-12 14:42 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.47 KB, patch)
2013-07-09 13:23 PDT, Botond Ballo [:botond]
bugmail: feedback+
Details | Diff | Splinter Review
updated patch (8.47 KB, patch)
2013-07-09 13:42 PDT, Botond Ballo [:botond]
no flags Details | Diff | Splinter Review
updated patch (8.51 KB, patch)
2013-07-09 14:19 PDT, Botond Ballo [:botond]
bugs: review+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2013-07-09 11:33:46 PDT
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.
Comment 1 Botond Ballo [:botond] 2013-07-09 13:23:33 PDT
Created attachment 772841 [details] [diff] [review]
patch
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-09 13:30:08 PDT
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".
Comment 3 Botond Ballo [:botond] 2013-07-09 13:42:47 PDT
Created attachment 772854 [details] [diff] [review]
updated patch
Comment 4 Botond Ballo [:botond] 2013-07-09 13:44:27 PDT
> ::: 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,
Comment 5 Botond Ballo [:botond] 2013-07-09 14:19:04 PDT
Created attachment 772879 [details] [diff] [review]
updated patch

Updated patch to fix a compiler error.
Comment 6 Botond Ballo [:botond] 2013-07-10 14:42:32 PDT
Try results: https://tbpl.mozilla.org/?tree=Try&rev=4264d54cc63e
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-07-11 07:46:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14404dc5c68
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-07-11 19:07:45 PDT
https://hg.mozilla.org/mozilla-central/rev/a14404dc5c68

Note You need to log in before you can comment on or make changes to this bug.