1090398 - Update APZ code/API to deal with dispatch-to-content regions if event regions are enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

As part of the plan for proper APZ hit-testing (bug 918288) we need to track the dispatch-to-content regions in the APZ code, and ensure that:
(1) if a touch input block starts in such a region, we wait for the main thread to get back to us with prevent-default state and confirmation of the hit target and
(2) if a touch input block starts in a regular hit region that's NOT a dispatch-to-content region, we process it immediately (obviously assuming any previous input blocks have already been processed).

Part of this also involves providing an API for gecko/widget/main-thread code to notify the APZ of the hit target in d-t-c cases.

Comment 1

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

Attachment: Part 1 - Use EventRegions instead of nsIntRegions

Comment 2

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

Attachment: Part 2 - Use layer event regions if provided

Comment 3

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

Attachment: Part 3 - Small refactoring

Comment 4

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

Attachment: Part 4 - Workaround for bug 1082594

Comment 5

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

Attachment: Part 5 - Distinguish hit test results more explicitly

Comment 6

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

Attachment: Part 6 - Require APZ target confirmation

This patch has evolved significantly since I first proposed it in bug 1083395, so I'm glad you asked me to push it to later :)

I'm going to try to write some gtests for this code as well, but I'm flagging you for review anyway now since it may take a bit of time to digest these. I'll also do a try push and more manual testing.

There's one more remaining piece which is the code that will actually set the confirmed APZC from the gecko side. I'll have to discuss that with roc and/or mstange first to figure out how to make a scrollable subframe active in the cases where we need to do that.

Comment 7

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

Manual testing is clean, try push:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=46e40619ef4e

Comment 8

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

Attachment: Part 7 - Basic gtest to exercise the event regions-based hit-testing code

The promised gtest.

Comment 9

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

Attachment: Part 0 - Update documentation to reflect changes in design

Comment 10

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

Pretty version of the docs with part 0: https://github.com/staktrace/gecko-dev/blob/a61a747/gfx/doc/AsyncPanZoom.md

Comment 11

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

Attachment: Part 0 - Update documentation to reflect changes in design

Rebased on top of the minor edits from bug 1094803.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8520194 [details] [diff] [review]
Part 0 - Update documentation to reflect changes in design

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

Looks good! The comments are mostly questions about the design itself that I'm curious about, rather than comments about the documentation.

::: gfx/doc/AsyncPanZoom.md
@@ +200,5 @@
>     These may trigger behaviours like scrolling or tap gestures.
>    </li>
>    <li value="ii">
> +   If the input events landed inside the dispatch-to-content event region for the layer, the events are left in the queue and a 300ms timeout is initiated.
> +   If the timeout expires before step 9 is completed, the APZ assumes the input block was not cancelled and the tentative target is correct, and processes them as part of step 10.

Does this mean that if a touch event handler takes too long to run, we can actually get incorrect hit testing results?

@@ +219,5 @@
>  </li>
>  <li value="8">
> +The call stack unwinds back to the widget code, which sends two notifications to the APZ code on the input thread.
> +The first notification is via APZCTreeManager::ContentReceivedTouch, and informs the APZ whether the input block was cancelled.
> +The second notification is via APZCTreeManager::SetTargetAPZC, and informs the APZ the results of the Gecko hit-test during event dispatch.

Is there a reason the notifications need to be / should be separate?

@@ +246,5 @@
>  
>  If the CSS touch-action property is enabled, the above steps are modified as follows:
>  <ul>
>  <li>
> + In step 4(i), the input events may land inside an touch-action event region for the layer. In this case, the events in the input block are processed, subject to the allowed touch-action behaviors.

Are the allowed touch-action behaviours available at this point? The previous wording suggested they weren't.

@@ +276,5 @@
>  Therefore, scrollframes as designated as either "active" or "inactive".
>  Active scrollframes are the ones that do have their contents put on a separate layer (or set of layers), and inactive ones do not.
>  
>  Consider a page with a scrollframe that is initially inactive.
> +When layout generates the layers for this page, the content of the scrollframe will be flattened some other PaintedLayer (call it P).

