Closed Bug 775459 Opened 7 years ago Closed 6 years ago

Implement (more) hit-testing logic for async/synchronously scrolled frames on UI thread

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
blocking-basecamp -

People

(Reporter: cjones, Assigned: BenWa)

References

Details

Attachments

(4 files, 15 obsolete files)

1.75 KB, patch
joe
: review+
Details | Diff | Splinter Review
6.30 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
12.28 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
32.94 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
In general, and arbitrary top-level window can have a mix of
 - asynchronously-scrolled viewport
 - asynchronously-scrolled subframes
 - synchronously-scrolled subframes

We need to do hit testing for the first two cases, always, and indeed already have most of the code we need to do this from xul-fennec.

We can greatly improve the UX of sync-scrolled subframes by including them in the UI-thread hit-test.  If an event targets a sync-scrolled frame, then the pan/zoom controller(s) can immediately know to ignore it and pass through to the content thread, and the content thread doesn't need to send a message back to the controller asking it to ignore the event / cancel in-progress animations.  This is both a UX and performance improvement.
Similarly, we could also improve the UX and performance of content that uses touchstart listeners by including the frames that have those listeners attached in the UI-thread hit-test.  If a touchstart targets one of those frames, we pay the round-trip cost of allowing content to preventDefault().  Otherwise, we immediately start async pan/zoom animation.

This work could be done in this bug or a follow that uses the machinery built here.
This doesn't sound like it necessarily blocks, but please nom if I'm wrong.
blocking-basecamp: --- → -
Blocks: 864438
Kats and I discussed a design that should properly handle the cases in comment 0 + fix position layers:

A layer will own a list of APZC with regions they will handle. When we receive input events we can perform hit testing on the layer tree where priority is given to the inner most APZC matches. The APZC can either handle the event or do a preventDefault check round trip. Note that the APZC may not have built a layer for the subframe and thus would need a round trip to create a layer+displayport for the subframe to allow async scrolling. The APZC and contentcontroller will be responsible for managing the displayport.
Assignee: nobody → bgirard
Attachment #761563 - Flags: review?(nical.bugzilla) → review?(joe)
Comment on attachment 761563 [details] [diff] [review]
Part 0: (Debugging) Add border for scrollable container layers

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

Presumably DrawDiagnostics does nothing if it's not turned on?

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +157,5 @@
> +    gfx::Rect rect(visibleRect.x, visibleRect.y, visibleRect.width, visibleRect.height);
> +    gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> +    aManager->GetCompositor()->DrawDiagnostics(gfx::Color(1.0, 0.0, 0.0, 1.0),
> +                                    rect, clipRect,
> +                                    transform, gfx::Point(aOffset.x, aOffset.y));

