998025 - Provide some sort of overscroll termination effect when overscroll handoff is not possible

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

This bug is to track the UX request (aimed for B2G 2.0) to provide a "bounce" effect when you reach the end of a scrolling container. Except it won't be "bounce" because of legal reasons, it'll be something else.

For a first cut this would extend the overscroll handoff code and the effect would be applied to the last scrollable frame in the handoff chain. Subsequent bugs can be filed to tune the behaviour once that is done.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Assigning to botond for now per our discussions.

Comment 2

[Gordon Brander :gordonb]

Hypothesis: overscroll resistance is best described as a 2D constraint
(spring), because:

* Springs respond to velocity
* Springs snap back to a resting position
* Springs offer increased resistance to increased extension (e.g. springs
  "fight back" when you stretch them too far).

Inputs for 2D Verlet-integrated springs:

* particle 0 (free particle)
  * mass
  * pos x
  * pos y
  * prev pos x
  * prev pos y
  * delta T
* particle 1 (anchor particle)
  * mass: Infinity
  * pos x
  * pos y
  * prev pos x
  * prev pos y
  * delta T
* Spring 0 (snap-back spring)
  * particle 0
  * particle 1
  * resting length: 0
  * stiffness

Fling velocity/user drag force would be applied via F / Hooke's law
https://en.wikipedia.org/wiki/Hooke%27s_law to find spring length at time.
Snap-back could use same equation, but solving for F instead of length.

Comment 3

[Gordon Brander :gordonb]

Scenarios for overscroll:

1. fling velocity would cause an eventual resting point outside of the bounds of the scroll. In this case, the "left over" force is applied to the overscroll physics (which respond by moving back to their resting point).

2. user drags "past" the normal bound of scrolling. E.g. I am at the top of a scrolling view and pull down. The further I pull down, the more the overscroll physics resist, until overscroll reaches full extension. Upon release of finger, overscroll physics snap back to resting position.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Just to be clear, when we talk about what we won't do ("bounce") we mean that overscroll, once released, will just go back to the 0 position, rather than go past it in the other direction, perhaps bouncing back and forth a few times?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Also, how is this effect different from the "overscroll" effect when getting to the end of the edge effect list and getting the stretch and snap (sorry, don't know what we're calling all of these things - by the edge effect I mean when we change between apps just dragging on the edge)

Comment 6

[Gordon Brander :gordonb]

(In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond] from comment #4)
> Just to be clear, when we talk about what we won't do ("bounce") we mean
> that overscroll, once released, will just go back to the 0 position, rather
> than go past it in the other direction, perhaps bouncing back and forth a
> few times?

It's possible for spring-based physics to bounce back and forth around resting point, or not, depending on how you tune mass and spring stiffness.

Comment 7

[Gordon Brander :gordonb]

(In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond] from comment #5)
> Also, how is this effect different from the "overscroll" effect when getting
> to the end of the edge effect list and getting the stretch and snap (sorry,
> don't know what we're calling all of these things - by the edge effect I
> mean when we change between apps just dragging on the edge)

Great question. Sheets = the edge gesture to move between apps and app states. In theory, it would be elegant to tie the spacial model of sheets into overscroll effect, but I think we'll have to keep the 2 things separate because of technical constraints:

* Overscroll is handled at the Gecko level, where sheets are managed by Gaia as separate iframes.
* Overscroll will need to work on nested scrolling views, since we frequently use multiple scrolling views within the same sheet (see Music app tabs for an example). Sheets are specifically system-level iframes rather than any scrolling view.

In the full sheets model, you can think of sheets as representing page refreshes. Clicking a link will slide over a new sheet, for example. It'll be a couple releases before we get to the full sheets model, however.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I discussed the use of shaders to accomplish these effects with some other people on the gfx team. The general consensus was that pretty much all of the effects are doable, but some are much more expensive than others. Things like blurring or averaging require readbacks and will be more expensive. Discontinuous transform functions will also be more expensive than continuous functions, because they generally require branching in the shader code. A lot of this depends on the implementation details but it would be good to have a list of effects that would be acceptable rather than just having The One Perfect Effect.

The perf hit will also vary across GPUs and some effects may be too slow on some GPUs. It would be worth thinking about whether in such cases it would be better to have a cheaper effect across the board, or to disable the effects on some devices and leave them enabled on others.

Also with respect to implementing the shader and hooking it up to the APZ code, Bas brought up tiling as a possible obstacle. Since we draw individual tiles with separate DrawQuad calls, we will have to add some more machinery to TiledContentClient that can adjust the shader as needed for different tiles.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

The other thing that came up is for hit detection. If the user taps on some element while an overscroll effect is applied, we probably can't or don't want to map that back to the element the user visually tapped on. We have three sane choices:

1) user interaction cancels the effect (i.e. the effect immediately ends and the tap goes to whatever is now at the tap point)
2) user interaction pretends the effect isn't there (i.e. the tap goes to where the element will be after the effect is over)
3) user interaction is ignored when an effect is present (i.e. tap does nothing)

Comment 10

[Gordon Brander :gordonb]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> The other thing that came up is for hit detection. If the user taps on some
> element while an overscroll effect is applied, we probably can't or don't
> want to map that back to the element the user visually tapped on.

Here's my take from UX side...

Try: 3) user interaction is ignored when an effect is present (i.e. tap does nothing)

Why:
* The other approaches break the continuity of the physics model.
  It feels like a "time skip" or "action at a distance".
* It's a temporary state that wants to "spring back" to a normal state on its own.
  You're only there if velocity is high or you're actively tugging past boundaries of page.
  In both cases, you're going to have an easier time hitting the correct thing when overscroll springs back.
