Closed Bug 1053766 Opened 6 years ago Closed 6 years ago

Multiple TransformBegin/TransformEnd notifications issued by APZ during a single conceptual operation

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: kats, Assigned: kats)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

According to the discussion in bug 1020801 comment 14, we fire two sets of TransformBegin/TransformEnd notifications from APZ in the case where we pan into overscroll and do a snapback. We should avoid doing this since conceptually it is a single user operation and there should be a single TransformBegin/TransformEnd that encompasses the whole thing.
This probably happens when we switch from the fling to the snapback animation. As the fling animation terminates, it schedules the snabpack as a deferred task and returns false for the continueAnimation return value. This will result in the state getting to NOTHING temporarily at [1] before it is set to SNAP_BACK at [2]. This state glitch is likely what results in the TransformEnd/TransformBegin notifications.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=70b5978c6fb6#2124
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=70b5978c6fb6#1807
A possible approach is to place Transform[Begin|End] notifications into a "queue" of size 1 (with a newer one replacing an older one), and flush the "queue" at the end of conceptual operations.

Looking at the code, flushing them at the end of the following functions should be sufficient:

  SampleContentTransformForFrame
  HandleInputEvent
  CancelAnimation
  Destroy
  ZoomToRect

Kats, does this approach sound reasonable?
While reviewing this part of your patch on bug 1039992:

+      // Note:
+      //   This needs to be a deferred task even though it can safely run
+      //   while holding mMonitor, because otherwise, if the overscrolled APZC
+      //   is this one, then the SetState(NOTHING) in UpdateAnimation will
+      //   stomp on the SetState(SNAP_BACK) it does.
+      mDeferredTasks.append(NewRunnableMethod(mOverscrollHandoffChain.get(),
+                                              &OverscrollHandoffChain::SnapBackOverscrolledApzc));
       return false;

I was thinking it be simpler to just do what you said in the note (i.e. run while holding mMonitor instead of posting a deferred task) and then return true instead of returning false. The return true will indicate that there's still an animation going, and we won't flip into state NONE. Would that work?
Bug 1046159 contains a similar case of going to the NOTHING state and then back into the transforming state.
Depends on: 1046159
I believe your latest patches for flinging will also introduce a NOTHING state after the pan but before the fling kicks in. (Because of how we need to find the scrollgrabbing APZC to start the fling).
Assignee: nobody → bugmail.mozilla
Attachment #8488292 - Flags: review?(botond)
Comment on attachment 8488290 [details] [diff] [review]
Part 1 - Introduce the state change notification blocker

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

Nice approach :) More concise than what I suggested in comment 2.
Attachment #8488290 - Flags: review?(botond) → review+
Attachment #8488291 - Flags: review?(botond) → review+
Attachment #8488292 - Flags: review?(botond) → review+
Try push has some build failures. These I'm definitely blaming on nested enum class thingy :p
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Try push has some build failures. These I'm definitely blaming on nested
> enum class thingy :p

I'm not opposed to changing APZStateChange to be a regular enum if that makes things easier.
Depends on: 1066421
You need to log in before you can comment on or make changes to this bug.