780397 - Convert FrameMetrics properties to float equivalents

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Doug Sherk (:drs) (inactive)]

The lack of precision in FrameMetrics' properties is really biting us when trying to use it for panning and zooming logic. We're running into a lot of rounding errors and weird situations where we're 1 px away from where we should be.

I think only mViewportScrollOffset should be a float, but this may have to change.

Comment 1

[Andrew Overholt [:overholt]]

Joe was wondering if we have similar issues on Fennec?

Comment 2

[Doug Sherk (:drs) (inactive)]

No, Java has its own class that is very similar called ViewportMetrics. It stores its scroll offset as a PointF, which is floating point.

Comment 3

[Doug Sherk (:drs) (inactive)]

Also I don't think this should block Basecamp, the main problem this is causing is really crappy finger following when the zoom level is maxed out.

Comment 4

[Andrew Overholt [:overholt]]

Thanks, Doug.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point

Proposed patch.

Try push (with other stuff that I know is fine): https://tbpl.mozilla.org/?tree=Try&rev=d9fb2084e4b9

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 653588 [details] [diff] [review]
Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point

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

::: dom/ipc/TabChild.cpp
@@ +725,5 @@
>      }
>  
>      nsCString data;
> +    data += nsPrintfCString("{ \"x\" : %d", NS_lround(aFrameMetrics.mViewportScrollOffset.x));
> +    data += nsPrintfCString(", \"y\" : %d", NS_lround(aFrameMetrics.mViewportScrollOffset.y));

Wouldn't it make more sense to use %f here?

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +812,5 @@
>              newDisplayPort = mFrameMetrics.mDisplayPort;
> +  gfx::Point oldScrollOffset = mLastPaintRequestMetrics.mViewportScrollOffset,
> +             newScrollOffset = mFrameMetrics.mViewportScrollOffset;
> +  oldDisplayPort.MoveBy(nsIntPoint(NS_lround(oldScrollOffset.x), NS_lround(oldScrollOffset.y)));
> +  newDisplayPort.MoveBy(nsIntPoint(NS_lround(newScrollOffset.x), NS_lround(newScrollOffset.y)));

This seems scary. We keep moving by something that's rounded ... aren't we going to accumulate error?

Would be better to not use MoveBy here.

::: layout/base/nsDisplayList.cpp
@@ +596,2 @@
>        aContainerParameters.mXScale, aContainerParameters.mYScale, auPerDevPixel);
> +    metrics.mViewportScrollOffset = mozilla::gfx::Point(scrollOffset.x, scrollOffset.y);

Don't snap to nearest pixels here. Just scale it and use the result.

Comment 7

[Doug Sherk (:drs) (inactive)]

Attachment: Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 653588 [details] [diff] [review]
> Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point
> 
> Review of attachment 653588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabChild.cpp
> @@ +725,5 @@
> >      }
> >  
> >      nsCString data;
> > +    data += nsPrintfCString("{ \"x\" : %d", NS_lround(aFrameMetrics.mViewportScrollOffset.x));
> > +    data += nsPrintfCString(", \"y\" : %d", NS_lround(aFrameMetrics.mViewportScrollOffset.y));
> 
> Wouldn't it make more sense to use %f here?

According to this:
https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindow.idl#297

ScrollTo takes long/ints. I figure it's generally better to serve JS any data already formatted the right way due to much stronger typing in C++ making it more clear what's happening.

What are your thoughts on this?

Otherwise, I dealt with your other review comments. Will push to try again on next iteration.

Comment 8

[Doug Sherk (:drs) (inactive)]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=aae0f1ae7abd

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point

Try push failed, here's a new version which should correct it. Asking for r? again because there were significant changes.

New try push:
https://tbpl.mozilla.org/?tree=Try&rev=d5a1c8da35fa

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 654069 [details] [diff] [review]
Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point

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

Document in nsIDOMWindowUtils.idl that setting the display port always triggers a recomposite so callers should avoid unnecessary changes.

Comment 11

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/610e57062816

(includes new comment in nsIDOMWindowUtils.idl)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/610e57062816