* Principle of least surprise. It's a pretty impressive feat to both tug the overscroll
  down and tap an element. Chances are if it is triggered, its not intentional.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Makes sense, I think we can roll with that.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Gordon Brander :gordonb from comment #6)
> (In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond]
> from comment #4)
> > Just to be clear, when we talk about what we won't do ("bounce") we mean
> > that overscroll, once released, will just go back to the 0 position, rather
> > than go past it in the other direction, perhaps bouncing back and forth a
> > few times?
> 
> It's possible for spring-based physics to bounce back and forth around
> resting point, or not, depending on how you tune mass and spring stiffness.

Good.  No IP issues with doing that back and forth? What is the IP issue? :)

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Gordon Brander :gordonb from comment #7)
> (In reply to Milan Sreckovic [:milan, travelling, maybe slow to respond]
> from comment #5)
> > Also, how is this effect different from the "overscroll" effect when getting
> > to the end of the edge effect list and getting the stretch and snap (sorry,
> > don't know what we're calling all of these things - by the edge effect I
> > mean when we change between apps just dragging on the edge)
> 
> Great question. Sheets = the edge gesture to move between apps and app
> states. In theory, it would be elegant to tie the spacial model of sheets
> into overscroll effect, but I think we'll have to keep the 2 things separate
> because of technical constraints:

I meant from the "this is what it feels like to the user" :)  We have this elastic behavior when we get past the last screen, it let's me stretch it, it snaps back, doesn't bounce.  Ignoring completely the implementation details (Gecko/Gaia division, etc.) how is the feel/behavior of what we want for overscroll to be the same or different from what we have for the edge effect?

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 1 - Remove some unused methods in Axis and AsyncPanZoomController

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 2 - Factor out an Axis::GetFrameMetrics() helper

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 3 - Remove Axis::HasRoomToPan() as it effectively duplicates Axis::Scrollable()

Comment 17

[Botond Ballo [:botond]]

Attachment: Part 2 - Factor out an Axis::GetFrameMetrics() helper

Comment 18

[Botond Ballo [:botond]]

Attachment: Part 3 - Refactor Axis::HasRoomToPan() and Axis::Scrollable()

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8417665 [details] [diff] [review]
Part 1 - Remove some unused methods in Axis and AsyncPanZoomController

Drive by.

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 4 - Support overscrolling during panning

This patch implements support in Axis for being in an overscrolled state, and support in AsyncPanZoomController/APZCTreeManager for taking this state into account while panning (including handoff situations).

Flings don't know about overscroll yet, and the snap-back animation needs to be put into place. Also, there is no rendering difference yet between an overscrolled state and a non-overscrolled state.

Comment 21

[Botond Ballo [:botond]]

Attachment: Part 4 - Support overscrolling during panning

Comment 22

[Botond Ballo [:botond]]

Attachment: Part 5 - Support overscrolling during flinging

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 6 - Allow AsyncPanZoomAnimation::Sample() to queue deferred tasks even if the animation is ending

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 7 - Snap-back animation to relieve overscroll

This patch is currently a bit rough around the edges. Physics-wise the snap-back starts at a fixed velocity and then accelerates as the animation advances; we are likely to want to change or at least fine-tune it. The parameters are hard-coded or piggy-back off of vaguely related prefs for now; once we figure out what the exact model is, we can give the parameters their own prefs.

Updated list of things that still need to be done:
  - Actual rendering effects
  - Resistance while panning or flinging in overscroll
  - Cancelling overscroll on touch events

Comment 25

[Botond Ballo [:botond]]

Attachment: Part 4 - Support overscrolling during panning

Added to Part 4 something that I overlooked: scheduling a composite (and if appropriate, requesting a repaint) while panning into overscroll.

I also added a work around to bug 1009825, which I ran into while testing the patches.

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 8 - Visual overscroll effect: translate layer by overscroll amount

This patch allows visualizing the overscroll state that is maintained and manipulated by the other patches by translating the layer by the overscroll amount.

With the patches so far, you can now see overscrolling in action, though there is definitely polishing to do UX-wise.

Once we figure out what the actual overscroll effect that we want is, we can replace this patch with one that implements that.

Comment 27

[Botond Ballo [:botond]]

Attachment: Part 9 - Apply resistance to pans and flings in overscroll

This patch adds "resistance" to pans and flings in overscroll. The physics is subject to revision after feedback from UX; currently it simply tries to avoid allowing overscrolling by more that the composition length. (With the translation effect from Part 8, this amounts to avoiding scrolling the entire content out of view.)

Comment 28

[Botond Ballo [:botond]]

Attachment: Part 10 - Ignore touch/mouse events when in an overscrolled state

Asking for feedback to make sure I have the right idea about ignoring events beyond APZ. In particular, I'm wondering if the call sites in TabParent and MetroInput handle the "ignoreEvent = true" case correctly.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8422753 [details] [diff] [review]
Part 10 - Ignore touch/mouse events when in an overscrolled state

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

