Closed Bug 1230047 Opened 8 years ago Closed 8 years ago

Make SynthesizeNativeTouch{Point,Tap}() take LayoutDeviceIntPoints

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(2 files, 1 obsolete file)

More LayoutDevicePixel conversions in widget code.
Specifically, the PaintWindow() functions in the following classes:
- nsIWidgetListener, and its subclasses nsView and nsWebBrowser;
- nsChildView;
- nsWindow (the one in widget/uikit/);
- nsViewManager.
Attachment #8695124 - Flags: review?(bugmail.mozilla)
Comment on attachment 8695121 [details] [diff] [review]
Make SynthesizeNativeTouch{Point,Tap}() take LayoutDeviceIntPoints

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

I believe these should all be ScreenIntPoint rather than LayoutDeviceIntPoint. The SynthesizeNativeXXX functions simulate input coming in from the OS level which should be in screen coordinates.
Attachment #8695121 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8695124 [details] [diff] [review]
(part 2) - Make several PaintWindow() functions use LayoutDevice coordinates

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

LGTM, thanks!

::: embedding/browser/nsWebBrowser.cpp
@@ +1741,1 @@
>      root->SetVisibleRegion(LayerIntRegion::FromUnknownRegion(dirtyRect));

Here we're taking a LayoutDeviceIntRegion and turning the bounds into a LayerIntRegion. I think this should be ok because the difference between those two spaces is the resolution, and there won't be a resolution on the root layer. However, we can probably add a new PixelCastJustification for this and do a ViewAs rather using an intermediate nsIntRect. I'm fine with deferring this to a follow-up.

::: view/nsViewManager.cpp
@@ +290,5 @@
>    }
>  
>    // damageRegion is the damaged area, in twips, relative to the view origin
> +  nsRegion damageRegion =
> +    aRegion.ToUnknownRegion().ToAppUnits(AppUnitsPerDevPixel());

This ToUnknownRegion() should be unnecessary; ToAppUnits exists on BaseIntRegion, and LayoutDeviceIntRegion extends from that.

::: widget/uikit/nsWindow.mm
@@ +347,5 @@
> +  LayoutDeviceIntRegion region(
> +    NSToIntRound(aRect.origin.x * scale),
> +    NSToIntRound(aRect.origin.y * scale),
> +    NSToIntRound(aRect.size.width * scale),
> +    NSToIntRound(aRect.size.height * scale));

Not sure this will compile, you might need to keep the same structure as the old code:

LayoutDeviceIntRegion region = LayoutDeviceIntRect(...);
Attachment #8695124 - Flags: review?(bugmail.mozilla) → review+
> I believe these should all be ScreenIntPoint rather than
> LayoutDeviceIntPoint.

Oh dear... I totally overlooked that the three FromUnknownPoint() calls I removed were ScreenIntPoint rather than LayoutDeviceIntPoint. Plus all the identifiers with "screen" in them. Thank you for catching my mistake.
Now with the correct units!
Attachment #8695551 - Flags: review?(bugmail.mozilla)
Attachment #8695121 - Attachment is obsolete: true
> ::: view/nsViewManager.cpp
> @@ +290,5 @@
> >    }
> >  
> >    // damageRegion is the damaged area, in twips, relative to the view origin
> > +  nsRegion damageRegion =
> > +    aRegion.ToUnknownRegion().ToAppUnits(AppUnitsPerDevPixel());
> 
> This ToUnknownRegion() should be unnecessary; ToAppUnits exists on
> BaseIntRegion, and LayoutDeviceIntRegion extends from that.

That doesn't work. BaseIntRegion::ToAppUnits() has this line:

> nsRect appRect = ::ToAppUnits(*currentRect, aAppUnitsPerPixel);

The ::ToAppUnits() function being called is this one:

> nsRect ToAppUnits(const mozilla::gfx::IntRect& aRect, nscoord aAppUnitsPerPixel);

which expects an IntRect (a.k.a. an IntRectTyped<UnknownUnits>) but it's receiving an IntRectTyped<LayoutDevicePixels>.

So I changed ::ToAppUnits() to be templated, like this:

> template<class units>
> nsRect ToAppUnits(const mozilla::gfx::IntRectTyped<units>& aRect, nscoord aAppUnitsPerPixel);

and now it works.
> So I changed ::ToAppUnits() to be templated... and now it works.

Oh, I also had to put the definition in the .h file to avoid linking errors.
Comment on attachment 8695551 [details] [diff] [review]
(part 1) - Make SynthesizeNativeTouch{Point,Tap}() take ScreenIntPoints

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

Great, thanks!

::: widget/nsBaseWidget.cpp
@@ +1915,5 @@
>  #endif
>      return;
>    }
>  
> +  AutoObserverNotifier notifier(self->mLongTapTouchPoint->mObserver, "touchtap");

Hah, thanks for fixing this! :)
Attachment #8695551 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a87bae7c3151c41067c49cd35fcf6ce60a2553
Bug 1230047 (part 1) - Make SynthesizeNativeTouch{Point,Tap}() take ScreenIntPoints. r=kats.

https://hg.mozilla.org/integration/mozilla-inbound/rev/605d74217b116eb40b211eb2545212ec15823cd1
Bug 1230047 (part 2) - Make several PaintWindow() functions use LayoutDevice coordinates. r=kats.
https://hg.mozilla.org/mozilla-central/rev/06a87bae7c31
https://hg.mozilla.org/mozilla-central/rev/605d74217b11
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.