Closed
Bug 1230047
Opened 8 years ago
Closed 8 years ago
Make SynthesizeNativeTouch{Point,Tap}() take LayoutDeviceIntPoints
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(2 files, 1 obsolete file)
21.80 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
25.25 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
More LayoutDevicePixel conversions in widget code.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8695121 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
> 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.
Assignee | ||
Comment 6•8 years ago
|
||
Now with the correct units!
Attachment #8695551 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8695121 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
> ::: 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.
Assignee | ||
Comment 8•8 years ago
|
||
> 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 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06a87bae7c31 https://hg.mozilla.org/mozilla-central/rev/605d74217b11
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•