681054 - Simplify the handling of SwipeAnimationCancelled

Status: RESOLVED INVALID

(Core :: Widget: Cocoa, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Simplify the handling of SwipeAnimationCancelled

The way we current handle SwipeAnimationCancelled looks really strange. We have a __block variable that holds a Boolean and a member value that points to it.

If the function where the block is declared is called twice, the member variable is overwritten nothing but the old block will see the old __block variable.

Since we already use "this" in the block, it looks simpler and more efficient (avoids a separate heap allocation) to just use a Boolean member variable. 

A try run of the attached patch is at:

http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=8417d7d25d6f

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 554956 [details] [diff] [review]
Simplify the handling of SwipeAnimationCancelled

This makes the code simpler. Asking Steven to take a look in case he had a specific reason for writing it that way.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

> Asking Steven to take a look in case he had a specific reason for writing
> it that way.

That's the way it was in the Apple sample code I started from.  I'll need to do some testing (and thinking) to make sure the suggested change is really equivalent to the original code.

Comment 3

[Markus Stange [:mstange]]

I think this patch changes behavior in the case where a new swipe is started while another swipe is still in progress: The old swipe isn't canceled, because the block doesn't run during the time when mSwipeAnimationCancelled is NO (it's set to YES again in the same function).

I don't know if there is a way to hit that situation, though... maybe if you have both a swipe-able touchpad and a Magic Mouse, and you start swiping on one while already swiping on the other.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

You are right. I was confused by the previous block being the only one to have a pointer to the old variable. While that is true, we are sure that every "dangling" variable has be set to YES, and that is what we want.

Sorry for the confusion.