Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 1 • 12 years ago

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.

	Joe Drew (not getting mail)
	Comment 2 • 12 years ago

This doesn't sound like it necessarily blocks, but please nom if I'm wrong.

	Benoit Girard (:BenWa) Assignee
	Comment 3 • 11 years ago

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.

	Joe Drew (not getting mail)
	Comment 6 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 7 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3344f97b4be

	Ed Morley [:emorley]
	Comment 8 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/e3344f97b4be

	Benoit Girard (:BenWa) Assignee
	Comment 10 • 11 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 11 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 12 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 14 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 15 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 16 • 11 years ago

(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.

	Benoit Girard (:BenWa) Assignee
	Comment 17 • 11 years ago

(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.

	Benoit Girard (:BenWa) Assignee
	Comment 20 • 11 years ago

Forgot to fix the comment

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 21 • 11 years ago

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());

	Benoit Girard (:BenWa) Assignee
	Comment 23 • 11 years ago

Fixed pass by value instead of reference causing use-after-free.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 24 • 11 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 25 • 11 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 27 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 28 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 29 • 11 years ago

rebased on mc + 888622

	Benoit Girard (:BenWa) Assignee
	Comment 30 • 11 years ago

Rebased. Try:
https://tbpl.mozilla.org/?tree=Try&rev=974aebe736a2

	Benoit Girard (:BenWa) Assignee
	Comment 31 • 11 years ago

Rebased + try:
https://tbpl.mozilla.org/?tree=Try&rev=7a1fe5ac5af9

	Benoit Girard (:BenWa) Assignee
	Comment 32 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 33 • 11 years ago

This has the review comments from Kats.

	Benoit Girard (:BenWa) Assignee
	Comment 34 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 35 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 36 • 11 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 37 • 11 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 38 • 11 years ago

Ok, sounds good.

	Benoit Girard (:BenWa) Assignee
	Comment 39 • 11 years ago

Compiler fiddling for b2g's g++:
https://tbpl.mozilla.org/?tree=Try&rev=438d748f3a5d

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 41 • 11 years ago

I think the fix for the issue in comment 36 should look something like this: https://github.com/staktrace/mozilla-central/commit/975d8df103d81fd9f50badd82e9fd82bcc544fd6

	Benoit Girard (:BenWa) Assignee
	Comment 42 • 11 years ago

Fixed the failure. Silly name duplication.

	Benoit Girard (:BenWa) Assignee
	Comment 43 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/63e0e9a4b257
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e58c025b75b5

	Phil Ringnalda (:philor)
	Comment 44 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/63e0e9a4b257
https://hg.mozilla.org/mozilla-central/rev/e58c025b75b5