1052063 - APZCTreeManager should support having multiple layers with the same scrollid

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

This is a prerequisite for bug 967844. Comes after the patches in bug 1051985. The main things I can think of here that need fixing are the hit-testing code and event untransforming code in APZCTreeManager.

Comment 1

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

Attachment: Part 1 - Re-use the same APZC for layers with the same scrollid

Comment 2

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

Attachment: Part 1 (diff ignoring whitespace)

Comment 3

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

Attachment: Part 2 - Accumulate hit regions for all matching layers into the APZC

Comment 4

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

Attachment: Part 2 - Accumulate hit regions for all matching layers into the APZC

Comment 5

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

Attachment: Part 3 - Don't run multiple animation steps in a single composite

Comment 6

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

Attachment: Part 2 - Accumulate hit regions for all matching layers into the APZC

Comment 7

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

Attachment: Part 4 - Flip order of transforms

Not sure if this is complete or even a good idea...

Comment 8

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

.. and it doesn't fix bug 1052474 unfortunately.

Comment 9

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

Attachment: Part 1 - Re-use the same APZC for layers with the same scrollid

Comment 10

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

Attachment: Part 1 (diff ignore whitespace changes)

Comment 11

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

Attachment: Part 2 - Accumulate hit regions for all matching layers into the APZC

Comment 12

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

Attachment: Part 3 - Flip order of transforms

Comment 13

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

Attachment: Part 4 - Don't run multiple animation steps in a single composite

Comment 14

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

Attachment: Part 5 - Expand APZCTreeManagerTester to do more boilerplate

Comment 15

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

Attachment: Part 6 - Add some basic tests for the multi-layer scenarios

I expect to flesh out these tests in future patches or future bugs once I have a better idea of the scenarios we need to cover.

Comment 16

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

Comment on attachment 8472461 [details] [diff] [review]
Part 4 - Don't run multiple animation steps in a single composite

Actually this one seems to introduce some buggy behavior. Unflagging for now.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8472458 [details] [diff] [review]
Part 1 (diff ignore whitespace changes)

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

::: gfx/layers/FrameMetrics.h
@@ +563,5 @@
>    {
>      return !(*this == other);
>    }
> +
> +  bool operator<(const ScrollableLayerGuid& other) const

A neat way to implement a function like this is to leverage the lexicographical comparison implemented for tuples:

  return MakeRefTuple(mLayersId, mPresShellId, mScrollId) 
       < MakeRefTuple(other.mLayersId, other.mPresShellId, other.mScrollId);

The 'Ref' part is so that we don't copy the elements, just create a tuple of references to them.

(You could even go farther and add a private method called RefTuple() and then have

  return RefTuple() < other.RefTuple();

And even

  return RefTuple() == other.RefTuple();

for operator==).

Up to you if you like this or not.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +143,5 @@
>                                  // aCompositor is null in gtest scenarios
>                                  aCompositor ? aCompositor->RootLayerTreeId() : 0,
>                                  Matrix4x4(), nullptr, nullptr,
>                                  aIsFirstPaint, aOriginatingLayersId,
> +                                paintLogger, &apzcsToDestroy, &apzcMap,

Is there a reason to pass the map by pointer? Passing by reference would work just as well, and I think it would be cleaner.

@@ +188,5 @@
> +      // that is supposed to scroll together is split into multiple layers because of
> +      // e.g. non-scrolling content interleaved in z-index order.
> +      ScrollableLayerGuid guid(aLayersId, metrics);
> +      if (aApzcMap->find(guid) != aApzcMap->end()) {
> +        apzc = (*aApzcMap)[guid];

operator[] will do another lookup in the map. Better to do this:

auto iter = aApzcMap->find(guid);
if (iter != aApzcMap->end()) {
  apzc = iter->second;
}

@@ +327,5 @@
>              apzc->UpdateZoomConstraints(apzc->GetParent()->GetZoomConstraints());
>            }
>          }
> +
> +        (*aApzcMap)[guid] = apzc;

