Closed
Bug 780397
Opened 12 years ago
Closed 12 years ago
Convert FrameMetrics properties to float equivalents
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
People
(Reporter: drs, Assigned: drs)
References
Details
Attachments
(1 file, 2 obsolete files)
40.49 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Joe was wondering if we have similar issues on Fennec?
blocking-basecamp: --- → ?
Assignee | ||
Comment 2•12 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•12 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.
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=aae0f1ae7abd
Assignee | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/610e57062816 (includes new comment in nsIDOMWindowUtils.idl)
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/610e57062816
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•