flattened into

@@ +277,5 @@
>  Active scrollframes are the ones that do have their contents put on a separate layer (or set of layers), and inactive ones do not.
>  
>  Consider a page with a scrollframe that is initially inactive.
> +When layout generates the layers for this page, the content of the scrollframe will be flattened some other PaintedLayer (call it P).
> +The layout code also adds the area of the scrollframe to the dispatch-to-content region of P.

Maybe mention that if the "area of the scrollframe" is not representable as a region (i.e. has a weird shape), then it returns a region that bounds this area?

@@ +290,5 @@
> +
> +This model implies that when the user initially attempts to scroll an inactive scrollframe, it may end up scrolling an ancestor scrollframe.
> +(This is because in the absence of the SetTargetAPZC notification, the input events will get applied to the closest ancestor scrollframe's APZC.)
> +Only after the round-trip to the gecko thread is complete is there a layer for async scrolling to actually occur on the scrollframe itself.
> +At that point the scrollframe will start receiving new input blocks and will scroll normally.

If the SetTargetAPZC notification arrives before the layer transaction, can we detect that (by seeing that the scroll-id is not present in the APZC tree), "save" the notification, and apply it when we get the layer transaction?

Comment 13

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

(In reply to Botond Ballo [:botond] from comment #12)
> ::: gfx/doc/AsyncPanZoom.md
> @@ +200,5 @@
> >     These may trigger behaviours like scrolling or tap gestures.
> >    </li>
> >    <li value="ii">
> > +   If the input events landed inside the dispatch-to-content event region for the layer, the events are left in the queue and a 300ms timeout is initiated.
> > +   If the timeout expires before step 9 is completed, the APZ assumes the input block was not cancelled and the tentative target is correct, and processes them as part of step 10.
> 
> Does this mean that if a touch event handler takes too long to run, we can
> actually get incorrect hit testing results?

Yes and no. If the SetTargetAPZC notification is sent but the handler takes too long to run, we will still respect the SetTargetAPZC notification. However the way I was planning to implement that bit involved always sending the SetTargetAPZC notification after running the touch listener, so in practice a touch listener taking too long would in fact potentially cause incorrect hit testing. I can probably change it to send the SetTargetAPZC notification first which would help in this respect, but I think incorrect hit testing is just something we'll have to live with in some scenarios. It's one of the inherent tradeoffs here, just like how we can ignore preventDefaults if they take too long. Hopefully we won't hit those cases too often.

> > +The first notification is via APZCTreeManager::ContentReceivedTouch, and informs the APZ whether the input block was cancelled.
> > +The second notification is via APZCTreeManager::SetTargetAPZC, and informs the APZ the results of the Gecko hit-test during event dispatch.
> 
> Is there a reason the notifications need to be / should be separate?

We could probably combine them if we really wanted to. However, there is one advantage to keeping them separate - recall that content can call preventDefault on the touchstart *or* the first touchmove. If we have a separate SetTargetAPZC then we can get that info to the APZ code earlier, and so even if the first touchmove handler takes 500ms we won't use an incorrect hit test result.

> @@ +246,5 @@
> >  
> >  If the CSS touch-action property is enabled, the above steps are modified as follows:
> >  <ul>
> >  <li>
> > + In step 4(i), the input events may land inside an touch-action event region for the layer. In this case, the events in the input block are processed, subject to the allowed touch-action behaviors.
> 
> Are the allowed touch-action behaviours available at this point? The
> previous wording suggested they weren't.

I went a bit overboard here I guess. Part of bug 928833 which we haven't dealt with yet involves expanding the EventRegions struct to include regions for the various touch-action areas. Once that is done the touch-action flow will be like what I described in this doc. Until that is done though the flow is unchanged. I can take out that hunk of changes to the doc and save it for later.

> If the SetTargetAPZC notification arrives before the layer transaction, can
> we detect that (by seeing that the scroll-id is not present in the APZC
> tree), "save" the notification, and apply it when we get the layer
> transaction?

We might be able to, yeah. I'm still exploring the different ways I can implement that part (it's not part of this bug, but it will be part of bug 918288).

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8512912 [details] [diff] [review]
Part 1 - Use EventRegions instead of nsIntRegions

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

::: gfx/layers/LayersTypes.h
@@ +157,5 @@
>  struct EventRegions {
>    nsIntRegion mHitRegion;
>    nsIntRegion mDispatchToContentHitRegion;
>  
> +  explicit EventRegions()

'explicit' on a zero-argument constructor doesn't do anything.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8512913 [details] [diff] [review]
Part 2 - Use layer event regions if provided

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

It took me a while to understand the changes to UPZCT. I would suggest adding a few clarifying comments:

  - Mention that the purpose of the mEventRegions stack is to accumulate
    the event regions of the descendants of a layer into the regions of
    that layer, and that this is necessary because the event regions of
    a layer don't necessarily include those of its children.

     - Mention that in UPZCT, immediately after the loop over the 
       children, mEventRegions.LastElement() contains the accumulated
       regions of the (non-APZC) descendants of |aLayer|.

  - Mention that if a layer has an APZC, we skip accumulating its event
    regions into those of its parent, but due to the children-first
    way we do hit testing, it should still be correct if we included them.

  - Mention that when |obscured| is subtracted after the loop, it
    includes the event regions of the children, but again due to the
    children-first way we do hit testing, it should still be correct
    if we subtracted |obscured| before the loop, where it only contains
    the event regions of siblings (and uncles and such) that are above
    the layer.

::: gfx/layers/LayerMetricsWrapper.h
@@ +303,5 @@
> +  {
> +    MOZ_ASSERT(IsValid());
> +
> +    if (AtBottomLayer()) {
> +      return mLayer->GetEventRegions();

I think this merits a comment: we attribute the event-regions to the innermost (bottom) metrics, because we want touch events to go to its APZC in preference to the ones above.

::: gfx/layers/Layers.cpp
@@ +1476,5 @@
>    } else {
>      aStream << " [not visible]";
>    }
> +  if (!mEventRegions.IsEmpty()) {
> +    AppendToString(aStream, mEventRegions, " ", "");

Won't this look cryptic when both regions are empty (just a "{}" with no label)?

::: gfx/layers/LayersTypes.h
@@ +188,5 @@
> +    mHitRegion.AndWith(aRegion);
> +    mDispatchToContentHitRegion.AndWith(aRegion);
> +  }
> +
> +  void Sub(const EventRegions& aMinuend, const nsIntRegion& aSubtrahend)

I learn new words every day :)

@@ +200,5 @@
> +    mHitRegion.Transform(aTransform);
> +    mDispatchToContentHitRegion.Transform(aTransform);
> +  }
> +
> +  bool IsEmpty()

const

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +494,5 @@
> +    EventRegions unobscured;
> +    unobscured.Sub(aLayer.GetEventRegions(), obscured);
> +    APZCTM_LOG("Picking up unobscured hit region %s from layer %p\n", Stringify(unobscured).c_str(), aLayer.GetLayer());
> +
> +    // Take the hit region of the layer subtree (which has already been

Clarify that you're talking about the layer subtree rooted at |aLayer|.

@@ +516,5 @@
> +      const CompositorParent::LayerTreeState* state = CompositorParent::GetIndirectShadowTree(aLayersId);
> +      MOZ_ASSERT(state);
> +      MOZ_ASSERT(state->mController.get());
> +      CSSRect touchSensitiveRegion;
> +      if (state->mController->GetTouchSensitiveRegion(&touchSensitiveRegion)) {

I remember us saying at one point that the touch-sensitive region in the GeckoContentController was a temporary hack that will go away when we have proper hit-testing. Do we still need it if event-regions are enabled?

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8512915 [details] [diff] [review]
Part 4 - Workaround for bug 1082594

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +395,5 @@
>  
>    return apzc;
>  }
>  
> +static EventRegions

Please add a comment saying this is a workaround for bug 1082594, and that it can be replaced with |aLayer.GetEventRegions()| once that bug is fixed.

@@ +400,5 @@
> +EventRegionsFor(const LayerMetricsWrapper& aLayer)
> +{
> +  if (aLayer.IsScrollInfoLayer()) {
> +    EventRegions regions(ParentLayerIntRect::ToUntyped(RoundedIn(aLayer.Metrics().mCompositionBounds)));
> +    regions.mDispatchToContentHitRegion = regions.mHitRegion;

Might make sense to give EventRegions a two-argument constructor.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8512917 [details] [diff] [review]
Part 5 - Distinguish hit test results more explicitly

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

Nice!

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +381,3 @@
>    already_AddRefed<AsyncPanZoomController> GetTargetAPZC(const ScrollableLayerGuid& aGuid);
>    already_AddRefed<AsyncPanZoomController> GetTargetAPZC(const ScreenPoint& aPoint,
> +                                                         HitTestResult* aHitResult);

aOutHitResult here and elsewhere

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8512922 [details] [diff] [review]
Part 6 - Require APZ target confirmation

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

This generally looks good. I'd like to hear about the caller(s) of APZCTreeManager::SetTargetAPZC, and the handling of pan gestures, before giving the r+.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +616,5 @@
>          PanGestureInput inputForApzc(panInput);
>          transformToApzc = GetScreenToApzcTransform(apzc);
>          ApplyTransform(&(inputForApzc.mPanStartPoint), transformToApzc);
> +        result = mInputQueue->ReceiveInputEvent(
> +            apzc, hitResult == ApzcHitRegion, inputForApzc, aOutInputBlockId);

My preference would be to write /* aTargetConfirmed = */ in these calls too, but I'll leave it up to you.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +217,5 @@
> +   * where the initial input event of the block hit a dispatch-to-content region
> +   * but is safe to call for all input blocks. This function should always be
> +   * invoked on the controller thread.
> +   */
> +  void SetTargetAPZC(uint64_t aInputBlockId, const ScrollableLayerGuid& aGuid);

I would, once again, be more comfortable reviewing this if I saw how it's called.

If that's impractical, I'll settle for a description of how it's called. In particular:  
  - where does the caller get the guid from?
  - why are we allowing the guid to refer to a non-existent APZC
    (i.e. why are we allowing a guid such that GetTargetAPZC()
    returns null?)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1465,5 @@
>      // SMOOTH_SCROLL scrolls are cancelled by pan gestures.
>      CancelAnimation();
>    }
>  
> +  mPanGestureState = MakeUnique<InputBlockState>(this, true);

What happens when a PANGESTURE_START hits a dispatch-to-content region?

::: gfx/layers/apz/src/InputQueue.cpp
@@ +67,5 @@
> +      waitForMainThread |= aTarget->NeedToWaitForContent();
> +    }
> +    if (waitForMainThread) {
> +      // We either don't know for sure if aTarget is the right APZC, or we may
> +      // need to wait for content to intercept the touch events and prevent-

I'd phrase it as "we need to wait to give content the opportunity to prevent-default the touch events".

@@ +125,5 @@
>  
>  uint64_t
>  InputQueue::InjectNewTouchBlock(AsyncPanZoomController* aTarget)
>  {
> +  TouchBlockState* block = StartNewTouchBlock(aTarget, true, true);

May want to start doing 

  /* argName = */ true

for readability.

@@ +193,5 @@
> +      // time out the touch-listener response and also confirm the existing
> +      // target apzc in the case where the main thread doesn't get back to us
> +      // fast enough.
> +      success = mTouchBlockQueue[i]->TimeoutContentResponse()
> +          | mTouchBlockQueue[i]->SetTargetApzc(mTouchBlockQueue[i]->GetTargetApzc());

||

::: gfx/layers/apz/src/InputQueue.h
@@ +57,5 @@
> +   * generally be decidable upon receipt of the input event, but in some cases
> +   * we may need to query the layout engine to know for sure. The input block
> +   * this applies to should be specified via the |aInputBlockId| parameter.
> +   */
> +  void SetTargetApzc(uint64_t aInputBlockId, const nsRefPtr<AsyncPanZoomController>& aTargetApzc);

Might it make more sense to call this (and the method in InputBlockState) ConfirmTargetAPZC?

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8512922 [details] [diff] [review]
Part 6 - Require APZ target confirmation

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

::: gfx/layers/apz/src/InputQueue.cpp
@@ +179,1 @@
>    INPQ_LOG("scheduling content response timeout for target %p\n", aTarget.get());

to be consistent, s/content response/main thread

@@ +187,3 @@
>    AsyncPanZoomController::AssertOnControllerThread();
>  
>    INPQ_LOG("got a content response timeout; block=%" PRIu64 "\n", aInputBlockId);

likewise

@@ +193,5 @@
> +      // time out the touch-listener response and also confirm the existing
> +      // target apzc in the case where the main thread doesn't get back to us
> +      // fast enough.
> +      success = mTouchBlockQueue[i]->TimeoutContentResponse()
> +          | mTouchBlockQueue[i]->SetTargetApzc(mTouchBlockQueue[i]->GetTargetApzc());

Oh, I see, we don't want short-circuit evaluation.

That's a bit subtle for my taste. Please add a comment saying that we're using '|' to avoid short-circuit evaluation, or separate into two statements (success = ...; success |= ...;).

Comment 20

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

(In reply to Botond Ballo [:botond] from comment #18)
> > +  void SetTargetAPZC(uint64_t aInputBlockId, const ScrollableLayerGuid& aGuid);
> 
> I would, once again, be more comfortable reviewing this if I saw how it's
> called.
> 
> If that's impractical, I'll settle for a description of how it's called. In
> particular:  
>   - where does the caller get the guid from?
>   - why are we allowing the guid to refer to a non-existent APZC
>     (i.e. why are we allowing a guid such that GetTargetAPZC()
>     returns null?)

I have WIPs on bug 918288 that might illustrate this. The caller gets the guid primarily from figuring out the scrollable element under the touchstart. And in some cases there may be no such scrollable element at all, so GetTargetAPZC could return null. (e.g. there's a d-t-c region for a border-radius, and the gecko hit-test decides it's outside the APZC layer and on a non-scrollable layer, so there's no APZC for it).

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +1465,5 @@
> >      // SMOOTH_SCROLL scrolls are cancelled by pan gestures.
> >      CancelAnimation();
> >    }
> >  
> > +  mPanGestureState = MakeUnique<InputBlockState>(this, true);
> 
> What happens when a PANGESTURE_START hits a dispatch-to-content region?

For now I didn't bother doing anything with non-touch events. I was thinking that we could construct input blocks out of PANGESTURE events as well and deal with them in the same way as touch events but I haven't implemented that since I was mostly focused on B2G.

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8515126 [details] [diff] [review]
Part 7 - Basic gtest to exercise the event regions-based hit-testing code

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

Nice test!

It might be worth also having a test where an APZC parent layer has a non-APZC child layer, and the parent's hit region does not include the child's hit region; the test would check that tapping on the child's hit region delivers the event to the parent's APZC (i.e., it would test that the whole "accumulating the event regions in the subtree" code from Part 2 is working).

Comment 22

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> (In reply to Botond Ballo [:botond] from comment #18)
> > > +  void SetTargetAPZC(uint64_t aInputBlockId, const ScrollableLayerGuid& aGuid);
> > 
> > I would, once again, be more comfortable reviewing this if I saw how it's
> > called.
> > 
> > If that's impractical, I'll settle for a description of how it's called. In
> > particular:  
> >   - where does the caller get the guid from?
> >   - why are we allowing the guid to refer to a non-existent APZC
> >     (i.e. why are we allowing a guid such that GetTargetAPZC()
> >     returns null?)
> 
> I have WIPs on bug 918288 that might illustrate this. The caller gets the
> guid primarily from figuring out the scrollable element under the
> touchstart. And in some cases there may be no such scrollable element at
> all, so GetTargetAPZC could return null. (e.g. there's a d-t-c region for a
> border-radius, and the gecko hit-test decides it's outside the APZC layer
> and on a non-scrollable layer, so there's no APZC for it).

OK, that's reasonable. We should probably ask that the caller passes in something specific, such as NULL_SCROLL_ID, to indicate that the touch hit non-scrollable content (and document this in the comment above the declaration of APZCTreeManager::SetTargetAPZC).

> > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> > @@ +1465,5 @@
> > >      // SMOOTH_SCROLL scrolls are cancelled by pan gestures.
> > >      CancelAnimation();
> > >    }
> > >  
> > > +  mPanGestureState = MakeUnique<InputBlockState>(this, true);
> > 
> > What happens when a PANGESTURE_START hits a dispatch-to-content region?
> 
> For now I didn't bother doing anything with non-touch events. I was thinking
> that we could construct input blocks out of PANGESTURE events as well and
> deal with them in the same way as touch events but I haven't implemented
> that since I was mostly focused on B2G.

Please add a TODO (and maybe file a bug) so we don't forget.

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8512922 [details] [diff] [review]
Part 6 - Require APZ target confirmation

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

r+ with relevant comments addressed. Thanks!

Comment 24

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

(In reply to Botond Ballo [:botond] from comment #12)
> > + In step 4(i), the input events may land inside an touch-action event region for the layer. In this case, the events in the input block are processed, subject to the allowed touch-action behaviors.
> 
> Are the allowed touch-action behaviours available at this point? The
> previous wording suggested they weren't.

I removed some of this and will save it for when we add touch-action regions to the EventRegions.

> flattened into

Fixed.

> Maybe mention that if the "area of the scrollframe" is not representable as
> a region (i.e. has a weird shape), then it returns a region that bounds this
> area?

Done.

(In reply to Botond Ballo [:botond] from comment #14)
> > +  explicit EventRegions()
> 
> 'explicit' on a zero-argument constructor doesn't do anything.

Removed.(In reply to Botond Ballo [:botond] from comment #15)
> It took me a while to understand the changes to UPZCT. I would suggest
> adding a few clarifying comments:

I added these comments to the relevant code in APZCTM.cpp.

> > +    if (AtBottomLayer()) {
> > +      return mLayer->GetEventRegions();
> 
> I think this merits a comment: we attribute the event-regions to the
> innermost (bottom) metrics, because we want touch events to go to its APZC
> in preference to the ones above.

So I don't think that's really correct. It's simpler: we attribute the event-regions to the innermost layer because as per the contract of LayerMetricsWrapper, all the layers above it are empty container layers. Since they have no content, they must have empty event regions. The relationship to APZCs and touch-events is not relevant here.

> > +  if (!mEventRegions.IsEmpty()) {
> > +    AppendToString(aStream, mEventRegions, " ", "");
> 
> Won't this look cryptic when both regions are empty (just a "{}" with no
> label)?

I don't think so, there's a guard against that (see quoted code above). :)

> > +  void Sub(const EventRegions& aMinuend, const nsIntRegion& aSubtrahend)
> 
> I learn new words every day :)

I had to look it up! I didn't like the existing naming convention in nsRegion...

> > +  bool IsEmpty()
> 
> const

Done

> > +    // Take the hit region of the layer subtree (which has already been
> 
> Clarify that you're talking about the layer subtree rooted at |aLayer|.

Done

> I remember us saying at one point that the touch-sensitive region in the
> GeckoContentController was a temporary hack that will go away when we have
> proper hit-testing. Do we still need it if event-regions are enabled?

Not sure, to be honest. I'll have to check - but regardless I'd like to remove that as a separate bug if it is removable.

(In reply to Botond Ballo [:botond] from comment #16)
> >  
> > +static EventRegions
> 
> Please add a comment saying this is a workaround for bug 1082594, and that
> it can be replaced with |aLayer.GetEventRegions()| once that bug is fixed.

Done

> > +    EventRegions regions(ParentLayerIntRect::ToUntyped(RoundedIn(aLayer.Metrics().mCompositionBounds)));
> > +    regions.mDispatchToContentHitRegion = regions.mHitRegion;
> 
> Might make sense to give EventRegions a two-argument constructor.

I considered that but in the future we're going to have more regions in there from touch-action and it might get confusing. Also this is a workaround that will come out eventually anyway so I preferred to leave the workaround messy than add more surface area to the API.

(In reply to Botond Ballo [:botond] from comment #17)
> aOutHitResult here and elsewhere

Done

(In reply to Botond Ballo [:botond] from comment #18)
> > +        result = mInputQueue->ReceiveInputEvent(
> > +            apzc, hitResult == ApzcHitRegion, inputForApzc, aOutInputBlockId);
> 
> My preference would be to write /* aTargetConfirmed = */ in these calls too,
> but I'll leave it up to you.

Didn't feel too strongly either way so I put it in.

> ::: gfx/layers/apz/src/InputQueue.cpp
> I'd phrase it as "we need to wait to give content the opportunity to
> prevent-default the touch events".

Done

> @@ +125,5 @@
> >  
> >  uint64_t
> >  InputQueue::InjectNewTouchBlock(AsyncPanZoomController* aTarget)
> >  {
> > +  TouchBlockState* block = StartNewTouchBlock(aTarget, true, true);
> 
> May want to start doing 
> 
>   /* argName = */ true
> 
> for readability.

Done.

> @@ +193,5 @@
> > +      success = mTouchBlockQueue[i]->TimeoutContentResponse()
> > +          | mTouchBlockQueue[i]->SetTargetApzc(mTouchBlockQueue[i]->GetTargetApzc());
> 
> ||

I split this into two statements as per comment 19.

> > +  void SetTargetApzc(uint64_t aInputBlockId, const nsRefPtr<AsyncPanZoomController>& aTargetApzc);
> 
> Might it make more sense to call this (and the method in InputBlockState)
> ConfirmTargetAPZC?

I ended up calling it SetConfirmedTargetApzc which I felt was a little better than either SetTargetApzc or ConfirmTargetApzc.

(In reply to Botond Ballo [:botond] from comment #19)
> >    INPQ_LOG("scheduling content response timeout for target %p\n", aTarget.get());
> 
> to be consistent, s/content response/main thread

> >    INPQ_LOG("got a content response timeout; block=%" PRIu64 "\n", aInputBlockId);
> 
> likewise
> 

Fixed both.

(In reply to Botond Ballo [:botond] from comment #22)
> OK, that's reasonable. We should probably ask that the caller passes in
> something specific, such as NULL_SCROLL_ID, to indicate that the touch hit
> non-scrollable content (and document this in the comment above the
> declaration of APZCTreeManager::SetTargetAPZC).

Updated the docs on APZCTreeManager::SetTargetAPZC to indicate this. I also updated my local WIP that calls this function to conform to this requirement.

> > For now I didn't bother doing anything with non-touch events. I was thinking
> > that we could construct input blocks out of PANGESTURE events as well and
> > deal with them in the same way as touch events but I haven't implemented
> > that since I was mostly focused on B2G.
> 
> Please add a TODO (and maybe file a bug) so we don't forget.

Filed bug 1098430 and added a TODO near the top of InputQueue::ReceiveInputEvent where it does touch-input check.

(In reply to Botond Ballo [:botond] from comment #21)
> It might be worth also having a test where an APZC parent layer has a
> non-APZC child layer, and the parent's hit region does not include the
> child's hit region; the test would check that tapping on the child's hit
> region delivers the event to the parent's APZC (i.e., it would test that the
> whole "accumulating the event regions in the subtree" code from Part 2 is
> working).

Sounds good, I'll see if I can write one.

Comment 25

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

Attachment: Part 0 - Update documentation to reflect changes in design (v2)

Comment 26

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

Attachment: Part 1 - Use EventRegions instead of nsIntRegions (v2)

All the v2 patches have addressed review comments and are rebased to latest inbound which includes bug 1055741.

Comment 27

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

Attachment: Part 2 - Use layer event regions if provided (v2)

Comment 28

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

Attachment: Part 3 - Small refactoring (v2)

Comment 29

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

Attachment: Part 4 - Workaround for bug 1082594 (v2)

Comment 30

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

Attachment: Part 5 - Distinguish hit test results more explicitly (v2)

Comment 31

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

Attachment: Part 6 - Require APZ target confirmation (v2)

Comment 32

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

Attachment: Part 7 - Basic gtests to exercise the event regions-based hit-testing code (v2)

I added a second test, r? for that.

Comment 33

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

And a try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=36750719c8e4

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8522382 [details] [diff] [review]
Part 7 - Basic gtests to exercise the event regions-based hit-testing code (v2)

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

Thanks!

Comment 35

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0f6773c89e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/91e14fb7253d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/99a62dc7395a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a733af37d7c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f85ae772445a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc7a52a6655
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73a6d613182a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c8df3f4ebd

Comment 36

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> > > +    if (AtBottomLayer()) {
> > > +      return mLayer->GetEventRegions();
> > 
> > I think this merits a comment: we attribute the event-regions to the
> > innermost (bottom) metrics, because we want touch events to go to its APZC
> > in preference to the ones above.
> 
> So I don't think that's really correct. It's simpler: we attribute the
> event-regions to the innermost layer because as per the contract of
> LayerMetricsWrapper, all the layers above it are empty container layers.
> Since they have no content, they must have empty event regions. The
> relationship to APZCs and touch-events is not relevant here.

Makes sense. Thanks for clarifying!

> > > +  if (!mEventRegions.IsEmpty()) {
> > > +    AppendToString(aStream, mEventRegions, " ", "");
> > 
> > Won't this look cryptic when both regions are empty (just a "{}" with no
> > label)?
> 
> I don't think so, there's a guard against that (see quoted code above). :)

Doh, missed that guard.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd0f6773c89e
https://hg.mozilla.org/mozilla-central/rev/91e14fb7253d
https://hg.mozilla.org/mozilla-central/rev/99a62dc7395a
https://hg.mozilla.org/mozilla-central/rev/3a733af37d7c
https://hg.mozilla.org/mozilla-central/rev/f85ae772445a
https://hg.mozilla.org/mozilla-central/rev/8bc7a52a6655
https://hg.mozilla.org/mozilla-central/rev/73a6d613182a
https://hg.mozilla.org/mozilla-central/rev/a9c8df3f4ebd

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

hey, graph server shows an improvement to linux dromaeo dom from this patch!

Comment 39

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

Not buying it. The dromaeo test has been the source of many false-positive emails in the past, and I think this is no different.

If you look at the data at http://graphs.mozilla.org/graph.html#tests=%5B%5B73,131,33%5D%5D&sel=1415775696844.4976,1415959132377.4924,785.9078590785907,991.869918699187&displayrange=7&datatype=running you'll see a sawtooth pattern which is very unlikely to be caused by actual tests. It's more likely that the servers this test was running on had a sudden spike in resource contention (for example) which gradually recovered. Twice.

Comment 40

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

If fact if you look at the Dromaeo(DOM) tests on Fx-team and b2g-inbound over the same time period [1] you will see a similar dip (the first "tooth" of the sawtooth).

[1] http://graphs.mozilla.org/graph.html#tests=%5B%5B73,131,33%5D,%5B73,132,33%5D,%5B73,203,33%5D%5D&sel=1415791799348.8057,1415878324994.6055,793.1598310396512,944.4065949039378&displayrange=7&datatype=running

Comment 41

[Joel Maher ( :jmaher ) (UTC -8)]

yeah, you are right, i suspect dromaeo dom is over sensitive to something which does this.  I file way too many bugs related to it.