The default bug view has changed. See this FAQ.

Convert FrameMetrics properties to float equivalents

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: drs, Assigned: drs)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 745136
Joe was wondering if we have similar issues on Fennec?
blocking-basecamp: --- → ?
(Assignee)

Comment 2

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

Comment 3

5 years ago
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.
Thanks, Doug.
blocking-basecamp: ? → -
(Assignee)

Comment 5

5 years ago
Created attachment 653588 [details] [diff] [review]
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
Assignee: nobody → bugzilla
Attachment #653588 - Flags: review?(roc)
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.
(Assignee)

Comment 7

5 years ago
Created attachment 653634 [details] [diff] [review]
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.
Attachment #653588 - Attachment is obsolete: true
Attachment #653588 - Flags: review?(roc)
Attachment #653634 - Flags: review?(roc)
Attachment #653634 - Flags: review?(roc) → review+
(Assignee)

Comment 8

5 years ago
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=aae0f1ae7abd
(Assignee)

Comment 9

5 years ago
Created attachment 654069 [details] [diff] [review]
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
Attachment #653634 - Attachment is obsolete: true
Attachment #654069 - Flags: review?(roc)
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.
Attachment #654069 - Flags: review?(roc) → review+
(Assignee)

Comment 11

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

(includes new comment in nsIDOMWindowUtils.idl)
https://hg.mozilla.org/mozilla-central/rev/610e57062816
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.