Last Comment Bug 780397 - Convert FrameMetrics properties to float equivalents
: Convert FrameMetrics properties to float equivalents
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on:
Blocks: 745136
  Show dependency treegraph
 
Reported: 2012-08-04 09:47 PDT by Doug Sherk (:drs) (inactive)
Modified: 2012-08-23 03:52 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point (35.50 KB, patch)
2012-08-20 17:24 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point (38.07 KB, patch)
2012-08-20 20:01 PDT, Doug Sherk (:drs) (inactive)
roc: review+
Details | Diff | Splinter Review
Convert FrameMetrics.mViewportScrollOffset from nsIntPoint to gfx::Point (40.49 KB, patch)
2012-08-21 21:14 PDT, Doug Sherk (:drs) (inactive)
roc: review+
Details | Diff | Splinter Review

Description Doug Sherk (:drs) (inactive) 2012-08-04 09:47:43 PDT
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] 2012-08-13 11:33:37 PDT
Joe was wondering if we have similar issues on Fennec?
Comment 2 Doug Sherk (:drs) (inactive) 2012-08-13 12:49:47 PDT
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) 2012-08-13 15:07:43 PDT
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] 2012-08-13 15:15:58 PDT
Thanks, Doug.
Comment 5 Doug Sherk (:drs) (inactive) 2012-08-20 17:24:56 PDT
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
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-20 17:35:31 PDT
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) 2012-08-20 20:01:07 PDT
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.
Comment 8 Doug Sherk (:drs) (inactive) 2012-08-21 00:24:58 PDT
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=aae0f1ae7abd
Comment 9 Doug Sherk (:drs) (inactive) 2012-08-21 21:14:24 PDT
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
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-21 21:26:52 PDT
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) 2012-08-21 21:39:04 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/610e57062816

(includes new comment in nsIDOMWindowUtils.idl)
Comment 12 Ed Morley [:emorley] 2012-08-23 03:52:50 PDT
https://hg.mozilla.org/mozilla-central/rev/610e57062816

Note You need to log in before you can comment on or make changes to this bug.