Thinking about this and what I plan to do in bug 1009733 I would rather line up the code so that the nsEventStatus return values of everything actually make sense per the docs (see http://mxr.mozilla.org/mozilla-central/source/widget/EventForwards.h#18)

This means:
- APZCTreeManager::ReceiveInputEvent would return:
-- nsEventStatus_eConsumeNoDefault in cases where the touch point hits an APZC in overscroll
-- nsEventStatus_eConsumeDefault in cases where the touch point hits an APZC not in overscroll
-- nsEventStatus_eIgnore in cases where the touch point doesn't hit any APZC at all

This would remove the need to pass around the out-param in TabParent, RenderFrameParent and Metro* code. Currently APZCTreeManager just returns whatever the APZC instance returns, but we can disconnect that because none of the APZCTM callers do anything useful with that return value anyway. Instead, they can now check and drop the event if APZCTM returns eConsumeNoDefault.

Does that sound reasonable?

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Oh sorry, missed the Metro code. Let me read through that and get back to you.

Comment 31

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> Currently APZCTreeManager just returns
> whatever the APZC instance returns, but we can disconnect that because none
> of the APZCTM callers do anything useful with that return value anyway.

There are two places in MetroInput where the return value from APZCTM::ReceiveInputEvent is used [1] [2]. However, these uses may be consistent with your interpretations - I don't think I understand the metro event handling code well enough to say.

On a different note: is the return value of APZC::ReceiveInputEvent used anywhere besides 
  - determining the return value of APZCTM::ReceiveInputEvent (the use we are proposing to remove)
  - in TestAsyncPanZoomController
If not, is there point in keeping that around for the sole benefit of test code?

> Does that sound reasonable?

Sounds good to me if there are no issues with the Metro call sites.

[1] http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp?from=MetroInput.cpp#1309
[2] http://dxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp?from=MetroInput.cpp#1433

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Looking at the call sites of ApzReceiveInputEvent in metro code I think the following would make sense with my suggested changes in comment 29:

- in HandleFirstTouchStartEvent
-- eConsumeNoDefault => just return early
-- eConsumeDefault or eIgnore => do what the code currently does

- in HandleFirstTouchMoveEvent
-- eConsumeNoDefault => just return early
-- eConsumeDefault => do what the code currently does for eConsumeNoDefault
-- eIgnore => do what the code currently does for !eConsumeNoDefault
(this effectively means adding an early return and replacing the eConsumeNoDefault check with eConsumeDefault instead)

- in DeliverNextQueuedTouchEvent: same as HandleFirstTouchMoveEvent

- in DispatchTouchCancel: no change; ignore return value.

As for the return value of APZC::ReceiveInputEvent, I'm happy to take it out but I'd rather do that after I deal with (or as part of) bug 1009733 in case I end up still needing it for something there.

Comment 33

[Botond Ballo [:botond]]

Attachment: Part 10 - Ignore touch/mouse events when in an overscrolled state

Revised approach for ignoring touch/mouse events when in an overscrolled state.

In addition to the changes Kats suggested, I realized that we want to ignore not just touch-start events while in an overscrolled state, but all the events in the touch block, so I had to add a new piece of state to APZC which tracks whether the current touch block is in an overscrolled APZC.

Comment 34

[Botond Ballo [:botond]]

Attachment: Part 7 - Snap-back animation to relieve scroll

Fixed a mistake in the Part 7 patch where I applied a CSS -> Screen scale in the wrong direction (as if it were a Screen -> CSS scale).

By the way, this mistake would have been caught if we had strongly-typed coordinate classes (bug 923512).

Comment 35

[Botond Ballo [:botond]]

Attachment: Part 11 - Overscroll rendering effect

This is a prototype of the overscroll rendering effect requested by UX (translation + shrink).

Proper handling of background color still needs to be implemented, and the physics can definitely use some tuning, particularly in the case where you are overscrolling at a corner and have overlapping x- and y-overscrolls, but I think this is enough for UX to start playing around with this.

Next step is to put the overscrolling functionality behind a developer pref and land it so UX can play with it.

Comment 36

[Botond Ballo [:botond]]

Attachment: Part 1 - Remove some unused methods in Axis and AsyncPanZoomController

Comment 37

[Botond Ballo [:botond]]

Attachment: Part 2 - Factor out an Axis::GetFrameMetrics() helper

Comment 38

[Botond Ballo [:botond]]

Attachment: Part 3 - Refactor Axis::HasRoomToPan() and Axis::Scrollable()

Comment 39

[Botond Ballo [:botond]]

Attachment: Part 4 - Support overscrolling during panning

Comment 40

[Botond Ballo [:botond]]

Attachment: Part 5 - Support overscrolling during flinging

Comment 41

[Botond Ballo [:botond]]

Attachment: Part 6 - Allow AsyncPanZoomAnimation::Sample() to queue deferred tasks even if the animation is ending

Comment 42

[Botond Ballo [:botond]]

Attachment: Part 7 - Snap-back animation to relieve overscroll

Comment 43

[Botond Ballo [:botond]]

Attachment: Part 8 - Apply resistance to pans and flings in overscroll

Comment 44

[Botond Ballo [:botond]]

Attachment: Part 9 - Ignore touch/mouse events when in an overscrolled state

Comment 45

[Botond Ballo [:botond]]

Attachment: Part 10 - Overscroll rendering effect (translation + shrinking)

Comment 46

[Botond Ballo [:botond]]

Attachment: [WIP] Part 11a - Put overscrolling behind a developer pref (gecko portion)

This doesn't actually work right now - setting the developer pref does not actually enable overscroll. I'm not sure why. I will debug it tomorrow and post it for review once I get it working, but I don't want to hold up the review of the other patches.

Comment 47

[Botond Ballo [:botond]]

Attachment: Part 11b - Put overscrolling behind a developer pref (gaia portion)

Comment 48

[Ivan Tsay (:ITsay)]

1. Add user story ID defined in Firefox OS product backlog Google sheet
2. Specify this is Firefox OS 2.0 feature by feature-b2g=2.0

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425895 [details] [diff] [review]
Part 3 - Refactor Axis::HasRoomToPan() and Axis::Scrollable()

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1562,5 @@
>  }
>  
>  bool AsyncPanZoomController::IsPannable() const {
>    ReentrantMonitorAutoEnter lock(mMonitor);
> +  return mX.CanScroll() || mY.CanScroll();

At some point we should make this code consistent with respect to using "pan" vs "scroll".

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425897 [details] [diff] [review]
Part 4 - Support overscrolling during panning

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1270,5 @@
>    }
>  
> +  return // If we consumed the entire displacement as a normal scroll, great.
> +         IsZero(overscroll)
> +

