APZCTreeManager should support having multiple layers with the same scrollid

RESOLVED FIXED in mozilla34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
mozilla34
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 17 obsolete attachments)

18.88 KB, patch
botond
: review+
Details | Diff | Splinter Review
7.01 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.85 KB, patch
botond
: review+
Details | Diff | Splinter Review
18.71 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.87 KB, patch
botond
: review+
Details | Diff | Splinter Review
22.10 KB, patch
kats
: review+
Details | Diff | Splinter Review
10.10 KB, patch
kats
: review+
Details | Diff | Splinter Review
31.04 KB, patch
botond
: review+
Details | Diff | Splinter Review
12.78 KB, patch
botond
: review+
Details | Diff | Splinter Review
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.
Created attachment 8471556 [details] [diff] [review]
Part 1 - Re-use the same APZC for layers with the same scrollid
Created attachment 8471557 [details] [diff] [review]
Part 1 (diff ignoring whitespace)
Created attachment 8471558 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC
Created attachment 8471559 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC
Attachment #8471558 - Attachment is obsolete: true
Created attachment 8471561 [details] [diff] [review]
Part 3 - Don't run multiple animation steps in a single composite
Depends on: 1052474
Created attachment 8471801 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC
Attachment #8471559 - Attachment is obsolete: true
Created attachment 8471802 [details] [diff] [review]
Part 4 - Flip order of transforms

Not sure if this is complete or even a good idea...
.. and it doesn't fix bug 1052474 unfortunately.
No longer depends on: 1052474
Created attachment 8472456 [details] [diff] [review]
Part 1 - Re-use the same APZC for layers with the same scrollid
Attachment #8471556 - Attachment is obsolete: true
Created attachment 8472458 [details] [diff] [review]
Part 1 (diff ignore whitespace changes)
Attachment #8471557 - Attachment is obsolete: true
Created attachment 8472459 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC
Attachment #8471801 - Attachment is obsolete: true
Created attachment 8472460 [details] [diff] [review]
Part 3 - Flip order of transforms
Attachment #8471802 - Attachment is obsolete: true
Created attachment 8472461 [details] [diff] [review]
Part 4 - Don't run multiple animation steps in a single composite
Attachment #8471561 - Attachment is obsolete: true
Created attachment 8472462 [details] [diff] [review]
Part 5 - Expand APZCTreeManagerTester to do more boilerplate
Created attachment 8472464 [details] [diff] [review]
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.
Attachment #8472456 - Flags: review?(botond)
Attachment #8472459 - Flags: review?(botond)
Attachment #8472461 - Flags: review?(botond)
Attachment #8472462 - Flags: review?(botond)
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.
Attachment #8472461 - Flags: review?(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;
Attachment #8472458 - Flags: review+
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.
Attachment #8472456 - Flags: review?(botond)
Created attachment 8472603 [details] [diff] [review]
Part 3 - Flip order of transforms

Now with 200% more flipping
Attachment #8472460 - Attachment is obsolete: true
Created attachment 8473161 [details] [diff] [review]
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+
Attachment #8472456 - Attachment is obsolete: true
Attachment #8472458 - Attachment is obsolete: true
Attachment #8473161 - Flags: review+
Created attachment 8473162 [details] [diff] [review]
Part 3 - Flip order of transforms

Updated to fix a couple of Fennec things. Seems to work well now.
Attachment #8472603 - Attachment is obsolete: true
Attachment #8473162 - Flags: review?(botond)
Created attachment 8473163 [details] [diff] [review]
Part 3b - Clarify stuff about the ancestor transform
Attachment #8473163 - Flags: review?(botond)
Created attachment 8473164 [details] [diff] [review]
Part 3c - Combine the CSS transform into the ancestor transform

Needs some more testing first.
An older try push without 3b and 3c: https://tbpl.mozilla.org/?tree=Try&rev=07dcee9e8b76
Attachment #8473164 - Flags: review?(botond)
New try push with everything except part 4, which I haven't had a chance to fix up yet. Will do that tomorrow hopefully.
Created attachment 8473347 [details] [diff] [review]
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.
Attachment #8473347 - Flags: review?(botond)
Created attachment 8473348 [details] [diff] [review]
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.
Attachment #8472461 - Attachment is obsolete: true
Attachment #8473348 - Flags: review?(botond)
Created attachment 8473349 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC

Slight documentation update as well.
Attachment #8472459 - Attachment is obsolete: true
Attachment #8472459 - Flags: review?(botond)
Attachment #8473349 - Flags: review?(botond)
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 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.
Attachment #8472464 - Flags: review?(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?
Attachment #8473349 - Flags: review?(botond) → review+
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)

?
Attachment #8473162 - Flags: review?(botond) → review-
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.
(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.
Created attachment 8475134 [details] [diff] [review]
Part 1 - Re-use the same APZC for layers with the same scrollid

Rebased, carrying r+
Attachment #8473161 - Attachment is obsolete: true
Attachment #8475134 - Flags: review+
Created attachment 8475136 [details] [diff] [review]
Part 2 - Accumulate hit regions for all matching layers into the APZC
Attachment #8473349 - Attachment is obsolete: true
Attachment #8475136 - Flags: review+
Created attachment 8475140 [details] [diff] [review]
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.
Attachment #8473162 - Attachment is obsolete: true
Attachment #8475140 - Flags: review?(botond)
Created attachment 8475141 [details] [diff] [review]
Part 3c - Combine the CSS transform into the ancestor transform

Updated because we don't need the CSS transform at all anymore.
Attachment #8473164 - Attachment is obsolete: true
Attachment #8473164 - Flags: review?(botond)
Attachment #8475141 - Flags: review?(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).
Attachment #8475140 - Flags: review?(botond) → review+
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.
Attachment #8473163 - Flags: review?(botond) → review+
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.
Attachment #8475141 - Flags: review?(botond) → review+

Updated

4 years ago
Attachment #8473347 - Flags: review?(botond) → review+
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.)
Attachment #8473348 - Flags: review?(botond) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Depends on: 1055741
Filed bug 1055741 for dealing with the ScreenPixel and mTransformScale coordinate space cleanup.
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.
Attachment #8472462 - Flags: review?(botond) → review+
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.
Attachment #8472464 - Flags: review?(botond) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Comment 51

4 years ago
greentry
New try push with all the patches updated: https://tbpl.mozilla.org/?tree=Try&rev=f36583c31913
Blocks: 1055760
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
Depends on: 1058664
You need to log in before you can comment on or make changes to this bug.