vertically align these arguments
Attachment #761563 - Flags: review?(joe) → review+
Attachment #761561 - Attachment is obsolete: true
Attachment #762255 - Flags: review?(matt.woodrow)
Attachment #762255 - Flags: review?(bugmail.mozilla)
Attachment #762255 - Attachment description: Create ScrollInfoLayers for frames we want APZC support (WIP) → Part 1: Create ScrollInfoLayers for frames we want APZC support (WIP)
This is already the case, let's enforce it. This will simplify the code and let us assume that scrollable layers are container layers with a scrollable frame metrics and that a scrollable container layer always has an APZC.
Attachment #762390 - Flags: review?(bugmail.mozilla)
Comment on attachment 762255 [details] [diff] [review]
Part 1: Create ScrollInfoLayers for frames we want APZC support (WIP)

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +544,5 @@
>      for (uint32_t i = 0; i < scrollableLayers.Length(); i++) {
> +      if (scrollableLayers[i] &&
> +          scrollableLayers[i]->AsContainerLayer()->GetFrameMetrics().IsRootScrollable()) {
> +        // FIXME Currently this method only applies to the root scrollable
> +        // layer.

I don't understand this change, I assume kats' review will cover this.

::: layout/base/nsDisplayList.cpp
@@ +3313,5 @@
> +nsRect
> +nsDisplayScrollInfoLayer::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap)
> +{
> +  nsRect borderBounds(ToReferenceFrame(), mFrame->GetSize());
> +  return borderBounds;

Just call nsDisplayItem::GetBounds instead. That will set aSnap too.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2228,5 @@
> +      wantSubAPZC &&
> +      ((styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN && mHScrollbarBox) ||
> +       (styles.mVertical   != NS_STYLE_OVERFLOW_HIDDEN && mVScrollbarBox)) &&
> +      (scrollRange.width > 0 || scrollRange.height > 0) &&
> +      (!mIsRoot || !mOuter->PresContext()->IsRootContentDocument())

This is a pretty confusing condition.

It would be nice to put the 'has scrollable overflow' checks into a helper.
Attachment #762255 - Flags: review?(matt.woodrow) → review+
I removed the userdata and now grab a proper refcount since I never bought the 'threadsafe refcounts are expensive' argument.

I moved the constructor/destructor to prevent including APZC in Layers.h which is already massive.
Attachment #762390 - Attachment is obsolete: true
Attachment #762390 - Flags: review?(bugmail.mozilla)
Attachment #762498 - Flags: review?(bugmail.mozilla)
Attachment #762255 - Attachment is obsolete: true
Attachment #762255 - Flags: review?(bugmail.mozilla)
Attachment #762503 - Flags: review?(bugmail.mozilla)
Comment on attachment 762503 [details] [diff] [review]
Part 1: Create ScrollInfoLayers for frames we want APZC support

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2226,5 @@
> +      (scrollRange.width > 0 || scrollRange.height > 0) &&
> +      ((styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN && mHScrollbarBox) ||
> +       (styles.mVertical   != NS_STYLE_OVERFLOW_HIDDEN && mVScrollbarBox));
> +    // TODO Turn this on for inprocess OMTC
> +    bool wantSubAPZC = (XRE_GetProcessType() == GeckoProcessType_Content);

I assume this wantSubAPZC condition will eventually change to be more like "was a sub apzc requested on this scroll frame by the parent apzc", is that right? i.e. this condition is more or less a placeholder for something that will come later.
Attachment #762503 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 762498 [details] [diff] [review]
Part 2: Make APZC attachable only to container layers

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

r=me with the comments below addressed. Please re-request review if the change to nsWindow.cpp turns out to be nontrivial.

::: gfx/layers/Layers.cpp
@@ +410,2 @@
>  void
> +ContainerLayer::SetAsyncPanZoomController(AsyncPanZoomController *controller)

You'll need to update the call at http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#2496 as well to not break fennec builds.

::: gfx/layers/Layers.h
@@ +1407,5 @@
>    }
>  
> +  // These functions allow attaching an AsyncPanZoomController to this layer,
> +  // and can be used anytime.
> +  // A container layer has an APZC if-and-only-if GetFrameMetrics().IsScrollable()

I don't know if this comment is correct. Shouldn't it say "A container layer can have an APZC only if GetFrameMetrics().IsScrollable()"? Because not all scrollable container layers *will* have an APZC all the time, and this comment implies that is the case.
Attachment #762498 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Comment on attachment 762503 [details] [diff] [review]
> Part 1: Create ScrollInfoLayers for frames we want APZC support
> 
> Review of attachment 762503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +2226,5 @@
> > +      (scrollRange.width > 0 || scrollRange.height > 0) &&
> > +      ((styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN && mHScrollbarBox) ||
> > +       (styles.mVertical   != NS_STYLE_OVERFLOW_HIDDEN && mVScrollbarBox));
> > +    // TODO Turn this on for inprocess OMTC
> > +    bool wantSubAPZC = (XRE_GetProcessType() == GeckoProcessType_Content);
> 
> I assume this wantSubAPZC condition will eventually change to be more like
> "was a sub apzc requested on this scroll frame by the parent apzc", is that
> right? i.e. this condition is more or less a placeholder for something that
> will come later.

It's not a placeholder but it will need to be true for any platform where we plan on using Multi-APZC so later we may use a preference or build flag for fennec. Building a layer here only builds a scroll info layer that we can use to do hit testing and have a unique ID to request a display port. We don't rasterize content when we build a layer here.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> ::: gfx/layers/Layers.h
> @@ +1407,5 @@
> >    }
> >  
> > +  // These functions allow attaching an AsyncPanZoomController to this layer,
> > +  // and can be used anytime.
> > +  // A container layer has an APZC if-and-only-if GetFrameMetrics().IsScrollable()
> 
> I don't know if this comment is correct. Shouldn't it say "A container layer
> can have an APZC only if GetFrameMetrics().IsScrollable()"? Because not all
> scrollable container layers *will* have an APZC all the time, and this
> comment implies that is the case.

