1039992 - Refactor the lifetime of the overscroll handoff chain

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Because APZCs need to wait for web content to preventDefault (or not) some touch events, they queue up the events internally and then process them later. However, the APZCTreeManager maintains the overscroll handoff chain based on input events as they are arriving to the APZC.

Theoretically, this means that it is possible for the two to get out of sync, and for touch events to be processed after the overscroll handoff chain has been destroyed. For example, consider a page with a slow touchstart listener that runs for 300ms. Now imagine a person does a super-quick scroll so that their finger goes down, moves, and comes back up in less than 300ms.

In this case, the APZC will not process the events for at least 300ms (because of the listeners) but by that time the APZCTM will have already received the touch-up and destroyed the overscroll handoff chain. This would result in scrolling not working.

A related problem is that the APZCTM only maintains a single overscroll handoff chain, while in fact there might be multiple APZCs on the page (say side-by-side overflow:scroll divs) that have simultaneous flings going and might require handing off overscroll.

One possible way to deal with this would be to move the overscroll handoff chain from the APZCTM into the APZC instance itself, and have it built/torndown based on when the touch events are actually processed. We'd have to carefully consider how this would impact things like PANGESTURE_INPUT events as well.

Comment 1

[Botond Ballo [:botond]]

I'm going to appropriate this bug for the overhaul of how we store the overscroll handoff chain that Kats and I discussed on IRC. Very briefly, the idea is:

 1) Move the handoff chain into the the APZC which is receiving the touch events,
    and build it / destroy it when the touch-start / touch-end events are processed,
    rather than when they are queued for processing. That will solve the particular
    problems mentioned in comment #0.

 2) Have the APZC store by handoff chain by nsRefPtr, and have the fling and snap-back
    animations keep it alive (by also keeping an nsRefPtr to it), rather than
    re-computing it when they need it. This will solve the issue described in 
    https://bugzilla.mozilla.org/show_bug.cgi?id=1042974#c2.

I will implement (1) and (2) at the same time because separating them out would involve a fair bit of work with little gain.

Comment 2

[Botond Ballo [:botond]]

Attachment: bug1039992.patch

This patch implements the proposed refactoring. It passes manual testing and our existing gtests. Needs some cleanup/commenting and possibly additional gtests before review.

Comment 3

[Botond Ballo [:botond]]

Attachment: bug1039992.patch

Cleaned up the patch. Gtests to come in a second patch.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8472617 [details] [diff] [review]
bug1039992.patch

Might as well get this reviewed while I work on the gtests.

I'm asking for review from:

  kats:    For the whole patch.

  bjacob:  For the NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING
           macro in OverscrollHandoffChain.h.

  mstange: For the changes related to pan gestures. Basically,
           pan gestures get their own overscroll handoff chain
           rather than sharing the ones used by touch events
           (because those are per-{touch block} now), and the
           building and clearing of the chain has been moved
           from APZCTM to APZC.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8472617 [details] [diff] [review]
bug1039992.patch

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

R+ to NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8472617 [details] [diff] [review]
bug1039992.patch

The pan related changes look good to me.

Comment 7

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

Comment on attachment 8472617 [details] [diff] [review]
bug1039992.patch

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

