1220925 - Event::GetScreenCoords should return CSSIntPoint instead of LayoutDevicePoint

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Randall Barker [:rbarker]]

Update Event::GetScreenCoord so that it returns a CSSIntPoint instead of a LayoutDevicePoint with containing CSSIntPoint values.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Fix-Event-GetScreenCoord-to-return-CSSIntPoint-15110216-b73c75a.patch

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110411-12fe91b.patch

Comment 3

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d3a66316bc3

Comment 4

[Randall Barker [:rbarker]]

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.

Comment 5

[Randall Barker [:rbarker]]

Attachment: 0001-try-b-do-p-linux-linux64-macosx64-win32-android-api-9-android-api-11-emulator-u-all-t-none-15110413-b1e21a7.patch

Comment 6

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110413-bc1d768.patch

Comment 7

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=543f31866f51

Comment 8

[Botond Ballo [:botond]]

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.)

Comment 9

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110909-2aed3e7.patch

Address review comments. Carry forward r+ from :botond

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f334b2d473

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I have a rebased version ready to land. Just waiting for the tree to reopen.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a26be9d0cfed

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a26be9d0cfed