You can avoid this lookup as well, as follows:

// Earlier:
auto insertResult = aApzcMap->insert(make_pair(guid, nullptr));  
  // only actually inserts if element wasn't there before
  // insertResult is a pair of (iterator, did I insert?)
auto iter = insertResult.first;
if (!insertResult.second) {
  apzc = iter->second;
}

// Now:
iter->second = apzc;

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8472456 [details] [diff] [review]
Part 1 - Re-use the same APZC for layers with the same scrollid

I put the r+ in the ignoring-whitespace patch, consider it to apply here too.

Comment 19

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

Attachment: Part 3 - Flip order of transforms

Now with 200% more flipping

Comment 20

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

Attachment: Part 1 - Re-use the same APZC for layers with the same scrollid

Updated part 1. Our MakeRefTuple doesn't have operator< defined so I couldn't do that. I did the insertResult stuff though. Carrying r+

Comment 21

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

Attachment: Part 3 - Flip order of transforms

Updated to fix a couple of Fennec things. Seems to work well now.

Comment 22

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

Attachment: Part 3b - Clarify stuff about the ancestor transform

Comment 23

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

Attachment: Part 3c - Combine the CSS transform into the ancestor transform

Needs some more testing first.

Comment 24

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

An older try push without 3b and 3c: https://tbpl.mozilla.org/?tree=Try&rev=07dcee9e8b76

Comment 25

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

New try push with everything except part 4, which I haven't had a chance to fix up yet. Will do that tomorrow hopefully.

Comment 26

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

https://tbpl.mozilla.org/?tree=Try&rev=8dde54f8dd12

Comment 27

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

Attachment: Part 4a - Do a single pass to advance animations before sampling

There turned out to be two problems with my old part 4. One of them was the we were calling SampleContentTransformForFrame in some cases from the scrollbar transform code, and ignoring the return value (i.e. whether or not to schedule another composite). There was a TODO there to fix it, so I fixed it.

Comment 28

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

Attachment: Part 4b - Deduplicate calls to UpdateAnimation

And the other problem was that we weren't updating mLastSampleTime if there was no animation, so the equality check was hitting too often. This fixes that.

Comment 29

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

Attachment: Part 2 - Accumulate hit regions for all matching layers into the APZC

Slight documentation update as well.

Comment 30

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

About part 3 (flipping order of transforms), i attempted something similar in bug 903460 (duped to 732971) but that attempt was flawed i now realize because i left the translation happening first as part of the async transform. Still there are a few cases in that discussion that i should verify are not broken, like fixed position content in fennec and metro. It looks like the code has always applied the async transform first and I've thought it was wrong on multiple occasions and never managed to fix it properly until now (fingers crossed).

Comment 31

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

Comment on attachment 8472464 [details] [diff] [review]
Part 6 - Add some basic tests for the multi-layer scenarios

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

I can't actually think of anything else that is testable in gtest and that isn't covered by the hit-testing tests already. The stuff I would like to test is the composition code in AsyncCompositionManager but that's not available to gtests.

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8473349 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +349,5 @@
> +        // Consider the case where we have three layers: A, B, and C. A is at the top in
> +        // z-order and C is at the bottom. A and C share a scrollid and scroll together; but
> +        // B has a different scrollid and scrolls independently. Depending on how B moves
> +        // a larger/smaller area of C may be unobscured. However we are combining the
> +        // non-transient hit region of A and C so we basically assume the same amount of C

Could you clarify what "non-transient hit region" means here?

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8473162 [details] [diff] [review]
Part 3 - Flip order of transforms

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

Most of the changes in this patch look good. However, r-'ing until the comments about ToParentLayerCoords and mTransformScale are addressed.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1391,5 @@
>    // leftmost matrix in a multiplication is applied first.
>  
>    // ancestorUntransform is OC.Inverse() * NC.Inverse() * MC.Inverse()
>    Matrix4x4 ancestorUntransform = aApzc->GetAncestorTransform();
>    ancestorUntransform.Invert();