I was planning on enforcing iff relationship but you're right that in some cases we might prefer to construct the APZC only if we really need it. I'll change this.
Attachment #762503 - Attachment is obsolete: true
Attachment #762841 - Flags: review+
Attachment #762841 - Attachment description: Create ScrollInfoLayers for frames we want APZC support → Part 1: Create ScrollInfoLayers for frames we want APZC support
Attachment #762498 - Attachment is obsolete: true
Attachment #762842 - Flags: review+
Forgot to fix the comment
Attachment #762842 - Attachment is obsolete: true
Attachment #762847 - Flags: review+
Comment on attachment 762847 [details] [diff] [review]
Part 2: Make APZC attachable only to container layers v2

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

::: gfx/layers/Layers.cpp
@@ +422,2 @@
>    }
> +#endif

MOZ_ASSERT(!mAPZC || GetFrameMetrics().IsScrollable());
Attachment #764385 - Flags: review?(bugmail.mozilla)
Fixed pass by value instead of reference causing use-after-free.
Attachment #764385 - Attachment is obsolete: true
Attachment #764385 - Flags: review?(bugmail.mozilla)
Attachment #764418 - Flags: review?(bugmail.mozilla)
Comment on attachment 764385 [details] [diff] [review]
Part 3: Implement hit testing on layer tree + tests

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

Generally looks good. I'd like to figure out the type of aRelativePointOut before r+'ing though. See below.

::: gfx/layers/Makefile.in
@@ +23,5 @@
>  LIBRARY_NAME   = layers
>  MSVC_ENABLE_PGO := 1
>  LIBXUL_LIBRARY = 1
>  FORCE_STATIC_LIB = 1
> +MOZ_OPTIMIZE_FLAGS += -O0 -g