I'd rather break this up into separate if statements with early returns than this long return statement. It's creative with respect to formatting, but also "different and unexpected" which generally means "harder to read"

@@ +1297,5 @@
> +  if (yCanScroll) {
> +    mY.OverscrollBy(aOverscroll.y);
> +  }
> +  if (xCanScroll || yCanScroll) {
> +    ScheduleCompositeAndMaybeRepaint();

In general we shouldn't needing to do a repaint when we are just adjusting overscroll, since it's a compositor-side effect. However, it should also be the case that the dupe-filtering code in RequestContentRepaint should detect that and just early-return. Can you verify that is the case?

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +881,5 @@
> +   * If we are pannable, 'aOverscroll' is added to any existing overscroll,
> +   * and the function returns true.
> +   * Otherwise, nothing happens and the function return false.
> +   */
> +  bool OverscrollBy(const CSSPoint& aOverscroll);

If you're putting this function in the "scroll handoff" section of the .h file then I'd like to see the section comment updated to say "... building the scroll handoff chain and overscroll ..."

Or you could move this function into the uncategorized part of the .h file.

::: gfx/layers/apz/src/Axis.cpp
@@ +76,2 @@
>    // If this displacement will cause an overscroll, throttle it. Can potentially
>    // bring it to 0 even if the velocity is high.

While you're here, can you update this comment? The velocity is irrelevant here. Maybe just replace the entire comment with something like "split the remaining displacement into normal scroll and overscroll"

::: gfx/layers/apz/src/Axis.h
@@ +16,5 @@
>  namespace layers {
>  
>  const float EPSILON = 0.0001f;
>  
> +class FrameMetrics;

Forward declaration shouldn't be needed here. Maybe it belongs in part 2?

@@ +88,5 @@
> +
> +  /**
> +   * Return the amount of overscroll on this axis, in CSS pixels.
> +   */
> +  float GetOverscroll() const;

This function isn't used in this patch, but presumably it will be used later.

Comment 51

[Kartikaya Gupta (email:[e-mail])]

CC'ing mstange as well as an FYI since some of these patches will require him to rebase the OS X work.

Comment 52

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425899 [details] [diff] [review]
Part 5 - Support overscrolling during flinging

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +279,5 @@
>     * hand off overscroll from a fling.
>     * @param aApzc the APZC that is handing off the fling
>     * @param aVelocity the current velocity of the fling, in |aApzc|'s screen
>     *                  pixels per millisecond
> +   * Returns whether another APZC accepted the handed-off fling. The caller

s/Returns whether/Returns true if/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +356,5 @@
>  class FlingAnimation: public AsyncPanZoomAnimation {
>  public:
> +  FlingAnimation(AsyncPanZoomController& aApzc,
> +                 bool aApplyAcceleration,
> +                 bool aAllowOverscroll = false)

I would rather make this parameter non-optional, but I don't feel too strongly about it. In general I prefer non-optional parameters unless there are a lot of call sites to change.

@@ +1452,3 @@
>  
> +      // Restore the velocity of the fling, which was zeroed out by
> +      // AdjustDisplacement()p

dangling "p"

@@ +1461,5 @@
> +
> +      // We may have reached the end of the scroll range along one axis but
> +      // not the other. In such a case we only want to hand off the relevant
> +      // component of the fling.
> +      if (FuzzyEqualsMultiplicative(overscroll.x, 0.0f)) {

I suppose this fuzzyequals call will only return true if overscroll.x is exactly zero :( It should probably be a FuzzyEqualsAdditive, but we can deal with it after bug 1009825 is resolved.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +759,4 @@
>  
>    /* ===================================================================
>     * The functions and members in this section are used to manage
>     * fling animations.

... and fling overscroll. (I guess that's implied but I'd rather make it explicit)

@@ +765,5 @@
>    /**
>     * Take over a fling with the given velocity from another APZC. Used for
> +   * during overscroll handoff for a fling. If we are not pannable, calls
> +   * mTreeManager->HandOffFling() to hand the fling off further.
> +   * Returns whether any APZC (whether this one or one further in the handoff

s/Returns whether/Returns true if/

Looking at this code I think it should be possible to rearrange it to reduce stack depth. Right now the call stack looks like APZCTM::HandOffFling -> APZC::TakeOverFling -> APZCTM::HandOffFling -> ... -> APZC::TakeOverFling
Instead we could have a loop in APZCTM::HandOffFling that iterates through the handoff chain, calling TakeOverFling on each one and aborting if one of them returns true.

This could be done as a follow-up mentored bug or something, as it's just refactoring.

@@ +781,5 @@
> +  // Used by FlingAnimation::Sample() to handle the case where a fling gets
> +  // us to the end of our scroll range. Calls mTreeManager->HandOffFling()
> +  // to try to hand off the fling to an APZC later in the handoff chain. If
> +  // none want it, this APZC will continue the fling, entering an overscrolled
> +  // state.

This comment seems to just be reiterating the code. I think a slightly higher-level comment would be more appropriate. For example, "Deal with overscroll resulting from a fling animation. This is only ever called on APZC instances that were actually performaing a fling, but can pass the overscroll up the handoff chain if necessary."

Comment 53

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425902 [details] [diff] [review]
Part 7 - Snap-back animation to relieve overscroll

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +251,5 @@
>   * The multiplier we apply to the displayport size if it is not skating (see
>   * documentation for the skate size multipliers above).
> + *
> + * "apz.overscroll.snap_back_accel"
> + * Amount of acceleration applied during the snap-back animation.

Move these up so they are sorted alphabetically by pref name. Also for this acceleration one update the comment to say that the velocity is multiplied by 1+accel every millisecond.

::: gfx/layers/apz/src/Axis.cpp
@@ +114,5 @@
> +
> +bool Axis::SampleSnapBack(const TimeDuration& aDelta) {
> +  // Accelerate the snap-back as time goes on.
> +  mVelocity *= pow(1.0f + gfxPrefs::APZSnapBackAcceleration(), float(aDelta.ToMilliseconds()));
> +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();

Technically this calculation doesn't result in a smooth curve because you're using assuming a fixed velocity over the delta when in fact it should be a slowly increasing velocity. I don't really care that much as long as it looks smooth but it might be worth a comment here in case somebody complains that the snapback doesn't look right.

@@ +117,5 @@
> +  mVelocity *= pow(1.0f + gfxPrefs::APZSnapBackAcceleration(), float(aDelta.ToMilliseconds()));
> +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();
> +  float cssDisplacement = screenDisplacement / GetFrameMetrics().GetZoom().scale;
> +  if (mOverscroll > 0) {
> +    mOverscroll = std::max(mOverscroll + cssDisplacement, 0.0f);

I'm a bit concerned that bugs in the code might result in situations where mVelocity ends up increasing the overscroll instead of decreasing it. It would be good to add a warning for that situation (i.e. if cssDisplacement > 0 here) so that we can diagnose it if it happens.

@@ +118,5 @@
> +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();
> +  float cssDisplacement = screenDisplacement / GetFrameMetrics().GetZoom().scale;
> +  if (mOverscroll > 0) {
> +    mOverscroll = std::max(mOverscroll + cssDisplacement, 0.0f);
> +    // Overscroll relieved, do not continue annimation.

s/annimation/animation/, here and below

::: gfx/thebes/gfxPrefs.h
@@ +130,5 @@
>    DECL_GFX_PREF(Live, "apz.x_stationary_size_multiplier",      APZXStationarySizeMultiplier, float, 3.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(Once, "apz.overscroll.snap_back_accel",        APZSnapBackAcceleration, float, 0.002f);
> +  DECL_GFX_PREF(Live, "apz.overscroll.snap_back_init_vel",     APZSnapBackInitialVelocity, float, 1.0f);

These also need to be moved up into sort order

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425905 [details] [diff] [review]
Part 9 - Ignore touch/mouse events when in an overscrolled state

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

It seems to me that the changes in APZCTM would be cleaner if you got rid of the out-param and just queried the return apzc instance to see if it's in overscroll. You'd have to do that at every call site, but it could be refactored into a helper function that takes an APZC and returns the return value. Thoughts? Minusing for now (I didn't do a detailed review of the APZCTM changes but I can if you convince me this way is better).

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +418,5 @@
>          MultiTouchInput inputForApzc(multiTouchInput);
>          for (size_t i = 0; i < inputForApzc.mTouches.Length(); i++) {
>            ApplyTransform(&(inputForApzc.mTouches[i].mScreenPoint), transformToApzc);
>          }
>          result = mApzcForInputBlock->ReceiveInputEvent(inputForApzc);

Remove the assignment to result here

::: widget/windows/winrt/MetroInput.cpp
@@ +1286,5 @@
>    nsEventStatus apzcStatus = nsEventStatus_eIgnore;
>  
>    WidgetTouchEvent transformedEvent(*aEvent);
>    DUMP_TOUCH_IDS("APZC(2)", aEvent);
> +  apzcStatus = mWidget->ApzReceiveInputEvent(&transformedEvent, &mTargetAPZCGuid, &ignoreEvent);

&ignoreEvent?

@@ +1422,5 @@
>      return;
>    }
>  
>    DUMP_TOUCH_IDS("APZC(3)", event);
> +  status = mWidget->ApzReceiveInputEvent(event, nullptr, &ignoreEvent);

Ditto

@@ +1475,5 @@
>      return;
>    }
>    if (mContentConsumingTouch) {
>      DUMP_TOUCH_IDS("APZC(4)", &touchEvent);
> +    mWidget->ApzReceiveInputEvent(&touchEvent, nullptr, nullptr);

Extra nullptr

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425907 [details] [diff] [review]
Part 10 - Overscroll rendering effect (translation + shrinking)

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

Some of this code will probably change but it's ok for now.

Comment 56

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8425908 [details] [diff] [review]
[WIP] Part 11a - Put overscrolling behind a developer pref (gecko portion)

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

::: gfx/thebes/gfxPrefs.h
@@ +129,5 @@
>    DECL_GFX_PREF(Live, "apz.x_skate_size_multiplier",           APZXSkateSizeMultiplier, float, 1.5f);
>    DECL_GFX_PREF(Live, "apz.x_stationary_size_multiplier",      APZXStationarySizeMultiplier, float, 3.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.overscroll.enabled",                APZOverscrollEnabled, bool, false);

Move this up, alphabetical

Comment 57

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #46)
> Created attachment 8425908 [details] [diff] [review]
> [WIP] Part 11a - Put overscrolling behind a developer pref (gecko portion)
> 
> This doesn't actually work right now - setting the developer pref does not
> actually enable overscroll. I'm not sure why.

I figured out the problem: I had quotes around "false" in b2g/app/b2g.js. I guess that made it a string pref instead of a bool pref and did not play nicely with gfxPrefs trying to read it as a bool.

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Doug, I'd like you to also take a look at these patches and familiarize yourself with how this works. Botond will be away for a good chunk of June and this code may need to be tuned and adjusted so having another person familiar with the code would help with reviews and load balancing. Also this might affect the stuff you're doing with the keyboard overlay.

Comment 59

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #50)
> @@ +1297,5 @@
> > +  if (yCanScroll) {
> > +    mY.OverscrollBy(aOverscroll.y);
> > +  }
> > +  if (xCanScroll || yCanScroll) {
> > +    ScheduleCompositeAndMaybeRepaint();
> 
> In general we shouldn't needing to do a repaint when we are just adjusting
> overscroll, since it's a compositor-side effect. However, it should also be
> the case that the dupe-filtering code in RequestContentRepaint should detect
> that and just early-return. Can you verify that is the case?

Good point! RequestContentRepaint does indeed early-return, but it seems unnecessary to call it even so. What do you think about just calling ScheduleComposite() here?

> ::: gfx/layers/apz/src/Axis.h
> @@ +16,5 @@
> >  namespace layers {
> >  
> >  const float EPSILON = 0.0001f;
> >  
> > +class FrameMetrics;
> 
> Forward declaration shouldn't be needed here. Maybe it belongs in part 2?

Yep, thanks for catching it.

> @@ +88,5 @@
> > +
> > +  /**
> > +   * Return the amount of overscroll on this axis, in CSS pixels.
> > +   */
> > +  float GetOverscroll() const;
> 
> This function isn't used in this patch, but presumably it will be used later.

It is used in the "visual overscroll effect" patch, but it seemed strange to put it there, especially as I was at the time playing around with different versions of the effect. Really this Part 4 patch could have been separated into "add overscroll support to Axis" and "support overscrolling while panning" patches, and then this would have more clearly belonged to the "add overscroll support to Axis" patch.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #52)
> @@ +1461,5 @@
> > +
> > +      // We may have reached the end of the scroll range along one axis but
> > +      // not the other. In such a case we only want to hand off the relevant
> > +      // component of the fling.
> > +      if (FuzzyEqualsMultiplicative(overscroll.x, 0.0f)) {
> 
> I suppose this fuzzyequals call will only return true if overscroll.x is
> exactly zero :( It should probably be a FuzzyEqualsAdditive, but we can deal
> with it after bug 1009825 is resolved.

I'll change it to Additive with a 1e-3 additive as per the discussion in that bug.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> ::: gfx/layers/apz/src/Axis.cpp
> @@ +114,5 @@
> > +
> > +bool Axis::SampleSnapBack(const TimeDuration& aDelta) {
> > +  // Accelerate the snap-back as time goes on.
> > +  mVelocity *= pow(1.0f + gfxPrefs::APZSnapBackAcceleration(), float(aDelta.ToMilliseconds()));
> > +  float screenDisplacement = mVelocity * aDelta.ToMilliseconds();
> 
> Technically this calculation doesn't result in a smooth curve because you're
> using assuming a fixed velocity over the delta when in fact it should be a
> slowly increasing velocity. I don't really care that much as long as it
> looks smooth but it might be worth a comment here in case somebody complains
> that the snapback doesn't look right.

I modelled the calculation after how we apply friction to flings at http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.cpp?from=Axis.cpp#118. Does that also have the same issue of not being smooth, or are the two situations different in some way?

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #54)
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +1286,5 @@
> >    nsEventStatus apzcStatus = nsEventStatus_eIgnore;
> >  
> >    WidgetTouchEvent transformedEvent(*aEvent);
> >    DUMP_TOUCH_IDS("APZC(2)", aEvent);
> > +  apzcStatus = mWidget->ApzReceiveInputEvent(&transformedEvent, &mTargetAPZCGuid, &ignoreEvent);
> 
> &ignoreEvent?

Whoops, those are leftovers from an earlier approach where APZCTM::ReceiveInputEvent() would pass out a boolean "ignore this event" out-parameter (before we repurposed the return value for this purpose).

Comment 60

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #59)
> Good point! RequestContentRepaint does indeed early-return, but it seems
> unnecessary to call it even so. What do you think about just calling
> ScheduleComposite() here?

Sounds good.

> > This function isn't used in this patch, but presumably it will be used later.
> 
> It is used in the "visual overscroll effect" patch, but it seemed strange to
> put it there, especially as I was at the time playing around with different
> versions of the effect. Really this Part 4 patch could have been separated
> into "add overscroll support to Axis" and "support overscrolling while
> panning" patches, and then this would have more clearly belonged to the "add
> overscroll support to Axis" patch.

Either way is fine. The patch queue was very well split up and it was pretty easy to review :)

> > Technically this calculation doesn't result in a smooth curve because you're
> > using assuming a fixed velocity over the delta when in fact it should be a
> > slowly increasing velocity. I don't really care that much as long as it
> > looks smooth but it might be worth a comment here in case somebody complains
> > that the snapback doesn't look right.
> 
> I modelled the calculation after how we apply friction to flings at
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/Axis.
> cpp?from=Axis.cpp#118. Does that also have the same issue of not being
> smooth, or are the two situations different in some way?

You're right, that code has the same issue. In both cases I guess it won't really be noticeable to the unaided eye.

Comment 61

[Botond Ballo [:botond]]

Attachment: Part 2 - Factor out an Axis::GetFrameMetrics() helper

Updated to address review comments. Carrying r+.

Comment 62

[Botond Ballo [:botond]]

Attachment: Part 4 - Support overscrolling during panning

Updated to address review comments. 

Also revised FuzzyEquals* usage as per bug 1009825 discussion. Re-requesting review for the introduction of COORDINATE_EPSILON.

Comment 63

[Botond Ballo [:botond]]

Attachment: Part 5 - Support overscrolling during flinging

Updated to address review comments. Carrying r+.

Comment 64

[Botond Ballo [:botond]]

Attachment: Part 7 - Snap-back animation to relieve overscroll

Updated to address review comments. Carrying r+.

Comment 65

[Botond Ballo [:botond]]

Attachment: Part 9 - Ignore touch/mouse events when in an overscrolled state

As discussed with Kats on IRC, we are keeping out parameter in GetTargetAPZC() for now.

The exact behaviour of input events in an overscrolled state is subject to change after UX feedback.

Comment 66

[Botond Ballo [:botond]]

Attachment: Part 11a - Put overscrolling behind a developer pref (gecko portion)

Comment 67

[Botond Ballo [:botond]]

Try pushes:

Emulator, build + tests: https://tbpl.mozilla.org/?tree=Try&rev=0654aa42aa0b
Other platforms, build only: https://tbpl.mozilla.org/?tree=Try&rev=e9c82a6514b1

Comment 68

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> Comment on attachment 8425895 [details] [diff] [review]
> Part 3 - Refactor Axis::HasRoomToPan() and Axis::Scrollable()
> 
> Review of attachment 8425895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1562,5 @@
> >  }
> >  
> >  bool AsyncPanZoomController::IsPannable() const {
> >    ReentrantMonitorAutoEnter lock(mMonitor);
> > +  return mX.CanScroll() || mY.CanScroll();
> 
> At some point we should make this code consistent with respect to using
> "pan" vs "scroll".

Filed bug 1014276.

Comment 69

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #35)
> Proper handling of background color still needs to be implemented

Filed bug 1014280 for this as a follow-up, as I intend to land the patches without this, to allow UX to play around with them sooner.

Comment 70

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #52)
> Looking at this code I think it should be possible to rearrange it to reduce
> stack depth. Right now the call stack looks like APZCTM::HandOffFling ->
> APZC::TakeOverFling -> APZCTM::HandOffFling -> ... -> APZC::TakeOverFling
> Instead we could have a loop in APZCTM::HandOffFling that iterates through
> the handoff chain, calling TakeOverFling on each one and aborting if one of
> them returns true.
> 
> This could be done as a follow-up mentored bug or something, as it's just
> refactoring.

I'll file a mentored follow-up bug when this lands.

Comment 71

[Botond Ballo [:botond]]

Attachment: Part 7 - Snap-back animation to relieve overscroll

(In reply to Botond Ballo [:botond] from comment #67)
> Try pushes:
> 
> Emulator, build + tests: https://tbpl.mozilla.org/?tree=Try&rev=0654aa42aa0b
> Other platforms, build only:
> https://tbpl.mozilla.org/?tree=Try&rev=e9c82a6514b1

Had -Werror failures for not handling the SNAP_BACK state in OnTouchMove and OnTouchEnd. Fixed in this updated patch. 

Re-requesting review for these added bits. Technically, their correctness depends on code in the Part 9 patch. That possibly suggests that the part 9 patch should come before this one, but I don't know if we care enough.

ReTrying:

https://tbpl.mozilla.org/?tree=Try&rev=203b893bafbc
https://tbpl.mozilla.org/?tree=Try&rev=f562c7a81433

Comment 72

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8426594 [details] [diff] [review]
Part 4 - Support overscrolling during panning

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +245,5 @@
>     *   represent different displacements in untransformed coordinates.
>     * |aOverscrollHandoffChainIndex| is the next position in the overscroll
>     *   handoff chain that should be scrolled.
>     *
> +   * Returns whether or not some APZC accepted the scroll and scrolled.

Returns true if

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +853,5 @@
>     * |aOverscrollHandoffChainIndex| is used by the tree manager to keep track
>     * of which APZC to hand off the overscroll to; this function increments it
>     * and passes it on to APZCTreeManager::DispatchScroll() in the event of
>     * overscroll.
> +   * Returns whether or not either this APZC, or an APZC further down the

Returns true if

::: gfx/layers/apz/src/Axis.h
@@ +19,5 @@
>  
> +// Epsilon to be used when comparing 'float' coordinate values
> +// with FuzzyEqualsAdditive. The rationale is that 'float' has 7 decimal
> +// digits of precision, and coordinate values are typically no larger than
> +// thousands.

I have seen coordinate values in the 10,000 order of magnitude (for example, in the contacts app with a medium workload). I'd feel more comfortable dropping this to 1e-2. Given that GetOrigin() et al return a CSS pixel length, and the smallest possible legitimate difference in page coordinates is 1 app unit (~0.02 CSS pixels) we should be safe with a 0.01 fuzz.

Comment 73

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8426672 [details] [diff] [review]
Part 7 - Snap-back animation to relieve overscroll

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

Wrong patch

Comment 74

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8426602 [details] [diff] [review]
Part 9 - Ignore touch/mouse events when in an overscrolled state

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +612,2 @@
>    }
> +  return inOverscrolledApzc ? nsEventStatus_eConsumeNoDefault : nsEventStatus_eIgnore;

I think for consistency we should return nsEventStatus_eConsumeDoDefault in the case where !inOverscrolledApzc && apzc != nullptr. (i.e. basically using the same nested ternary expression you used everywhere else)

Comment 75

[Botond Ballo [:botond]]

Attachment: Part 7 - Snap-back animation to relieve overscroll

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #73)
> Wrong patch

Whoops! Here is the correct patch. The thing I'm requesting re-review for is the added cases in APZC::OnTouchMove() and OnTouchEnd().

Comment 76

[Botond Ballo [:botond]]

Attachment: Part 4 - Support overscrolling during panning

Updated to address review comments. Carrying r+.

Comment 77

[Botond Ballo [:botond]]

Attachment: Part 9 - Ignore touch/mouse events when in an overscrolled state

Updated to address review comments. Carrying r+.

Comment 78

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8427094 [details] [diff] [review]
Part 7 - Snap-back animation to relieve overscroll

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

LGTM

Comment 79

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #71)
> ReTrying:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=203b893bafbc
> https://tbpl.mozilla.org/?tree=Try&rev=f562c7a81433

The Try builds had some gtest failures. Investigating them led me to discover three bugs:

 1) The tasks in AsyncPanZoomAnimation::mDeferredTasks were neither posted
    to a message loop (which destroys a Task after it runs it), nor
    explicitly destroyed, and as a result, they - along with anything they
    hold a strong reference to - were being leaked.

    This bug has been around since mDeferredTasks was introduced in
    bug 965871, but it didn't cause any failures until the patches in this
    added additional uses of mDeferredTasks.

    The fix is to delete the tasks after running them in
    SampleContentTransformForFrame(). I'll amend the Part 6 patch to include
    this fix.

    Kudos to gtests for catching this with a "mock object leaked" error!

 2) My modifications to APZCTreeManager::GetAPZCAtPoint() in the Part 9 patch
    had a bug where I changed a 'return' from a loop into an assignment to a 
    return variable, but forgot to add a 'break'. This was causing
    AsyncPanZoomControllerTest.HitTesting1 to fail.
    I'll amend the Part 9 patch to fix this.

 3) ApzcPan() in TestAsyncPanZoomController.cpp was explicitly building the
    overscroll handoff chain, but failing to explicitly clear it. While this
    did not manifest in failures, it has the potential to cause a leak, so
    I will fix it in a Part 12 patch.

Comment 80

[Botond Ballo [:botond]]

Attachment: Part 6 - Allow AsyncPanZoomAnimation::Sample() to queue deferred tasks even if the animation is ending

Updated patch to delete the deferred tasks after running them, fixing a gtest failure. Carrying r+.

Comment 81

[Botond Ballo [:botond]]

Attachment: Part 9 - Ignore touch/mouse events when in an overscrolled state

Updated patch to fix a bug it introduced in GetAPZCAtPoint() which was causing AsyncPanZoomControllerTester.HitTesting1 to fail. Carrying r+.

Comment 82

[Botond Ballo [:botond]]

Attachment: Part 12 - Have APZ gtests clear the overscroll handoff chain when appropriate

Comment 83

[Botond Ballo [:botond]]

Another Try:

https://tbpl.mozilla.org/?tree=Try&rev=b17586b9550a
https://tbpl.mozilla.org/?tree=Try&rev=825b96fc5d12

Comment 84

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #79)
>     The fix is to delete the tasks after running them in
>     SampleContentTransformForFrame(). I'll amend the Part 6 patch to include
>     this fix.

It would be better to file a separate bug and patch for this and land it first, because it might need uplifting. If I understand correctly, the code in 1.4 is going to leak a Task every time we try to hand off a fling, which is going to be pretty frequent.

Comment 85

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #84)
> (In reply to Botond Ballo [:botond] from comment #79)
> >     The fix is to delete the tasks after running them in
> >     SampleContentTransformForFrame(). I'll amend the Part 6 patch to include
> >     this fix.
> 
> It would be better to file a separate bug and patch for this and land it
> first, because it might need uplifting. If I understand correctly, the code
> in 1.4 is going to leak a Task every time we try to hand off a fling, which
> is going to be pretty frequent.

Good point! I filed bug 1015331.

Comment 86

[Botond Ballo [:botond]]

Comment on attachment 8425909 [details] [diff] [review]
Part 11b - Put overscrolling behind a developer pref (gaia portion)

Flagging some other Gaia::System peers for review of developer pref.

Comment 87

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8425909 [details] [diff] [review]
Part 11b - Put overscrolling behind a developer pref (gaia portion)

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

r+ if you had the settings into build/config/common-settings.js too.

Comment 88

[Botond Ballo [:botond]]

Attachment: Part 11b - Put overscrolling behind a developer pref (gaia portion)

Updated to address review comments. Carrying r+.

Comment 89

[Botond Ballo [:botond]]

Landed gecko patches:
  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=bda83125b312

Comment 90

[Botond Ballo [:botond]]

Pull request for gaia patch:
  https://github.com/mozilla-b2g/gaia/pull/19654

Comment 91

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/452c17e72618
https://hg.mozilla.org/mozilla-central/rev/badbdb955bf8
https://hg.mozilla.org/mozilla-central/rev/a4ed587492a9
https://hg.mozilla.org/mozilla-central/rev/ca51a664b266
https://hg.mozilla.org/mozilla-central/rev/7d0fa6e57949
https://hg.mozilla.org/mozilla-central/rev/03314207fd6a
https://hg.mozilla.org/mozilla-central/rev/a1121f0918aa
https://hg.mozilla.org/mozilla-central/rev/302af2ad18b2
https://hg.mozilla.org/mozilla-central/rev/aacf3431da8e
https://hg.mozilla.org/mozilla-central/rev/3b86d4a9c9ef
https://hg.mozilla.org/mozilla-central/rev/f74e8b3f674d
https://hg.mozilla.org/mozilla-central/rev/bda83125b312

Comment 92

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Gaia master: https://github.com/mozilla-b2g/gaia/commit/63c975a2a4cc7b4cee6b77b0f66f07d6b3c8b04e

Comment 94

[Doug Sherk (:drs) (inactive)]

Okay, I read through this for a couple of hours and I think I mostly get it now.

Comment 95

[Kartikaya Gupta (email:[e-mail])]

Awesome, thanks!

Comment 96

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.

Comment 97

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #70)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #52)
> > Looking at this code I think it should be possible to rearrange it to reduce
> > stack depth. Right now the call stack looks like APZCTM::HandOffFling ->
> > APZC::TakeOverFling -> APZCTM::HandOffFling -> ... -> APZC::TakeOverFling
> > Instead we could have a loop in APZCTM::HandOffFling that iterates through
> > the handoff chain, calling TakeOverFling on each one and aborting if one of
> > them returns true.
> > 
> > This could be done as a follow-up mentored bug or something, as it's just
> > refactoring.
> 
> I'll file a mentored follow-up bug when this lands.

Filed bug 1056367. Sorry for taking so long.