Skip to content

Commit

Permalink
Bug 1697756. Restructure dmanip code so it's not possible to hit MOZ_…
Browse files Browse the repository at this point in the history
…ASSERT(updateLastScale) in DManipEventHandler::OnContentUpdated. r=botond

I'm not sure if the weird dmanip behaviour coming from the OS is new (because we have a new bug (bug 1697091) that seems like dmanip has changed what it sends us sometimes), or has always existed. But it seems like a good idea to make the code robust against this.

We can hit this assert in the following way. It seems as though dmanip decides after the first few OnContentUpdate calls if it is processing a pinch or a pan (because we can get a scale that is slightly different from 1 on the first OnContentUpdated call, but after that point never changes at all while the offset is changing fluidly) and if it decides it is a pan then it locks the scale to whatever it's last value was. So if we get a scale that is very close to 1 but not fuzzy equal to 1 on the first OnContentUpdated call we will decide that we are processing a pinch, but dmanip can then decide that it's processing a pan. Once the user lifts their finger dmanip will inform us via OnViewportStatusChanged that it is now in inertia mode, it is allowed to go into inertia for pans but not pinches because we pass DIRECTMANIPULATION_CONFIGURATION_TRANSLATION_INERTIA but not DIRECTMANIPULATION_CONFIGURATION_SCALING_INERTIA when we create dmanip here https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/widget/windows/DirectManipulationOwner.cpp#554 And then dmanip can go straight from inertia to running, this causes us to start a new pinch because our current scale is not fuzzy equal to 1. But also, the current scale has not changed, so it is equal to our last scale, so we have no scale change to send in the pinch start event.

An example of logging when it happens is helpful for understanding and can be found at https://bugzilla.mozilla.org/show_bug.cgi?id=1697756#c2

Differential Revision: https://phabricator.services.mozilla.com/D107994
  • Loading branch information
tnikkel committed Mar 13, 2021
1 parent 547a309 commit c21d494
Showing 1 changed file with 10 additions and 5 deletions.
15 changes: 10 additions & 5 deletions widget/windows/DirectManipulationOwner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,11 @@ void DManipEventHandler::TransitionToState(State aNewState) {
MOZ_ASSERT(aNewState == State::eNone);
// ePinching -> eNone: PinchEnd. ePinching should only transition to
// eNone.
SendPinch(Phase::eEnd, 0.f);
// Only send a pinch end if we sent a pinch start.
if (!mShouldSendPinchStart) {
SendPinch(Phase::eEnd, 0.f);
}
mShouldSendPinchStart = false;
break;
}
case State::eNone: {
Expand Down Expand Up @@ -325,10 +329,11 @@ DManipEventHandler::OnContentUpdated(IDirectManipulationViewport* viewport,
} else if (mState == State::ePinching) {
if (mShouldSendPinchStart) {
updateLastScale = SendPinch(Phase::eStart, scale);
// If we get here our current scale is not fuzzy equal to the previous
// scale, so SendPinch should return true.
MOZ_ASSERT(updateLastScale);
mShouldSendPinchStart = false;
// Only clear mShouldSendPinchStart if we actually sent the event
// (updateLastScale tells us if we sent an event).
if (updateLastScale) {
mShouldSendPinchStart = false;
}
} else {
updateLastScale = SendPinch(Phase::eMiddle, scale);
}
Expand Down

0 comments on commit c21d494

Please sign in to comment.