Closed
Bug 1220925
Opened 9 years ago
Closed 9 years ago
Event::GetScreenCoords should return CSSIntPoint instead of LayoutDevicePoint
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: rbarker, Assigned: rbarker)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
Update Event::GetScreenCoord so that it returns a CSSIntPoint instead of a LayoutDevicePoint with containing CSSIntPoint values.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8682260 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d3a66316bc3
Assignee | ||
Updated•9 years ago
|
Attachment #8683238 -
Flags: review?(botond)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8683238 [details] [diff] [review] 0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110411-12fe91b.patch Need to address try bustage with latest patch.
Attachment #8683238 -
Flags: review?(botond)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8683238 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8683335 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=543f31866f51
Assignee | ||
Updated•9 years ago
|
Attachment #8683336 -
Flags: review?(botond)
Comment 8•9 years ago
|
||
Comment on attachment 8683336 [details] [diff] [review] 0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110413-bc1d768.patch Review of attachment 8683336 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. We should be on alert for any regressions caused by this, as it's changing the behaviour of a pretty core piece of code in some cases. I believe the change makes the behaviour correct, but who knows what may have been relying on the old (incorrect) behaviour... ::: dom/events/Event.cpp @@ +933,5 @@ > } > > WidgetGUIEvent* guiEvent = aEvent->AsGUIEvent(); > if (!guiEvent->widget) { > + return RoundedToInt(aPoint / aPresContext->CSSToDevPixelScale()); CSSToDevPixelScale() uses AppUnitsPerDevPixel(), whereas below we use AppUnitsPerDevPixelAtUnitFullZoom(). I think we should be doing the same in both cases. I'd suggest the following: WidgetGUIEvent* guiEvent = aEvent->AsGUIEvent(); LayoutDeviceIntPoint offset = aPoint; if (guiEvent->widget) { offset += guiEvent->widget->WidgetToScreenOffset(); } nsPoint pt = ...; ::: dom/events/UIEvent.cpp @@ +363,4 @@ > Event::GetScreenCoords(mPresContext, mEvent, mEvent->refPoint); > nsresult rv = Event::DuplicatePrivateData(); > if (NS_SUCCEEDED(rv)) { > + mEvent->refPoint = mPresContext ? RoundedToInt(screenPoint * mPresContext->CSSToDevPixelScale()) : LayoutDeviceIntPoint(screenPoint.x, screenPoint.y); Would prefer: CSSToLayoutDeviceScale scale = mPresContext ? mPresContext->CSSToDevPixelScale() : CSSToLayoutDeviceScale(1); mEvent->refPoint = RoundedToInt(screenPoint * scale); ::: dom/events/UIEvent.h @@ -39,5 @@ > NS_IMETHOD DuplicatePrivateData() override; > NS_IMETHOD_(void) Serialize(IPC::Message* aMsg, bool aSerializeInterfaceType) override; > NS_IMETHOD_(bool) Deserialize(const IPC::Message* aMsg, void** aIter) override; > > - static LayoutDeviceIntPoint CalculateScreenPoint(nsPresContext* aPresContext, Nice find! (The duplication that you're removing, I mean.)
Attachment #8683336 -
Flags: review?(botond) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Address review comments. Carry forward r+ from :botond try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f334b2d473
Assignee | ||
Updated•9 years ago
|
Attachment #8683336 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 years ago
|
||
Hi, this didn;t apply cleanly: applying -Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110909-2aed3e7.patch patching file dom/events/Touch.cpp Hunk #2 FAILED at 48 1 out of 2 hunks FAILED -- saving rejects to file dom/events/Touch.cpp.rej patching file dom/events/Touch.h Hunk #1 FAILED at 75 1 out of 1 hunks FAILED -- saving rejects to file dom/events/Touch.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and refresh -Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110909-2aed3e7.patch
Flags: needinfo?(rbarker)
Keywords: checkin-needed
Comment 11•9 years ago
|
||
I have a rebased version ready to land. Just waiting for the tree to reopen.
Flags: needinfo?(rbarker)
Comment 13•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a26be9d0cfed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•