Closed Bug 1240065 Opened 4 years ago Closed 4 years ago

Reconsider viewport update policy when pinch zooming

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: snorp, Assigned: rbarker)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → rbarker
Attachment #8757036 - Flags: review?(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.
Attachment #8757036 - Flags: review?(botond) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be44f7cfc72
Throttle repaints when pinch zooming r=botond
https://hg.mozilla.org/mozilla-central/rev/9be44f7cfc72
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.