Last Comment Bug 859929 - AsyncPanZoomController doesn't play well with progressive tile painting
: AsyncPanZoomController doesn't play well with progressive tile painting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: 23 Branch
: All Android
: -- normal (vote)
: mozilla25
Assigned To: Botond Ballo [:botond]
:
Mentors:
Depends on:
Blocks: apz-fennec 887819 895905 898580
  Show dependency treegraph
 
Reported: 2013-04-09 11:19 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2013-07-26 13:19 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.44 KB, patch)
2013-07-19 07:48 PDT, Botond Ballo [:botond]
bugmail.mozilla: review+
Details | Diff | Review
updated patch (10.46 KB, patch)
2013-07-19 08:21 PDT, Botond Ballo [:botond]
botond: review+
Details | Diff | Review
updated patch (10.46 KB, patch)
2013-07-19 09:11 PDT, Botond Ballo [:botond]
botond: review+
Details | Diff | Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2013-04-09 11:19:07 PDT
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.
Comment 1 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2013-04-09 18:43:01 PDT
There is general bugginess with this else clause. This also causes Bug 846611.
Comment 2 Botond Ballo [:botond] 2013-07-19 07:48:45 PDT
Created attachment 778470 [details] [diff] [review]
patch

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 3 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-19 08:05:12 PDT
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 4 Botond Ballo [:botond] 2013-07-19 08:21:30 PDT
Created attachment 778485 [details] [diff] [review]
updated patch
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-19 08:56:07 PDT
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.
Comment 6 Botond Ballo [:botond] 2013-07-19 09:11:14 PDT
Created attachment 778510 [details] [diff] [review]
updated patch
Comment 7 Botond Ballo [:botond] 2013-07-19 12:12:51 PDT
Try results: https://tbpl.mozilla.org/?tree=Try&rev=7b3fbc3d8d8f
Comment 8 Botond Ballo [:botond] 2013-07-22 10:48:47 PDT
Note: this patch is affected badly by bug 896547. It should be tested with the dynamic toolbar turned off.
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-22 19:36:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5069084abd
Comment 10 Ed Morley [:emorley] 2013-07-23 10:13:58 PDT
https://hg.mozilla.org/mozilla-central/rev/ab5069084abd
Comment 11 Botond Ballo [:botond] 2013-07-26 13:19:26 PDT
This may need to be revised for multi-APZC as well.

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