The default bug view has changed. See this FAQ.

AsyncPanZoomController doesn't play well with progressive tile painting

RESOLVED FIXED in mozilla25

Status

()

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

People

(Reporter: kats, Assigned: botond)

Tracking

(Blocks: 1 bug)

23 Branch
mozilla25
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Version: 22 Branch → 23 Branch
There is general bugginess with this else clause. This also causes Bug 846611.
Blocks: 887819
Assignee: nobody → botond

Updated

4 years ago
Blocks: 895358
(Assignee)

Comment 2

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

Updated

4 years ago
Attachment #778470 - Flags: review?(bugmail.mozilla)
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
Attachment #778470 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Updated

4 years ago
Blocks: 895905
(Assignee)

Updated

4 years ago
No longer blocks: 895358
(Assignee)

Comment 4

4 years ago
Created attachment 778485 [details] [diff] [review]
updated patch
Attachment #778470 - Attachment is obsolete: true
Attachment #778485 - Flags: review+
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.
(Assignee)

Comment 6

4 years ago
Created attachment 778510 [details] [diff] [review]
updated patch
Attachment #778485 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #778510 - Flags: review+
(Assignee)

Comment 7

4 years ago
Try results: https://tbpl.mozilla.org/?tree=Try&rev=7b3fbc3d8d8f
(Assignee)

Comment 8

4 years ago
Note: this patch is affected badly by bug 896547. It should be tested with the dynamic toolbar turned off.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5069084abd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ab5069084abd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

4 years ago
Blocks: 898580
(Assignee)

Comment 11

4 years ago
This may need to be revised for multi-APZC as well.
You need to log in before you can comment on or make changes to this bug.