Overall I really like this patch! It makes things a lot simpler conceptually. I have some comments below. I think we can also simplify the code a bit more by moving more stuff into the OverscrollHandoffChain implementation. Things like CancelAnimationsForOverscrollHandoffChain, FlushRepaintsForOverscrollHandoffChain, etc (anything that walks the entire handoff chain) could be moved to the handoff chain class for better cohesiveness.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ -431,5 @@
>                                                              &inOverscrolledApzc);
>        if (apzc) {
> -        if (panInput.mType == PanGestureInput::PANGESTURE_START ||
> -            panInput.mType == PanGestureInput::PANGESTURE_MOMENTUMSTART) {
> -          BuildOverscrollHandoffChain(apzc);

Yay code deletion \o/

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +840,5 @@
>  {
> +  AssertOnControllerThread();
> +  MOZ_ASSERT(aOverscrollHandoffChain.Length() > 0);
> +  for (uint32_t i = 0; i < aOverscrollHandoffChain.Length(); i++) {
> +    if (AsyncPanZoomController* item = aOverscrollHandoffChain.GetApzcAtIndex(i)) {

item should never be null here, since you never insert a null entry into the chain and they're stored as refptrs. So we shouldn't need this check, and I'd rather remove it to catch errors.

@@ +853,5 @@
> +  AssertOnControllerThread();
> +  const OverscrollHandoffChain& overscrollHandoffChain = *CurrentTouchBlock()->GetOverscrollHandoffChain();
> +  MOZ_ASSERT(overscrollHandoffChain.Length() > 0);
> +  for (uint32_t i = 0; i < overscrollHandoffChain.Length(); i++) {
> +    if (AsyncPanZoomController* item = overscrollHandoffChain.GetApzcAtIndex(i)) {

Ditto on null check

@@ +874,5 @@
> +  // See whether any APZC in the handoff chain starting from |aApzc|
> +  // has room to be panned.
> +  for (uint32_t j = i; j < aOverscrollHandoffChain.Length(); ++j) {
> +    if (aOverscrollHandoffChain.GetApzcAtIndex(j)->IsPannable()) {
> +      return true;

I note here you're not checking for null :)

@@ +927,5 @@
>    if (aEvent.AsMultiTouchInput().mType == MultiTouchInput::MULTITOUCH_START) {
>      block = StartNewTouchBlock(false);
> +    if (!block) {
> +      // This APZC IsDestroyed(), don't do further processing.
> +      return nsEventStatus_eIgnore;

Hm.. this bring up an interesting edge case. The touch event will be discarded in the APZ code but that touch event will still get dispatched to content. In general this can cause the touch-block-balance to get out of whack, because content will still send a ContentReceivedTouch back. Arguably this APZC is destroyed and so will never receive that ContentReceivedTouch but we might also have a new APZC with the same scrollablelayerguid by that point and that would receive it instead.

I think this is probably a pre-existing issue in the code. Still, it might be better to have APZC::BuildOverscrollHandoffChain just return new "empty" handoff chain instead of nullptr. (I don't recall if the handoff chain includes this APZC instance; if so "empty" would mean size 1). It would make the rest of the code to worry less about cases like this and generally make things more consistent. Thoughts?

@@ +1394,5 @@
>    APZC_LOG("%p got a pan-maybegin in state %d\n", this, mState);
>  
>    mX.StartTouch(aEvent.mPanStartPoint.x, aEvent.mTime);
>    mY.StartTouch(aEvent.mPanStartPoint.y, aEvent.mTime);
> +  CancelAnimationsForOverscrollHandoffChain(*mPanGestureState->GetOverscrollHandoffChain());

I think this gets called before OnPanBegin, so it's possible mPanGestureState is nullptr here.

@@ +1821,5 @@
> +
> +void AsyncPanZoomController::SnapBackOverscrolledApzc(const nsRefPtr<const OverscrollHandoffChain>& aOverscrollHandoffChain) {
> +  for (uint32_t i = 0; i < aOverscrollHandoffChain->Length(); ++i) {
> +    AsyncPanZoomController* apzc = aOverscrollHandoffChain->GetApzcAtIndex(i);
> +    if (apzc && !apzc->IsDestroyed() && apzc->SnapBackIfOverscrolled()) {

apzc should never be null here, so no need to check for it (same as my above comment)

@@ +2771,5 @@
>  {
> +  nsRefPtr<const OverscrollHandoffChain> overscrollHandoffChain = BuildOverscrollHandoffChain();
> +  if (!overscrollHandoffChain) {
> +    // This APZC IsDestroyed(), no point doing further processing.
> +    return nullptr;

See my above comment. I would rather not have this special-cased behaviour, and just have BuildOverscrollHandoffChain always return non-null.

@@ +2848,5 @@
> +  return (mState == PANNING || mState == PANNING_LOCKED_X || mState == PANNING_LOCKED_Y);
> +}
> +
> +bool AsyncPanZoomController::IsInTouchingState() const {
> +  // Any state where we expec there to be a touch point.

s/expec/expect/. But this function doesn't appear to be used anywhere, can we get rid of it?

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +789,5 @@
>     * The functions and members in this section are used to manage
> +   * pan gestures.
> +   */
> +
> +  UniquePtr<PanGestureState> mPanGestureState;

Add a private: on this. I'd rather have each section not rely on visibility left over from the previous section.

::: gfx/layers/apz/src/GestureState.h
@@ +17,5 @@
> +class OverscrollHandoffChain;
> +
> +/**
> + * A base class that stores state common to various gestures.
> + * Currently, it just stores the overscroll handoff chain.

This feels a bit like overgeneralization. For one thing, there is no real state associated with the pan gesture. And for another, a block of touch events isn't really a "gesture". At least not the way we use the term in the rest of the code.

I think I would prefer calling this InputBlockState, having TouchBlockState extend from it, and get rid of PanGestureState entirely. Code that currently uses PanGestureState can use InputBlockState directly, and we can add other subclasses later if needed. Thoughts?

::: gfx/layers/apz/src/OverscrollHandoffChain.h
@@ +28,5 @@
> + * mutable versions.
> + */
> +#define NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING(_class)                 \
> +public:                                                                       \
> +  NS_METHOD_(MozExternalRefCountType) AddRef(void) const {                    \

I don't really like that this macros is defined here. They probably belong in nsISupportsImpl.h or some other reusable header file. I'm ok with it landing here for the moment but I would like a follow-up bug to move them out. Also, as long as they're in this header file it would be good to #undef it at the bottom to prevent some random person from accidentally including it in faraway code and assuming it can be used as a general-purpose thing.

@@ +54,5 @@
> + * This class represents the chain of APZCs along which overscroll is handed off.
> + * It is created by APZCTreeManager by starting from an initial APZC which is
> + * the target for input events, and following the scroll parent ID links (often
> + * but not always corresponding to parent pointers in the APZC tree), then
> + * adjusting from scrollgrab.

s/from/for/

@@ +89,5 @@
> +  uint32_t Length() const {
> +    return mChain.size();
> +  }
> +  const nsRefPtr<AsyncPanZoomController>& GetApzcAtIndex(uint32_t aIndex) const {
> +    return mChain[aIndex];

assert aIndex is within bounds

Comment 8

[Botond Ballo [:botond]]

Attachment: bug1039992.patch

Updated patch to address Kats' review comments. Carrying r+ from Markus and Benoit.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> I think we can also simplify the
> code a bit more by moving more stuff into the OverscrollHandoffChain
> implementation. Things like CancelAnimationsForOverscrollHandoffChain,
> FlushRepaintsForOverscrollHandoffChain, etc (anything that walks the entire
> handoff chain) could be moved to the handoff chain class for better
> cohesiveness.

I like it! Done.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ -431,5 @@
> >                                                              &inOverscrolledApzc);
> >        if (apzc) {
> > -        if (panInput.mType == PanGestureInput::PANGESTURE_START ||
> > -            panInput.mType == PanGestureInput::PANGESTURE_MOMENTUMSTART) {
> > -          BuildOverscrollHandoffChain(apzc);
> 
> Yay code deletion \o/

To be fair, this is move of a code move, as we now have 'mPanGesture = MakeUnique<PanGestureState>(BuildOverscrollHandoffChain())' in OnPanBegin() and OnPanMomentumStart(), the APZC methods that are triggered by these events.

(The code deletion that I was happy about, was the removal of the [Build|Clear]OverscrollHandoffChain() calls from TestAsyncPanZoomController.cpp. It's nice that the chain Just Gets Built when it needs to.)

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +840,5 @@
> >  {
> > +  AssertOnControllerThread();
> > +  MOZ_ASSERT(aOverscrollHandoffChain.Length() > 0);
> > +  for (uint32_t i = 0; i < aOverscrollHandoffChain.Length(); i++) {
> > +    if (AsyncPanZoomController* item = aOverscrollHandoffChain.GetApzcAtIndex(i)) {
> 
> item should never be null here, since you never insert a null entry into the
> chain and they're stored as refptrs. So we shouldn't need this check, and
> I'd rather remove it to catch errors.
> 
> @@ +853,5 @@
> > +  AssertOnControllerThread();
> > +  const OverscrollHandoffChain& overscrollHandoffChain = *CurrentTouchBlock()->GetOverscrollHandoffChain();
> > +  MOZ_ASSERT(overscrollHandoffChain.Length() > 0);
> > +  for (uint32_t i = 0; i < overscrollHandoffChain.Length(); i++) {
> > +    if (AsyncPanZoomController* item = overscrollHandoffChain.GetApzcAtIndex(i)) {
> 
> Ditto on null check
> 
> @@ +874,5 @@
> > +  // See whether any APZC in the handoff chain starting from |aApzc|
> > +  // has room to be panned.
> > +  for (uint32_t j = i; j < aOverscrollHandoffChain.Length(); ++j) {
> > +    if (aOverscrollHandoffChain.GetApzcAtIndex(j)->IsPannable()) {
> > +      return true;
> 
> I note here you're not checking for null :)

Good point. Removed null checks.

> @@ +927,5 @@
> >    if (aEvent.AsMultiTouchInput().mType == MultiTouchInput::MULTITOUCH_START) {
> >      block = StartNewTouchBlock(false);
> > +    if (!block) {
> > +      // This APZC IsDestroyed(), don't do further processing.
> > +      return nsEventStatus_eIgnore;
> 
> Hm.. this bring up an interesting edge case. The touch event will be
> discarded in the APZ code but that touch event will still get dispatched to
> content. In general this can cause the touch-block-balance to get out of
> whack, because content will still send a ContentReceivedTouch back. Arguably
> this APZC is destroyed and so will never receive that ContentReceivedTouch
> but we might also have a new APZC with the same scrollablelayerguid by that
> point and that would receive it instead.
> 
> I think this is probably a pre-existing issue in the code. Still, it might
> be better to have APZC::BuildOverscrollHandoffChain just return new "empty"
> handoff chain instead of nullptr. (I don't recall if the handoff chain
> includes this APZC instance; if so "empty" would mean size 1). It would make
> the rest of the code to worry less about cases like this and generally make
> things more consistent. Thoughts?

Good point, and I like this approach. Done.

> @@ +1394,5 @@
> >    APZC_LOG("%p got a pan-maybegin in state %d\n", this, mState);
> >  
> >    mX.StartTouch(aEvent.mPanStartPoint.x, aEvent.mTime);
> >    mY.StartTouch(aEvent.mPanStartPoint.y, aEvent.mTime);
> > +  CancelAnimationsForOverscrollHandoffChain(*mPanGestureState->GetOverscrollHandoffChain());
> 
> I think this gets called before OnPanBegin, so it's possible
> mPanGestureState is nullptr here.

Hmm, good point again. I think the best we can do is call CancelAnimationsForOverscrollHandoffChain() if mPanGestureState is not nullptr, and CancelAnimation() if it is.

> @@ +1821,5 @@
> > +
> > +void AsyncPanZoomController::SnapBackOverscrolledApzc(const nsRefPtr<const OverscrollHandoffChain>& aOverscrollHandoffChain) {
> > +  for (uint32_t i = 0; i < aOverscrollHandoffChain->Length(); ++i) {
> > +    AsyncPanZoomController* apzc = aOverscrollHandoffChain->GetApzcAtIndex(i);
> > +    if (apzc && !apzc->IsDestroyed() && apzc->SnapBackIfOverscrolled()) {
> 
> apzc should never be null here, so no need to check for it (same as my above
> comment)

Removed null check.

> @@ +2848,5 @@
> > +  return (mState == PANNING || mState == PANNING_LOCKED_X || mState == PANNING_LOCKED_Y);
> > +}
> > +
> > +bool AsyncPanZoomController::IsInTouchingState() const {
> > +  // Any state where we expec there to be a touch point.
> 
> s/expec/expect/. But this function doesn't appear to be used anywhere, can
> we get rid of it?

Yeah, this was a remnant from an earlier design where I'd clear the handoff chain whenever we transitioned into a non-touching state. Removed.

> ::: gfx/layers/apz/src/AsyncPanZoomController.h
> @@ +789,5 @@
> >     * The functions and members in this section are used to manage
> > +   * pan gestures.
> > +   */
> > +
> > +  UniquePtr<PanGestureState> mPanGestureState;
> 
> Add a private: on this. I'd rather have each section not rely on visibility
> left over from the previous section.

Done.

> ::: gfx/layers/apz/src/GestureState.h
> @@ +17,5 @@
> > +class OverscrollHandoffChain;
> > +
> > +/**
> > + * A base class that stores state common to various gestures.
> > + * Currently, it just stores the overscroll handoff chain.
> 
> This feels a bit like overgeneralization. For one thing, there is no real
> state associated with the pan gesture. And for another, a block of touch
> events isn't really a "gesture". At least not the way we use the term in the
> rest of the code.
> 
> I think I would prefer calling this InputBlockState, having TouchBlockState
> extend from it, and get rid of PanGestureState entirely. Code that currently
> uses PanGestureState can use InputBlockState directly, and we can add other
> subclasses later if needed. Thoughts?

Sounds reasonable. I also renamed the file to InputBlockState.{h,cpp} correspondingly.

> ::: gfx/layers/apz/src/OverscrollHandoffChain.h
> @@ +28,5 @@
> > + * mutable versions.
> > + */
> > +#define NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING(_class)                 \
> > +public:                                                                       \
> > +  NS_METHOD_(MozExternalRefCountType) AddRef(void) const {                    \
> 
> I don't really like that this macros is defined here. They probably belong
> in nsISupportsImpl.h or some other reusable header file. I'm ok with it
> landing here for the moment but I would like a follow-up bug to move them
> out. 

> Also, as long as they're in this header file it would be good to #undef
> it at the bottom to prevent some random person from accidentally including
> it in faraway code and assuming it can be used as a general-purpose thing.

Good idea. Added #undef.

> @@ +54,5 @@
> > + * This class represents the chain of APZCs along which overscroll is handed off.
> > + * It is created by APZCTreeManager by starting from an initial APZC which is
> > + * the target for input events, and following the scroll parent ID links (often
> > + * but not always corresponding to parent pointers in the APZC tree), then
> > + * adjusting from scrollgrab.
> 
> s/from/for/

Fixed.

> @@ +89,5 @@
> > +  uint32_t Length() const {
> > +    return mChain.size();
> > +  }
> > +  const nsRefPtr<AsyncPanZoomController>& GetApzcAtIndex(uint32_t aIndex) const {
> > +    return mChain[aIndex];
> 
> assert aIndex is within bounds

Done.

Comment 9

[Botond Ballo [:botond]]

Try push that includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=0eed380c2ead

Comment 10

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> ::: gfx/layers/apz/src/OverscrollHandoffChain.h
> @@ +28,5 @@
> > + * mutable versions.
> > + */
> > +#define NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING(_class)                 \
> > +public:                                                                       \
> > +  NS_METHOD_(MozExternalRefCountType) AddRef(void) const {                    \
> 
> I don't really like that this macros is defined here. They probably belong
> in nsISupportsImpl.h or some other reusable header file. I'm ok with it
> landing here for the moment but I would like a follow-up bug to move them
> out.

Yep, that's what I was planning to do.

Comment 11

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

Comment on attachment 8473159 [details] [diff] [review]
bug1039992.patch

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

Much nicer, thanks! r+ with some style nits

::: gfx/layers/apz/src/OverscrollHandoffChain.cpp
@@ +12,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +void
> +OverscrollHandoffChain::Add(AsyncPanZoomController* aApzc) {

brace on newline per mozilla style for new files (here and throughout this file)

::: gfx/layers/apz/src/OverscrollHandoffChain.h
@@ +65,5 @@
> +  // nsRefPtr<const OverscrollHandoffChain> and thus enforce that, once built,
> +  // the chain is not modified.
> +  NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING(OverscrollHandoffChain)
> +
> +  /**

This should be a /* instead of /** - if we run doxygen on this code it will use this comment as documentation for the Add method and the SortByScrollPriority will not be documented. Same for the other ones below.

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 1 - Fix

Updated patch to address latest review comments and fix some build errors seen in the Try push.

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 2 - Minor refactoring to gtests

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 3 - Gtest

Gtests for scrollgrab will come in bug 1042974.

Comment 15

[Botond Ballo [:botond]]

https://tbpl.mozilla.org/?tree=Try&rev=6dd0bc196206

Comment 16

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

Comment on attachment 8474677 [details] [diff] [review]
Part 2 - Minor refactoring to gtests

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

This patch looks like a subset of my part 5 on bug 1052063.

Comment 17

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> This patch looks like a subset of my part 5 on bug 1052063.

Indeed. My suggestion is that we land this, and I'll rebase your part 5 patch to apply on top of this; I'm also open to other suggestions.

Comment 18

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

I'm a little biased but it seems like it would be simpler to rebase your part 3 on top of my part 5. All you'd have to do is rename a few variables. Rebasing my part 5 will be more work as the changes are more extensive. (Also I'd like to get that bug 1052063 landed sooner rather than later, since it's actually the thing we committed to for 2.1).

Comment 19

[Botond Ballo [:botond]]

Attachment: Part 1.5 - Fix a unified build issue

The Try results show a unified build issue on Windows; this should fix it.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8474677 [details] [diff] [review]
Part 2 - Minor refactoring to gtests

As discussed on IRC, I will land Part 1 (and Part 1.5 along with it) now, and wait for bug 1052063 to land before rebasing and landing the tests (Part 2 and Part 3).

Dropping review flag on Parts 2 and 3 for now; I will ask for re-review after rebasing.

Comment 21

[Botond Ballo [:botond]]

New Try push: https://tbpl.mozilla.org/?tree=Try&rev=857e8bae54cd

Comment 22

[Botond Ballo [:botond]]

Landed the Part 1 and Part 1.5 patches:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/e189b65bba07
  https://hg.mozilla.org/integration/mozilla-inbound/rev/dde94c2f9be6

Marking leave-open until Parts 2 and 3 land as well.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e189b65bba07
https://hg.mozilla.org/mozilla-central/rev/dde94c2f9be6

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8474677 [details] [diff] [review]
Part 2 - Minor refactoring to gtests

This patch is entirely obsoleted by the Part 5 patch in bug 1052063.

Comment 25

[Botond Ballo [:botond]]

Attachment: Part 2 - Gtests

This patch:

  - Is rebased on top of bug 1052063.

  - Introduces an APZOverscrollHandoffTester subclass of APZCTreeManagerTester.

  - Renames the test in the original patch to a more descriptive name.

  - Adds two additional gtests to test the scenarios described in the third and
    fourth paragraphs of comment 0.

Comment 26

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

Comment on attachment 8475569 [details] [diff] [review]
Part 2 - Gtests

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1332,5 @@
>    virtual void SetUp() {
>      gfxPrefs::GetSingleton();
>      AsyncPanZoomController::SetThreadAssertionsEnabled(false);
>  
> +    testStartTime = TimeStamp::Now();

Doh, nice catch!

@@ +1754,5 @@
> +  TestAsyncPanZoomController* childApzc = (TestAsyncPanZoomController*)layers[1]->GetAsyncPanZoomController();
> +
> +  // Enable touch-listeners so that we can separate the queueing of input
> +  // events from them being processed.
> +  childApzc->GetFrameMetrics().mMayHaveTouchListeners = true;

Doesn't seem like you need to set this for this test. Is it just for consistency with the next test?

Comment 27

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> @@ +1754,5 @@
> > +  TestAsyncPanZoomController* childApzc = (TestAsyncPanZoomController*)layers[1]->GetAsyncPanZoomController();
> > +
> > +  // Enable touch-listeners so that we can separate the queueing of input
> > +  // events from them being processed.
> > +  childApzc->GetFrameMetrics().mMayHaveTouchListeners = true;
> 
> Doesn't seem like you need to set this for this test. Is it just for
> consistency with the next test?

The test is designed to fail in a scenario where we store a per-APZC (or per-APZCTM) handoff chain and clear it in RIE(touch-end). We established that doing that would be wrong because if the touch block is not ready to be processed at the time the touch-end comes in, the handoff chain would be cleared by the time the block is processed.

Setting mMayHaveTouchListeners ensures that during the ApzcPanNoFling() call (which makes RIE calls), the touch block is not processed yet, only when ContentReceivedTouch() is called.

Comment 28

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

Ah true, that makes sense. Thanks for clarifying.

Comment 29

[Botond Ballo [:botond]]

Landed test patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/064d7f0b464d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/064d7f0b464d

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> > ::: gfx/layers/apz/src/OverscrollHandoffChain.h
> > @@ +28,5 @@
> > > + * mutable versions.
> > > + */
> > > +#define NS_INLINE_DECL_THREADSAFE_MUTABLE_REFCOUNTING(_class)                 \
> > > +public:                                                                       \
> > > +  NS_METHOD_(MozExternalRefCountType) AddRef(void) const {                    \
> > 
> > I don't really like that this macros is defined here. They probably belong
> > in nsISupportsImpl.h or some other reusable header file. I'm ok with it
> > landing here for the moment but I would like a follow-up bug to move them
> > out.
> 
> Yep, that's what I was planning to do.

I filed bug 1056356 to track supporting 'nsRefPtr<const T>'. It may end up being done slightly differently than how we did it here (and once that's done, we can get rid of this local macro and its usage).