The AsyncPanZoomController used in B2G doesn't play well with the progressive tile painting code (which isn't on B2G currently but is on Fennec). This blocks using APZC on Fennec. The main problem is that when progressive tile painting is turned on, AsyncPanZoomController::NotifyLayersUpdated gets called per tile paint, rather than once per paint request. Unfortunately APZC assumes that it gets exactly one NotifyLayersUpdated call per paint request, and tracks this state using mWaitingForContentToPaint and the mPaintThrottler, as far as I can tell. When multiple NotifyLayersUpdated calls come in, mWaitingForContentToPaint is false for all but the first one, and so the code flows through the "else" case near the start of the function, clobbering the mFrameMetrics.mScrollOffset with the (old) scroll offset from aViewportFrame. On devices with large screens (e.g. Galaxy Tab) this results in the viewport constantly getting reset backwards as you pan.

There is general bugginess with this else clause. This also causes Bug 846611.

The attached patch fixes the problem on Fennec by removing the clobbering of mFrameMetrics.mScrollOffset in AsyncPanZoomController::NotifyLayersUpdated. Instead, a new mechanism for informing the APZC about content scrollTo()'s is introduced. On Fennec, browser.js informs GeckoLayerClient about scroll events, and GeckoLayerClient informs the APZC. On B2G, the mechanism is yet to be implemented.

Comment on attachment 778470 [details] [diff] [review] patch Review of attachment 778470 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Upload a new patch with the comments below addressed and flag it for checkin. ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +1158,5 @@ > > mFrameMetrics.mMayHaveTouchListeners = aViewportFrame.mMayHaveTouchListeners; > + > + // TODO: Once a mechanism for calling UpdateScrollOffset() when content does > + // a scrollTo() is implemented for B2G, this block can be removed. Update this comment to reference the new bug filed for the B2G code path ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +381,5 @@ > abortPanZoomAnimation(); > } > + mPanZoomController.updateScrollOffset( > + messageMetrics.viewportRectLeft / messageMetrics.zoomFactor, > + messageMetrics.viewportRectTop / messageMetrics.zoomFactor); nit: remove trailing whitespace ::: mobile/android/base/gfx/NativePanZoomController.java @@ +103,5 @@ > } > } > } > + > + public native void updateScrollOffset(float cssX, float cssY); fix indentation

Comment on attachment 778485 [details] [diff] [review] updated patch Review of attachment 778485 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +1159,5 @@ > mFrameMetrics.mMayHaveTouchListeners = aViewportFrame.mMayHaveTouchListeners; > + > + // TODO: Once a mechanism for calling UpdateScrollOffset() when content does > + // a scrollTo() is implemented for B2G (bug 895905), this block can be removed. > +#ifdef MOZ_WIDGET_GONK Sorry.. now that I think about this, it would be better to invert this condition and write it as #ifndef MOZ_WIDGET_ANDROID. The metro team is trying to stand up using APZC on win metro tablets and this will break it for them, so it's better to flip the condition.

Try results: https://tbpl.mozilla.org/?tree=Try&rev=7b3fbc3d8d8f

Note: this patch is affected badly by bug 896547. It should be tested with the dynamic toolbar turned off.

This may need to be revised for multi-APZC as well.