(When I last reviewed this code, it was 'gfx3DMatrix ancestorUntransform = aApzc->GetAncestorTransform().Inverse()'. The switch to Matrix4x4 didn't exactly make things nicer...)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -2218,5 @@
> -  // In a ViewTransform, the translation is applied before the scale. We want
> -  // to apply our translation after our scale, so we compensate for that here.
> -  translation.x /= scale;
> -  translation.y /= scale;
> -

I like that we can get rid of this!

@@ -2257,5 @@
> -      // part of aNewTransform->mScale, this results in an overscroll
> -      // translation whose magnitude varies with the zoom. To avoid this,
> -      // we adjust for that here.
> -      aOutOverscrollTransform->mTranslation.x *= aOutTransform->mScale.scale;
> -      aOutOverscrollTransform->mTranslation.y *= aOutTransform->mScale.scale;

Ditto.

@@ +2890,5 @@
>  }
>  
>  ParentLayerPoint AsyncPanZoomController::ToParentLayerCoords(const ScreenPoint& aPoint)
>  {
> +  return TransformTo<ParentLayerPixel>(To3DMatrix(GetCSSTransform() * GetNontransientAsyncTransform()), aPoint);

I think this part is wrong. Since we are no longer un-applying the LN and LC transforms when deliver input events to L's APZC, I think the input points are now in ParentLayer space. Or, to put it another way, "local" Screen space (which we have been defining as "the space APZC gets its inputs in") has become identical to ParentLayer space.

Therefore, I think the conversion from ParentLayer coords to Screen coords should now be 1, and ToParentLayerCoords() and mFrameMetrics.mTransformScale should be updated to reflect this. (In the future, I think we can get rid of one of the two coordinate systems, and remove mTransformScale and ToParentLayerCoords(), but obviously we don't need to do that in this bug.)

Does that make sense?

@@ +2895,5 @@
>  }
>  
>  void AsyncPanZoomController::UpdateTransformScale()
>  {
> +  Matrix4x4 nontransientTransforms = GetCSSTransform() * GetNontransientAsyncTransform();

See comment about ToParentLayerCoords().

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +37,5 @@
>    operator gfx::Matrix4x4() const
>    {
>      return
> +      gfx::Matrix4x4().Scale(mScale.scale, mScale.scale, 1) *
> +      gfx::Matrix4x4().Translate(mTranslation.x, mTranslation.y, 0);

Can this be expressed more efficiently as

 gfx::Matrix4x4().Scale(mScale.scale, mScale.scale, 1)
                 .PostTranslate(mTranslation.x, mTranslation.y, 0)

?

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8473162 [details] [diff] [review]
Part 3 - Flip order of transforms

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

Also, while I haven't really thought this through, I think it's possible that this patch will exacerbate the effects of our improper handling of CSS transforms (bug 993525). I don't think that's a reason to hold up this patch though - the solution is to fix bug 993525, which I hope to get to doing in the very near future.

Comment 35

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

(In reply to Botond Ballo [:botond] from comment #32)
> @@ +349,5 @@
> > +        // B has a different scrollid and scrolls independently. Depending on how B moves
> > +        // a larger/smaller area of C may be unobscured. However we are combining the
> > +        // non-transient hit region of A and C so we basically assume the same amount of C
> 
> Could you clarify what "non-transient hit region" means here?

Will do. "Depending on how B moves and the async transform on it, a larger/smaller ... However, when we combine the hit regions of A and C here we are ignoring that async transform and so we basically assume ..."

(In reply to Botond Ballo [:botond] from comment #33)
> (When I last reviewed this code, it was 'gfx3DMatrix ancestorUntransform =
> aApzc->GetAncestorTransform().Inverse()'. The switch to Matrix4x4 didn't
> exactly make things nicer...)

Yeah I made a comment to the same effect; I think dzbarsky will add an Inverse() function to Matrix4x4 at some point which should restore the old niceness.

> >  ParentLayerPoint AsyncPanZoomController::ToParentLayerCoords(const ScreenPoint& aPoint)
> >  {
> > +  return TransformTo<ParentLayerPixel>(To3DMatrix(GetCSSTransform() * GetNontransientAsyncTransform()), aPoint);
> 
> I think this part is wrong. Since we are no longer un-applying the LN and LC
> transforms when deliver input events to L's APZC, I think the input points
> are now in ParentLayer space. Or, to put it another way, "local" Screen
> space (which we have been defining as "the space APZC gets its inputs in")
> has become identical to ParentLayer space.
> 
> Therefore, I think the conversion from ParentLayer coords to Screen coords
> should now be 1, and ToParentLayerCoords() and mFrameMetrics.mTransformScale
> should be updated to reflect this. (In the future, I think we can get rid of
> one of the two coordinate systems, and remove mTransformScale and
> ToParentLayerCoords(), but obviously we don't need to do that in this bug.)
> 
> Does that make sense?

Yup, that makes sense. I was actually also wondering about this, because this is the only place left we still use AsyncPanZoomController::GetCSSTransform() and it seemed wrong to me, because APZC::GetCSSTransform itself is not well defined any more. If we take this out we can get rid of that too which will make me happy :)

> ::: gfx/layers/composite/AsyncCompositionManager.h
> > +      gfx::Matrix4x4().Scale(mScale.scale, mScale.scale, 1) *
> > +      gfx::Matrix4x4().Translate(mTranslation.x, mTranslation.y, 0);
> 
> Can this be expressed more efficiently as
> 
>  gfx::Matrix4x4().Scale(mScale.scale, mScale.scale, 1)
>                  .PostTranslate(mTranslation.x, mTranslation.y, 0)
> 
> ?

Yup, will do that.

Comment 36

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

Attachment: Part 1 - Re-use the same APZC for layers with the same scrollid

Rebased, carrying r+

Comment 37

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

Attachment: Part 2 - Accumulate hit regions for all matching layers into the APZC

Comment 38

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

Attachment: Part 3 - Flip order of transforms (v2)

Updated as per review comments. The only changes from the previous version were in ToParentLayerCoords, UpdateTransformScale, and the ViewTransform::operator Matrix4x4() implementation.

Comment 39

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

Attachment: Part 3c - Combine the CSS transform into the ancestor transform

Updated because we don't need the CSS transform at all anymore.

Comment 40

[Botond Ballo [:botond]]

Comment on attachment 8475140 [details] [diff] [review]
Part 3 - Flip order of transforms (v2)

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2899,5 @@
>  void AsyncPanZoomController::UpdateTransformScale()
>  {
> +  // The parent layer pixel space and the screen space for a given layer are the
> +  // same as of bug 1052063. FIXME: Unify these two coordinate systems.
> +  mFrameMetrics.mTransformScale.scale = 1;

Since mTransformScale is now always 1, there is no point in taking up space in every FrameMetrics instance to store it; we can just have a static method FrameMetrics::GetTransformScale() or something like that.

Feel free to do that in a follow-up (though not necessarily in the same one that unifies ParentLayerPixel and ScreenPixel, as that's a considerably larger change).

Comment 41

[Botond Ballo [:botond]]

Comment on attachment 8473163 [details] [diff] [review]
Part 3b - Clarify stuff about the ancestor transform

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +189,5 @@
>  
>  AsyncPanZoomController*
>  APZCTreeManager::UpdatePanZoomControllerTree(CompositorParent* aCompositor,
>                                               Layer* aLayer, uint64_t aLayersId,
> +                                             Matrix4x4 aAncestorTransform,

Now that this function does not modify this parameter, it can be taken by const reference.

Comment 42

[Botond Ballo [:botond]]

Comment on attachment 8475141 [details] [diff] [review]
Part 3c - Combine the CSS transform into the ancestor transform

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1227,5 @@
>    // explained in the comment on GetInputTransforms. This function will recurse with aApzc at L and P, and the
>    // comments explain what values are stored in the variables at these two levels. All the comments
>    // use standard matrix notation where the leftmost matrix in a multiplication is applied first.
>  
>    // ancestorUntransform takes points from aApzc's parent APZC's layer coordinates

As dicussed on IRC, this comment needs updating.

@@ +1242,5 @@
>    APZCTM_LOG("Untransformed %f %f to transient coordinates %f %f for hit-testing APZC %p\n",
>             aHitTestPoint.x, aHitTestPoint.y,
>             hitTestPointForThisLayer.x, hitTestPointForThisLayer.y, aApzc);
>  
>    // childUntransform takes points from aApzc's parent APZC's layer coordinates

And so does this one.

Comment 43

[Botond Ballo [:botond]]

Comment on attachment 8473348 [details] [diff] [review]
Part 4b - Deduplicate calls to UpdateAnimation

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

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2112,5 @@
>  
>  bool AsyncPanZoomController::UpdateAnimation(const TimeStamp& aSampleTime,
>                                               Vector<Task*>* aOutDeferredTasks)
>  {
> +  // This function may get called multiple with the same sample time, so

Maybe say why? (Because we call this function while iterating over a layer tree, and multiple layers may have the same APZC.)

Comment 44

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

(In reply to Botond Ballo [:botond] from comment #40)
> Since mTransformScale is now always 1, there is no point in taking up space
> in every FrameMetrics instance to store it; we can just have a static method
> FrameMetrics::GetTransformScale() or something like that.
> 
> Feel free to do that in a follow-up (though not necessarily in the same one
> that unifies ParentLayerPixel and ScreenPixel, as that's a considerably
> larger change).

I was wondering if we'll need something similar to mTransformScale to fix bug 993525. Now instead of having LayerPixel -> ScreenPixel -> ParentLayerPixel, we're going to have something like LayerPixel -> TransformedLayerPixel -> ParentLayerPixel, where TransformedLayerPixel is LayerPixel with the CSS transform applied. I'm fine with getting rid of it now though and then later adding something else if we need it.

For all your other comments (on parts 3b, 3c, 4b), I'll update my local patches to address those comments.

Comment 45

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #44)
> I was wondering if we'll need something similar to mTransformScale to fix
> bug 993525. Now instead of having LayerPixel -> ScreenPixel ->
> ParentLayerPixel, we're going to have something like LayerPixel ->
> TransformedLayerPixel -> ParentLayerPixel, where TransformedLayerPixel is
> LayerPixel with the CSS transform applied.

Interesting idea. Perhaps we can leave mTransformScale around until we know how we're going to fix bug 993525, then.

Comment 46

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

Filed bug 1055741 for dealing with the ScreenPixel and mTransformScale coordinate space cleanup.

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8472462 [details] [diff] [review]
Part 5 - Expand APZCTreeManagerTester to do more boilerplate

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1434,2 @@
>    Matrix4x4 transformToApzc;
>    Matrix4x4 transformToGecko;

I'm not sure it makes sense to have these in the base APZCTreeManagerTester class. It's used by the existing two APZCTreeManager tests, but it won't necessarily be by others (for example, it's not used by the one I wrote for bug 1039992, nor for the ones I will write for bug 1042974). 

I'd prefer moving them into the individual tests, or into an HitTestingTester subclass.

@@ +1434,5 @@
>    Matrix4x4 transformToApzc;
>    Matrix4x4 transformToGecko;
>  
> +protected:
> +  void SetScrollableFrameMetrics(Layer* aLayer, FrameMetrics::ViewID aScrollId,

This method can be static.

@@ +1448,5 @@
> +    metrics.SetScrollOffset(CSSPoint(0, 0));
> +    aLayer->SetFrameMetrics(metrics);
> +  }
> +
> +  already_AddRefed<AsyncPanZoomController> GetTargetAPZC(const ScreenPoint& aPoint) {

I guess this will need to go wherever transformToApzc and transformToGecko go, unless we make them parameters.

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8472464 [details] [diff] [review]
Part 6 - Add some basic tests for the multi-layer scenarios

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +1723,5 @@
> +  manager->UpdatePanZoomControllerTree(nullptr, root, false, 0, 0);
> +
> +  // so they should have the same APZC
> +  EXPECT_EQ(nullptr, layers[0]->GetAsyncPanZoomController());
> +  EXPECT_NE(nullptr, layers[1]->GetAsyncPanZoomController());

Might make sense to have locals apzc1, apzc2.

Comment 49

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

(In reply to Botond Ballo [:botond] from comment #47)
> I'm not sure it makes sense to have these in the base APZCTreeManagerTester
> class. It's used by the existing two APZCTreeManager tests, but it won't
> necessarily be by others (for example, it's not used by the one I wrote for
> bug 1039992, nor for the ones I will write for bug 1042974). 
> 
> I'd prefer moving them into the individual tests, or into an
> HitTestingTester subclass.

Fair enough, I'll extract an APZHitTestingTester subclass. (I like all the testers starting with A, so I can run |mach gtest "A*"| and run all the APZ tests.)

> > +protected:
> > +  void SetScrollableFrameMetrics(Layer* aLayer, FrameMetrics::ViewID aScrollId,
> 
> This method can be static.

Changed.

> I guess this will need to go wherever transformToApzc and transformToGecko
> go, unless we make them parameters.

Yup, will move it into the subclass as well.

(In reply to Botond Ballo [:botond] from comment #48)
> > +  EXPECT_NE(nullptr, layers[1]->GetAsyncPanZoomController());
> 
> Might make sense to have locals apzc1, apzc2.

I started making this change but then I realized I would have to update the locals at a couple of points in the test because the test changes the APZCs bound to the layers. Didn't really seem worth it with that in mind.

Comment 50

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> > I'd prefer moving them into the individual tests, or into an
> > HitTestingTester subclass.
> 
> Fair enough, I'll extract an APZHitTestingTester subclass. (I like all the
> testers starting with A, so I can run |mach gtest "A*"| and run all the APZ
> tests.)

Sounds good.

> (In reply to Botond Ballo [:botond] from comment #48)
> > > +  EXPECT_NE(nullptr, layers[1]->GetAsyncPanZoomController());
> > 
> > Might make sense to have locals apzc1, apzc2.
> 
> I started making this change but then I realized I would have to update the
> locals at a couple of points in the test because the test changes the APZCs
> bound to the layers. Didn't really seem worth it with that in mind.

Good point.

Comment 51

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

New try push with all the patches updated: https://tbpl.mozilla.org/?tree=Try&rev=f36583c31913

Comment 52

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

That try push is looking green but I had to rebase through a bunch of botond's changes on inbound so I did a build-only push to sanity check:

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

Comment 53

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4402de56732
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/073b4e162b5d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc893ff9125
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/239a9450b9ab
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/413e0473a1d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e433c535bd7c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac2cb10d6a4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/315997f45415
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/108038542674

Comment 54

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c4402de56732
https://hg.mozilla.org/mozilla-central/rev/073b4e162b5d
https://hg.mozilla.org/mozilla-central/rev/7dc893ff9125
https://hg.mozilla.org/mozilla-central/rev/239a9450b9ab
https://hg.mozilla.org/mozilla-central/rev/413e0473a1d8
https://hg.mozilla.org/mozilla-central/rev/e433c535bd7c
https://hg.mozilla.org/mozilla-central/rev/3ac2cb10d6a4
https://hg.mozilla.org/mozilla-central/rev/315997f45415
https://hg.mozilla.org/mozilla-central/rev/108038542674