1240065 - Reconsider viewport update policy when pinch zooming

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Right now when you pinch zoom out, you often see a broken-looking page because we don't have tiles for the newly exposed area. Only when you release the pinch do we update the viewport to include this area. I think we should consider other times to do the update. One strategy would be to use a timer, and update the viewport rect when there isn't any (significant) pinching activity after, say, 250ms. That way we don't introduce compositor jank for quick pinchs, but slower ones where you are more likely to notice weird stuff will get updated. Since this is mostly a problem when we zoom out, we could also restrict this behavior to that case.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1240065-Throttle-repaints-when-pinch-zooming-16052614-30ddb0a.patch

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1240065-Throttle-repaints-when-pinch-zooming-r-16052614-dd6eaf3.patch

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8757036 [details] [diff] [review]
0001-Bug-1240065-Throttle-repaints-when-pinch-zooming-r-16052614-dd6eaf3.patch

Review of attachment 8757036 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3874,5 @@
> +void AsyncPanZoomController::DoDelayedRequestContentRepaint()
> +{
> +  if (!IsDestroyed() && mPaintTimerSet) {
> +    ReentrantMonitorAutoEnter lock(mMonitor);
> +    ScheduleComposite();

There shouldn't be a need to schedule another composite here - we schedule one on each scale event already, and when the repaint comes back that will schedule its own.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +1196,5 @@
>    // GetSnapPointForDestination). It should generally be determined by the
>    // type of event that's triggering the scroll.
>    Maybe<CSSPoint> FindSnapPointNear(const CSSPoint& aDestination,
>                                      nsIScrollableFrame::ScrollUnit aUnit);
> +  void DoDelayedRequestContentRepaint();

We've started organizing the members of AsyncPanZoomController by functionality. Notice the comments that say "The functions and members in this section are used to ...". These sections are towards the end of the class declaration, and then there's still a bunch of uncategorized stuff earlier on.

In keeping with this, please do one of the following:

  1) Find a relevant section, and add these members there.
     (I had a quick look and didn't see an obvious fit.)

  2) Add a new section for these members.

  3) Put these members in the uncategorized section.

@@ +1197,5 @@
>    // type of event that's triggering the scroll.
>    Maybe<CSSPoint> FindSnapPointNear(const CSSPoint& aDestination,
>                                      nsIScrollableFrame::ScrollUnit aUnit);
> +  void DoDelayedRequestContentRepaint();
> +  bool mPaintTimerSet;

The name of this variable should include the word "scale" or "pinch", to make it clear it's specific to that, as opposed to being a more general paint timer.

::: gfx/thebes/gfxPrefs.h
@@ +186,5 @@
>    DECL_GFX_PREF(Live, "apz.y_skate_highmem_adjust",            APZYSkateHighMemAdjust, float, 0.0f);
>    DECL_GFX_PREF(Live, "apz.y_skate_size_multiplier",           APZYSkateSizeMultiplier, float, 2.5f);
>    DECL_GFX_PREF(Live, "apz.y_stationary_size_multiplier",      APZYStationarySizeMultiplier, float, 3.5f);
>    DECL_GFX_PREF(Live, "apz.zoom_animation_duration_ms",        APZZoomAnimationDuration, int32_t, 250);
> +  DECL_GFX_PREF(Live, "apz.scale_repaint_delay_ms",            APZScaleRepaintDelay, int32_t, 500);

Please add the pref to modules/libpref/init/all.js as well.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be44f7cfc72
Throttle repaints when pinch zooming r=botond

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9be44f7cfc72