Is this left over from debugging?

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1525,5 @@
> +
> +  if (intersect) {
> +    if (aLayer.GetFrameMetrics().IsScrollable()) {
> +      *aApzcOut = aLayer.GetAsyncPanZoomController();
> +      *aRelativePointOut = CSSIntPoint(NS_lround(layerPoint.x), NS_lround(layerPoint.y));

This doesn't look right... You're taking a layer point and returning it as a CSS point without any conversion. I think aRelativePointOut should be a LayerPoint or LayerIntPoint.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +262,5 @@
> +   */
> +  static void GetAPZCAtPoint(const ContainerLayer& aLayerTree,
> +                             const ScreenIntPoint& aPoint,
> +                             AsyncPanZoomController** aApzcOut,
> +                             CSSIntPoint* aRelativePointOut);

Add a comment here clarifying that aRelativePointOut is still relative to the scroll origin, not to the specific layer that has aApzcOut. This was unclear to me until I saw what the test was doing.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +43,5 @@
>  FrameMetrics TestFrameMetrics() {
>    FrameMetrics fm;
>  
> +  fm.mDisplayPort = CSSRect(0, 0, 10, 10);
> +  fm.mCompositionBounds = LayerIntRect(0, 0, 10, 10);

I changed mCompositionBounds to a ScreenIntRect (bug 880676), so this will need to be updated as well.

@@ +297,5 @@
> +  EXPECT_EQ(apzcOut, nullptr);
> +
> +
> +  // Now we have a root APZC that will match the page
> +  scrollable.mScrollId = 1;

Use FrameMetrics::ROOT_SCROLL_ID instead of 1 here because otherwise it's basically a magic number

@@ +306,5 @@
> +  EXPECT_EQ(apzcOut, apzcMain.get());
> +  EXPECT_EQ(CSSIntPoint(20, 20), relativePointOut);
> +
> +  // Now we have a sub APZC with a better fit
> +  scrollable.mScrollId++;

Here again I would prefer setting this to FrameMetrics::START_SCROLL_ID because it probably needs to match up. After setting it to START_SCROLL_ID incrementing it should be fine for additional subframes.

@@ +363,5 @@
> +
> +  // Scale layer[4] outside the range
> +  layers[4]->SetBaseTransform(transform);
> +  AsyncPanZoomController::GetAPZCAtPoint(*root->AsContainerLayer(), touchPoint,
> +                 &apzcOut, &relativePointOut);

It might help here to have a comment saying that the effective transform on layers[4] puts it at (.05, .05, .2, .2) and so does not contain (2, 2).

@@ +384,5 @@
> +  gfx3DMatrix translateTransform3;
> +  translateTransform3.ScalePost(1,15,1);
> +  layers[7]->SetBaseTransform(translateTransform3);
> +
> +  touchPoint = ScreenIntPoint(1,45);

Again, comments indicating what the effective transform on layer 7 is here would be useful.

::: gfx/tests/gtest/TestLayers.cpp
@@ +61,5 @@
> +    }
> +    if (!mFirstChild) {
> +      mFirstChild = aChild;
> +    }
> +    if (mLastChild) {

I would also do a MOZ_CRASH | if (aAfter != nullptr && aAfter != mLastChild) | because otherwise people might expect those cases to work correctly.

@@ +209,5 @@
> +  nsRefPtr<Layer> lastLayer = nullptr;
> +  int layerNumber = 0;
> +  for (size_t i = 0; i < strlen(aLayerTreeDescription); i++) {
> +    if (aLayerTreeDescription[i] == '(') {
> +      parentContainerLayer = lastLayer->AsContainerLayer();

lastLayer could be null here if somebody started their layer description with "(". Not a big deal but since you're catching the other error below you might as well catch this one too.

@@ +215,5 @@
> +        printf("Layer before '(' must be a container.\n");
> +        MOZ_CRASH();
> +      }
> +    } else if (aLayerTreeDescription[i] == ')') {
> +      parentContainerLayer = parentContainerLayer->GetParent();

Also set lastLayer to null here, so that descriptions that contain the sequence ")(" are always invalid.

::: gfx/tests/gtest/TestLayers.h
@@ +11,5 @@
> +
> +// Create layer tree from a simple layer tree description syntax.
> +// Each index is either a the first letter of the layer type or
> +// a '(',')' to indicate the start/end of the child layers.
> +// The aim of this function is to remove and hard to read

s/remove and hard/remove hard/

@@ +16,5 @@
> +// layer tree creation code.
> +//
> +// Example "c(c(c(tt)t))" would yield:
> +//       / t  / t
> +//  c - c - c - t

This tree representation is kinda hard to read since it's horizontal.. maybe do it more vertically?
Attachment #764385 - Attachment is obsolete: false
Comment on attachment 764385 [details] [diff] [review]
Part 3: Implement hit testing on layer tree + tests

Mid-aired with your new patch upload. I think all the comments still apply to the new version.
Attachment #764385 - Attachment is obsolete: true
Attachment #764418 - Flags: review?(bugmail.mozilla) → review-
Attachment #764418 - Attachment is obsolete: true
Attachment #766933 - Flags: review?(bugmail.mozilla)
Comment on attachment 766933 [details] [diff] [review]
Part 3: Implement hit testing on layer tree + tests v3

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1526,5 @@
> +  if (intersect) {
> +    if (aLayer.GetFrameMetrics().IsScrollable()) {
> +      *aApzcOut = aLayer.GetAsyncPanZoomController();
> +      *aRelativePointOut = LayerIntPoint(NS_lround(layerPoint.x), NS_lround(layerPoint.y));
> +      return;

Unnecessary return statement.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +23,5 @@
>    MOCK_METHOD1(RequestContentRepaint, void(const FrameMetrics&));
> +  MOCK_METHOD1(HandleDoubleTap, void(const CSSIntPoint&));
> +  MOCK_METHOD1(HandleSingleTap, void(const CSSIntPoint&));
> +  MOCK_METHOD1(HandleLongTap, void(const CSSIntPoint&));
> +  MOCK_METHOD2(SendAsyncScrollDOMEvent, void(const CSSRect &aContentRect, const CSSSize &aScrollableSize));

These changes that convert nsIntPoint -> CSSIntPoint are already in the tree now, so they can be removed from this patch.

@@ +327,5 @@
> +                 &apzcOut, &relativePointOut);
> +  EXPECT_EQ(apzcOut, apzcSub4.get());
> +  EXPECT_EQ(LayerIntPoint(15, 15), relativePointOut);
> +
> +  // Hit test ouside the reach of apzc1/2 but inside apzcMain

I think this comment should say "apzcSub3/4" instead of "apzc1/2"

@@ +387,5 @@
> +  translateTransform3.ScalePost(1,15,1);
> +  layers[7]->SetBaseTransform(translateTransform3);
> +
> +  touchPoint = ScreenIntPoint(1,45);
> +  // layer 7 effective visible screenrect (2,0,4,60) but clip by parent layers

According to my calculations the screenrect for layer 7 should be (0,16,4,60).

It starts off at (10,10,40,40). Then you apply the scale of (1,15,1) which puts it at (10,150,40,600). Then there's a translate of (-20, 0, 0) and (10, 10, 0) which moves it to (0, 160, 40, 600). And finally the root layer's scale of (0.1, 0.1, 1) shrinks it down to (0, 16, 4, 60).

Applying the transforms in reverse to the (1,45) screenpoint gives (20,29) as expected, so that part is correct.

::: gfx/tests/gtest/TestLayers.cpp
@@ +191,5 @@
> +// Create layer tree from a simple layer tree description syntax.
> +// Each index is either the first letter of the layer type or
> +// a '(',')' to indicate the start/end of the child layers.
> +// The aim of this function is to remove hard to read
> +// layer tree creation code.

Move this comment from TestLayers.cpp to TestLayers.h. You have the same comment there but it's the version from the previous patch. I'd rather have just one copy (this one) in the .h file and none in the .cpp
Attachment #766933 - Flags: review?(bugmail.mozilla) → review+
Depends on: 888622
rebased on mc + 888622
Attachment #762841 - Attachment is obsolete: true
Attachment #770215 - Flags: review+
There's an assertion failure with part 1. Trying to land part 2+3 and if that works I'll move part 1 to a different bug since it never really belonged here in the first place:
https://tbpl.mozilla.org/?tree=Try&rev=bf914b747ae8
This has the review comments from Kats.
Attachment #770226 - Attachment is obsolete: true
Attachment #770412 - Flags: review+
I just realized that part 3 here is not actually correct, with respect to the return value of aRelativePointOut. In the case where there is no CSS transforms set it's fine, but once CSS transforms are applied, then the aLayer.GetLocalTransform().Inverse() will include the un-transformation of those transforms, which we don't really want to do. Layout is already aware of those transforms and will unapply them during its own hit testing, so the aRelativePointOut that we want to return shouldn't have them unapplied.

I think the aRelativePointOut value should be the aPoint untransformed by the ViewTransform that SampleContentTransformForFrame returns - that is the async pan/zoom transform, which doesn't include any CSS transforms. Matt Woodrow pointed me to nsDisplayTransform::HitTest [1] which is the code that does the CSS un-transform for hit testing, so the point that we pass to layout should still include CSS transforms.

(Another way of looking at this is that the current patch will return a LayerPoint, but it's actually in the "LayerPoint minus CSS transforms" coordinate space).

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#4002
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)

Yes that's right. I suggest that we fix this after landing in a follow-up with a test to make sure we always do this properly.
Compiler fiddling for b2g's g++:
https://tbpl.mozilla.org/?tree=Try&rev=438d748f3a5d
Attachment #770412 - Attachment is obsolete: true
Attachment #770876 - Flags: review+
Attachment #770876 - Attachment is obsolete: true
Attachment #770929 - Flags: review+
Fixed the failure. Silly name duplication.
Attachment #770929 - Attachment is obsolete: true
Attachment #771025 - Flags: review+
Blocks: 890279
Blocks: 890280
https://hg.mozilla.org/mozilla-central/rev/63e0e9a4b257
https://hg.mozilla.org/mozilla-central/rev/e58c025b75b5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 904184
You need to log in before you can comment on or make changes to this bug.