Make nsEvent::{refPoint, lastRefPoint} LayoutDeviceIntPoints

RESOLVED FIXED in mozilla25

Status

()

Core
Event Handling
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Depends on: 1 bug)

Trunk
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 778939 [details] [diff] [review]
refPoint

This adds LayoutDeviceIntPoint::{To,From}Untyped functions to limit the size of the patch; I've got a some patches to clean up some of those.
Attachment #778939 - Flags: review?(bugs)
Attachment #778939 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

5 years ago
Blocks: 896259
Comment on attachment 778939 [details] [diff] [review]
refPoint

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

Thanks for doing this! I'm a little concerned that now we might be typing things as LayoutDevicePixel when they're not actually (e.g. in AsyncPanZoomController) but I think that might be the only case where that happens so hopefully it won't be a problem. Also I really hope WidgetToScreenOffset() is next on the chopping block as it will help a lot with removing the type conversions.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +282,5 @@
>      break;
>    }
>    default: {
> +    // NOTE: refPoint is in a different coordinate system in subprocessed on
> +    // B2G. The following code is therefore completely bogus.

s/subprocessed/subprocesses/?

::: layout/base/Units.h
@@ +129,5 @@
>  struct LayoutDevicePixel {
> +  static LayoutDeviceIntPoint FromUntyped(const nsIntPoint& aPoint)
> +  {
> +    return LayoutDeviceIntPoint(aPoint.x, aPoint.y);
> +  }

Please stick to the existing style in this file. Opening brace should be on the tail of the declaration, and leave one blank line between functions.
Attachment #778939 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Please stick to the existing style in this file. Opening brace should be on
> the tail of the declaration, and leave one blank line between functions.
Hrm, why does such a new file not use Mozilla coding style, but some random style?
Comment on attachment 778939 [details] [diff] [review]
refPoint

>   nsGUIEvent* guiEvent = static_cast<nsGUIEvent*>(aEvent);
>   if (!guiEvent->widget) {
>-    return aPoint;
>+    return LayoutDeviceIntPoint::ToUntyped(aPoint);
All these ToUntyped are just super ugly. I guess they are good hints what all needs to be fixed

Why you can't just change those various methods to return LayoutDeviceIntPoint? Perhaps a followup?

>-nsIntPoint nsEventStateManager::sLastRefPoint = kInvalidRefPoint;
>-nsIntPoint nsEventStateManager::sLastScreenPoint = nsIntPoint(0,0);
>-nsIntPoint nsEventStateManager::sSynthCenteringPoint = kInvalidRefPoint;
>+LayoutDeviceIntPoint nsEventStateManager::sLastRefPoint = kInvalidRefPoint;
>+nsIntPoint nsEventStateManager::sLastScreenPoint = nsIntPoint(0, 0);
>+LayoutDeviceIntPoint nsEventStateManager::sSynthCenteringPoint = kInvalidRefPoint;
sLastScreenPoint needs some comment. Why it is nsIntPoint but others are LayoutDeviceIntPoint

>-static nsIntPoint
>-ToWidgetPoint(float aX, float aY, const nsPoint& aOffset,
>+static LayoutDeviceIntPoint
>+ToWidgetPoint(const CSSPoint& aPoint, const nsPoint& aOffset,
>               nsPresContext* aPresContext)
> {
>-  double appPerDev = aPresContext->AppUnitsPerDevPixel();
>-  nscoord appPerCSS = nsPresContext::AppUnitsPerCSSPixel();
>-  return nsIntPoint(NSToIntRound((aX*appPerCSS + aOffset.x)/appPerDev),
>-                    NSToIntRound((aY*appPerCSS + aOffset.y)/appPerDev));
>+  return LayoutDeviceIntPoint::FromAppUnits(
>+    CSSPoint::ToAppUnits(aPoint) + aOffset,
>+    aPresContext->AppUnitsPerDevPixel());
> }
Huh. x/y and offset are in different coordinate space...

>+++ b/widget/gtk2/nsGtkKeyUtils.cpp
>@@ -544,17 +544,17 @@ KeymapWrapper::OnKeysChanged(GdkKeymap *
>                "This instance must be the singleton instance");
> 
>     // We cannot reintialize here becasue we don't have GdkWindow which is using
>     // the GdkKeymap.  We'll reinitialize it when next GetInstance() is called.
>     sInstance->mInitialized = false;
> 
>     // Reset the bidi keyboard settings for the new GdkKeymap
>     if (!sBidiKeyboard) {
>-        nsresult rv = CallGetService("@mozilla.org/widget/bidikeyboard;1", &sBidiKeyboard);
>+        CallGetService("@mozilla.org/widget/bidikeyboard;1", &sBidiKeyboard);
Totally unrelated change, but looks ok.
Attachment #778939 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #2)
> Hrm, why does such a new file not use Mozilla coding style, but some random
> style?

Oh, whoops. I guess I haven't looked at the Mozilla coding style doc in a while. If you want to update the style throughout the file then go ahead.
(Assignee)

Comment 5

4 years ago
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 778939 [details] [diff] [review]
> refPoint
> 
> >   nsGUIEvent* guiEvent = static_cast<nsGUIEvent*>(aEvent);
> >   if (!guiEvent->widget) {
> >-    return aPoint;
> >+    return LayoutDeviceIntPoint::ToUntyped(aPoint);
> All these ToUntyped are just super ugly. I guess they are good hints what
> all needs to be fixed

Yeah, and I'd say it's better than doing "nsIntPoint(aPoint.x, aPoint.y)" everywhere, especially if the argument is more complex than just a variable.

> Why you can't just change those various methods to return
> LayoutDeviceIntPoint? Perhaps a followup?

I think you'll appreciate that I didn't fix the whole tree in one patch :)

Bug 896259 is the first followup to get rid of some of those; I'm sure more will follow.

> >-nsIntPoint nsEventStateManager::sLastRefPoint = kInvalidRefPoint;
> >-nsIntPoint nsEventStateManager::sLastScreenPoint = nsIntPoint(0,0);
> >-nsIntPoint nsEventStateManager::sSynthCenteringPoint = kInvalidRefPoint;
> >+LayoutDeviceIntPoint nsEventStateManager::sLastRefPoint = kInvalidRefPoint;
> >+nsIntPoint nsEventStateManager::sLastScreenPoint = nsIntPoint(0, 0);
> >+LayoutDeviceIntPoint nsEventStateManager::sSynthCenteringPoint = kInvalidRefPoint;
> sLastScreenPoint needs some comment. Why it is nsIntPoint but others are
> LayoutDeviceIntPoint

Because nsDOMEvent::GetScreenCoords / nsDOMUIEvent::CalculateScreenPoint / nsEventStateManager::sLastScreenPoint share a coordinate space that's almost but not quite LayoutDevicePixels, and that's used nowhere else (as far as I know).

I'll add a comment.

> >-static nsIntPoint
> >-ToWidgetPoint(float aX, float aY, const nsPoint& aOffset,
> >+static LayoutDeviceIntPoint
> >+ToWidgetPoint(const CSSPoint& aPoint, const nsPoint& aOffset,
> >               nsPresContext* aPresContext)
> > {
> >-  double appPerDev = aPresContext->AppUnitsPerDevPixel();
> >-  nscoord appPerCSS = nsPresContext::AppUnitsPerCSSPixel();
> >-  return nsIntPoint(NSToIntRound((aX*appPerCSS + aOffset.x)/appPerDev),
> >-                    NSToIntRound((aY*appPerCSS + aOffset.y)/appPerDev));
> >+  return LayoutDeviceIntPoint::FromAppUnits(
> >+    CSSPoint::ToAppUnits(aPoint) + aOffset,
> >+    aPresContext->AppUnitsPerDevPixel());
> > }
> Huh. x/y and offset are in different coordinate space...

Strange, yes, but at least we can statically check that now. (Well, sort of, all the arguments are just CSSPoint(x, y), but at least it's more obvious to the caller.)

> >+++ b/widget/gtk2/nsGtkKeyUtils.cpp
> >@@ -544,17 +544,17 @@ KeymapWrapper::OnKeysChanged(GdkKeymap *
> >                "This instance must be the singleton instance");
> > 
> >     // We cannot reintialize here becasue we don't have GdkWindow which is using
> >     // the GdkKeymap.  We'll reinitialize it when next GetInstance() is called.
> >     sInstance->mInitialized = false;
> > 
> >     // Reset the bidi keyboard settings for the new GdkKeymap
> >     if (!sBidiKeyboard) {
> >-        nsresult rv = CallGetService("@mozilla.org/widget/bidikeyboard;1", &sBidiKeyboard);
> >+        CallGetService("@mozilla.org/widget/bidikeyboard;1", &sBidiKeyboard);
> Totally unrelated change, but looks ok.

(I think dholbert already fixed this, in fact.)
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/d26846484b7c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 1228466
You need to log in before you can comment on or make changes to this bug.