1144940 - Some Screen coordinate cleanups

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[Botond Ballo [:botond]]

Comment on attachment 8579668 [details] [diff] [review]
Patch

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

::: widget/android/AndroidJavaWrappers.h
@@ +575,5 @@
>      int mType;
>      bool mAckNeeded;
>      int64_t mTime;
>      nsTArray<nsIntPoint> mPoints;
> +    nsTArray<mozilla::ScreenSize> mPointRadii;

It looks like you would need some changes not in this patch to make this compile, like at [1] and [2].

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp?rev=52de461bce37#745
[2] http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp?rev=52de461bce37#432

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Updated patch

Sorry about that, this version actually builds.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8596963 [details] [diff] [review]
Updated patch

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

::: dom/events/Touch.h
@@ +79,5 @@
>    int32_t mIdentifier;
>    CSSIntPoint mPagePoint;
>    CSSIntPoint mClientPoint;
>    LayoutDeviceIntPoint mScreenPoint;
> +  ScreenSize mRadius;

Did you mean to change this from integers to floats? The getters still return integers, so I think it would make more sense as a ScreenIntSize.

::: widget/windows/winrt/MetroInput.cpp
@@ +88,5 @@
>      props->get_XTilt(&tiltX);
>      props->get_YTilt(&tiltY);
>  
>      nsIntPoint touchPoint = MetroUtils::LogToPhys(position);
> +    ScreenSize touchRadius;

Note that bug 1039866 removed widget/windows/winrt.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8596963 [details] [diff] [review]
Updated patch

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

::: widget/android/AndroidJavaWrappers.h
@@ +576,5 @@
>      int mType;
>      bool mAckNeeded;
>      int64_t mTime;
>      nsTArray<nsIntPoint> mPoints;
> +    nsTArray<mozilla::ScreenSize> mPointRadii;

After having a closer look and discussing this with Kats, I don't think this change is appropriate.

The coordinate system mPointRadii is in depends on the type of the event. For example, MakeTouchEvent() interprets it in CSS coordinates, while MakeMultiTouchInput() interprets it in Screen coordinates.

Therefore, it would be misleading to change the type of mPointRadii to be one or the other. We should instead leave it untyped, and cast to a typed object where appropriate.

(I believe the eventual proper solution here is to have these classes auto-generated (bug 720542), in which case we could have different classes for the different event types. Alternatively, it's also possible that one of the interpretations will become obsolete when Fennec switches to APZ).

Apologies for not noticing this sooner.

Comment 5

[David Zbarsky (:dzbarsky)]

Ah, I thought that multiplying by scale would make sure we were in the right coordinate system, but I just noticed we actually multiply by scale.scale there, so we lose the typing =(.

Comment 6

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

Comment on attachment 8596963 [details] [diff] [review]
Updated patch

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

::: dom/ipc/TabParent.cpp
@@ +904,5 @@
>    return false;
>  }
>  
>  void
> +TabParent::UpdateDimensions(const ScreenIntRect& rect, const ScreenIntSize& size)

According to the changes being made in bug 1125325 I suspect this may really be LayoutDeviceIntRect. Or perhaps the two are equivalent somehow in this context? It may be that it's LayoutDevice in the parent process but maps to Screen in the child process? I don't really know.

Comment 7

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> >  void
> > +TabParent::UpdateDimensions(const ScreenIntRect& rect, const ScreenIntSize& size)
> 
> According to the changes being made in bug 1125325 I suspect this may really
> be LayoutDeviceIntRect. Or perhaps the two are equivalent somehow in this
> context? It may be that it's LayoutDevice in the parent process but maps to
> Screen in the child process? I don't really know.

I'm not sure either, but I'll note that we treat the result of the APZC -> Gecko transform - which is basically Screen space with async transforms unapplied - as LayoutDevice space [1].

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=d7428dd3f948#1404

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.