1222943 - Remove IntSizeTyped::{To,From}UnknownSize()

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicholas Nethercote [inactive]]

IntSizeTyped::{To,From}UnknownSize() can be removed without too much effort.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Give AndroidGeckoEvent::{mPoints,mPointRadii} units

Please note that the PixelCastJustification is just a guess. Other than that,
the patch seems solid.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove IntSizeTyped::{To,From}UnknownSize()

This patch also removes the last, trivial use of ToUnknownSize().

Comment 3

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

Comment on attachment 8684846 [details] [diff] [review]
(part 1) - Give AndroidGeckoEvent::{mPoints,mPointRadii} units

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

The radius stored on a Touch should be in LayoutDevicePixels, and should basically go through the same conversion as the refpoint wherever possible. But also you should talk to Botond about this because it may conflict with bug 1222661.

::: dom/base/nsDOMWindowUtils.cpp
@@ +952,5 @@
>      LayoutDeviceIntPoint pt =
>        nsContentUtils::ToWidgetPoint(CSSPoint(aXs[i], aYs[i]), offset, presContext);
>      RefPtr<Touch> t = new Touch(aIdentifiers[i],
> +                                pt,
> +                                ScreenIntPoint(aRxs[i], aRys[i]),

Here, for example, aRxs is in CSS pixels, and needs to be converted to LayoutDevicePixels using the same conversion factor that nsContentUtils::ToWidgetPoint uses (i.e. LayoutDeviceIntPoint::FromAppUnitsRounded(CSSPoint::ToAppUnits(...), presContext->AppUnitsPerDevPixel()))

::: dom/ipc/TabParent.cpp
@@ +2879,5 @@
>          presContext->AppUnitsPerDevPixel());
>  
>      RefPtr<Touch> t = new Touch(aIdentifiers[i],
> +                                pt,
> +                                ScreenIntPoint(aRxs[i], aRys[i]),

Here also aRxs is in CSSPixels and needs to be converted properly.

::: widget/android/AndroidJavaWrappers.cpp
@@ +235,5 @@
>      jEndDrawingMethod = layerRendererFrame.getMethod("endDrawing", "()V");
>  }
>  
>  void
> +AndroidGeckoEvent::ReadScreenIntPointArray(nsTArray<ScreenIntPoint> &points,

I'm hesitant to convert this function wholesale. the GeckoEvent::mPoints field isn't specifically in any coordinate system, because different messages use it in different ways and it's not guaranteed to always be in ScreenPixels. Also :jchen is working on getting rid of the AndroidGeckoEvent stuff in bug 1188959, so it solve this problem in a better way.

Comment 4

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

Comment on attachment 8684847 [details] [diff] [review]
(part 2) - Remove IntSizeTyped::{To,From}UnknownSize()

I'll defer this to Botond so that he make the call as to how it integrates with the work in bug 1222661.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8684847 [details] [diff] [review]
(part 2) - Remove IntSizeTyped::{To,From}UnknownSize()

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

This is basically orthogonal to bug 1222661; that's about transitioning the older ToUntyped() functions to the newer ToUnknownXXX() functions, and this is about removing one of the ToUnknownXXX() functions by replacing all its uses with strongly-typed units. The only interaction would be if there were ToUntyped() calls involving Size, but there aren't.

Comment 6

[Nicholas Nethercote [inactive]]

> >  void
> > +AndroidGeckoEvent::ReadScreenIntPointArray(nsTArray<ScreenIntPoint> &points,
> 
> I'm hesitant to convert this function wholesale. the GeckoEvent::mPoints
> field isn't specifically in any coordinate system, because different
> messages use it in different ways and it's not guaranteed to always be in
> ScreenPixels.

Fair enough. But that will defeat the goal of this bug, which was to remove {To,From}UnknownSize(). Well, I could just change this:

> ScreenSize::FromUnknownSize(gfx::Size(radius.x, radius.y)),

to this:

> ScreenSize(radius.x, radius.y)

And likewise for the FromUnknownPoint() call. That feels like cheating, but maybe it's ok?

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Change Touch::mRadius from nsIntPoint to LayoutDeviceIntPoint

Things I'm unsure about:

- In nsWindow.mm I took an educated guess about what to do. That code appears
  to not be actually compiled on my Mac -- I inserted some deliberate syntax
  errors and Firefox compiled ok.

- In SingleTouchData::ToNewDOMTouch() we convert from ScreenPixels to
  LayoutDevicePixels without any apparent justification.

- If the radius is never actually used, maybe it would be better just to remove
  it?

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove an unnecessary call to ToUnknownSize()

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8685152 [details] [diff] [review]
(part 2) - Remove an unnecessary call to ToUnknownSize()

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

Carrying over r+; this is a subset of the previously r+'d patch.

Comment 10

[Botond Ballo [:botond]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Fair enough. But that will defeat the goal of this bug, which was to remove
> {To,From}UnknownSize(). Well, I could just change this:
> 
> > ScreenSize::FromUnknownSize(gfx::Size(radius.x, radius.y)),
> 
> to this:
> 
> > ScreenSize(radius.x, radius.y)
> 
> And likewise for the FromUnknownPoint() call. That feels like cheating, but
> maybe it's ok?

My understanding is that the purpose of the FromUnknown*() and ToUnknown*() functions is to make conversions between typed and untyped quantities more visible and greppable until we switch them over to typed units. 

If we're not going to be switching a call site over to typed units, I'd rather keep the FromUnknown*() function and keep the conversion visible/greppable, than hide it by doing 'ScreenSize(radius.x, radius.y)'.

Comment 11

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

Comment on attachment 8685151 [details] [diff] [review]
(part 1) - Change Touch::mRadius from nsIntPoint to LayoutDeviceIntPoint

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

This looks good, thanks. The widget/uikit/ code is built in the iOS port that ted is working on, so that's probably why compile errors weren't getting caught when you build for Mac desktop.

Comment 12

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

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Created attachment 8685151 [details] [diff] [review]
> (part 1) - Change Touch::mRadius from nsIntPoint to LayoutDeviceIntPoint
> 
> - In SingleTouchData::ToNewDOMTouch() we convert from ScreenPixels to
>   LayoutDevicePixels without any apparent justification.

This is a bit of a special case because the screen point in the SingleTouchData gets modified in-place from screen coordinates to layout coordinates before ToNewDOMTouch gets called.

> 
> - If the radius is never actually used, maybe it would be better just to
> remove
>   it?

It would be nice to but it is exposed to web content so removing it is nontrivial. And maybe in the future once more hardware actually provides real data for this we can start relying on it.

Comment 13

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

And yeah, I agree with what Botond said in comment 10. The reason these functions were added were to allow incremental conversion to strongly typed units; the to/from Unknown calls happened at the boundary between the strongly typed and non-strongly typed units.

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d81f5f2da40430d5692e38359fa92366f7d7929b
Bug 1222943 (part 1) - Change Touch::mRadius from nsIntPoint to LayoutDeviceIntPoint. r=kats.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a037109abaee4b38b0149d538631cfe0337eb76b
Bug 1222943 (part 2) - Remove an unnecessary call to ToUnknownSize(). r=botond.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d81f5f2da404
https://hg.mozilla.org/mozilla-central/rev/a037109abaee