Closed Bug 1222943 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

IntSizeTyped::{To,From}UnknownSize() can be removed without too much effort.
Please note that the PixelCastJustification is just a guess. Other than that,
the patch seems solid.
Attachment #8684846 - Flags: review?(bugmail.mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This patch also removes the last, trivial use of ToUnknownSize().
Attachment #8684847 - Flags: review?(bugmail.mozilla)
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.
Attachment #8684846 - Flags: review?(bugmail.mozilla) → review-
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.
Attachment #8684847 - Flags: review?(bugmail.mozilla) → review?(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.
Attachment #8684847 - Flags: review?(botond) → review+
> >  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?
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?
Attachment #8685151 - Flags: review?(bugmail.mozilla)
Attachment #8684846 - Attachment is obsolete: true
Attachment #8684847 - Attachment is obsolete: true
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.
Attachment #8685152 - Flags: review+
(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 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.
Attachment #8685151 - Flags: review?(bugmail.mozilla) → review+
(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.
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.
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.
https://hg.mozilla.org/mozilla-central/rev/d81f5f2da404
https://hg.mozilla.org/mozilla-central/rev/a037109abaee
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.