917761 - [10.7] Improvements for bug 678392 that were backed out with bug 673875 should reland in isolation

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stephen A Pohl [:spohl]]

After the patches for bug 678392 had been reviewed, additional code changes/improvements were added to the closely related patches for bug 673875. Unfortunately, when bug 673875 had to be backed out, we also lost the changes that affected bug 678392. We should reland these changes separately while bug 673875 is being revised. This would allow us to turn on history swipe animations by default (bug 860493) once bug 817700 has landed.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: bug917761

This re-adds improvements for:
1. Fast swiping.
2. No reload of the page when the user swipes back and forth and ends up on the same page.
3. Add damp value for bounce effect.
4. Workaround for bug 770626.

I will test all of the above some more before asking for a review.

Comment 2

[Stephen A Pohl [:spohl]]

Comment on attachment 806584 [details] [diff] [review]
bug917761

This is working as expected on my end. I specifically made sure that bug 770626 doesn't regress. Note that technically, this has already been reviewed in bug 673875. The only difference here is that I'm removing the parts that are specific to the vertical bounce behavior.

Comment 3

[Stephen A Pohl [:spohl]]

Comment on attachment 806584 [details] [diff] [review]
bug917761

Testing some more to make sure that we're not reintroducing bug 907275 (the original reason for backing out bug 673875).

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 806696 [details] [diff] [review]
Patch

This block (in current code) is obsoleted by your patch, and also needs to be removed:

  // Only initiate tracking for events whose horizontal element is at least
  // eight times larger than its vertical element.  This minimizes performance
  // problems with vertical scrolls (by minimizing the possibility that they'll
  // be misinterpreted as horizontal swipes), while still tolerating a small
  // vertical element to a true horizontal swipe.  The number '8' was arrived
  // at by trial and error.
  if ((deltaX == 0) || (fabs(deltaX) <= fabs(deltaY) * 8)) {
    return;
  }

+      uint32_t directionCopy = direction;

Why not just use mCurrentSwipeDir?

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Steven Michaud from comment #5)
> Comment on attachment 806696 [details] [diff] [review]
> Patch
> 
> This block (in current code) is obsoleted by your patch, and also needs to
> be removed:

Good catch, thanks! Removed.

> 
> +      uint32_t directionCopy = direction;
> 
> Why not just use mCurrentSwipeDir?

If we use mCurrentSwipeDir, it appears to be reset to 0 via a reentrant call to this handler before we could send the NS_SIMPLE_GESTURE_SWIPE event. I would need to trace execution to tell you exactly what happens, but what is clear is that using mCurrentSwipeDir is breaking fast swiping, i.e. multiple fast swipes in one direction keeps animating the same page instead of quickly swiping through the snapshots of the browser history.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

>>> +      uint32_t directionCopy = direction;
>>
>> Why not just use mCurrentSwipeDir?
>
> If we use mCurrentSwipeDir, it appears to be reset to 0 via a
> reentrant call to this handler before we could send the
> NS_SIMPLE_GESTURE_SWIPE event. I would need to trace execution to
> tell you exactly what happens, but what is clear is that using
> mCurrentSwipeDir is breaking fast swiping, i.e. multiple fast swipes
> in one direction keeps animating the same page instead of quickly
> swiping through the snapshots of the browser history.

OK.

But in that case it doesn't seem mCurrentSwipeDir serves any useful
purpose.  Why not get rid of it?

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Patch

Right, this must have become obsolete through the various revisions.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 806886 [details] [diff] [review]
Patch

Looks fine to me.  But you should probably also get someone else to review the non-widgets parts.

Comment 10

[Stephen A Pohl [:spohl]]

Hmm, my comment didn't post. I meant to say:

Thanks, Steven! Felipe, sending this to you since you've reviewed the original code in bug 673875. Feel free to reassign if necessary.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 806886 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +633,3 @@
>      if ((aVal >= 0 && this.isLTR) ||
>          (aVal <= 0 && !this.isLTR)) {
> +      let tempDampValue = 1;

The comment removed here ("Cap value to avoid sliding...") seems pretty useful. Let's keep it if it's still valid.

Comment 12

[Stephen A Pohl [:spohl]]

Thanks, Felipe!

(In reply to :Felipe Gomes from comment #11)
> ::: browser/base/content/browser-gestureSupport.js
> @@ +633,3 @@
> >      if ((aVal >= 0 && this.isLTR) ||
> >          (aVal <= 0 && !this.isLTR)) {
> > +      let tempDampValue = 1;
> 
> The comment removed here ("Cap value to avoid sliding...") seems pretty
> useful. Let's keep it if it's still valid.

We actually don't need to cap aVal anymore here. This patch adds the NSEventSwipeTrackingClampGestureAmount flag to our call to NSEvent::trackSwipeEventWithOptions in nsChildView.mm, so we get properly clamped values back from the OS.

Comment 13

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92ae873dea3a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92ae873dea3a