Closed Bug 945203 Opened 6 years ago Closed 6 years ago

Annotate layer tree with hit regions for event handling

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: roc, Assigned: roc)

References

Details

(Whiteboard: [qa-])

Attachments

(8 files)

11.86 KB, patch
kats
: review+
Details | Diff | Splinter Review
961 bytes, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.55 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
20.63 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.37 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
20.96 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.60 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
26.07 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
No description provided.
Probably we should flip the ShouldBuildEventRegions flag to false for the initial landing.

I'm nervous about the overhead of doing this stuff. The nsDisplayEventRegions approach is a little shaky but probably good enough. There's some simple optimizations we can do to reduce overhead if it becomes a problem. Some overhead is inevitable.

I haven't done much testing. Some simple-but-tricky cases seem to work OK.

One way to use this data right away would be to start async panning immediately when we get a touchstart on a page that has touch event handlers but not where the user is pressing.

We probably need to do something to handle the new pointer events, probably something like the way we handle touch events here.
Comment on attachment 8347996 [details] [diff] [review]
Part 1: Add Layers API to annotate layers with hit-test regions

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

LGTM
Attachment #8347996 - Flags: review?(bugmail.mozilla) → review+
Attachment #8347998 - Flags: review?(jmuizelaar) → review+
Attachment #8347997 - Flags: review?(matt.woodrow) → review+
Attachment #8348000 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8348001 [details] [diff] [review]
Part 6: Move ThebesLayerData to the top level so it can be referenced elsewhere

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

::: layout/base/FrameLayerBuilder.cpp
@@ +607,5 @@
>     */
>    ThebesLayerData* FindThebesLayerFor(nsDisplayItem* aItem,
> +                                      const nsIntRect& aVisibleRect,
> +                                      const nsIFrame* aAnimatedGeometryRoot,
> +                                      const nsPoint& aTopLeft);

Did you mean to drop these two parameters?
Attachment #8348001 - Flags: review?(matt.woodrow) → review+
Attachment #8348002 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8348003 [details] [diff] [review]
Part 8: Add FrameLayerBuilder support for capturing nsDisplayEventRegions data into ThebesLayers

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1729,5 @@
> +  gfxRect transformed =
> +    aTransform.TransformBounds(gfxRect(bounds.x, bounds.y, bounds.width, bounds.height));
> +  transformed.RoundOut();
> +  nsIntRect intRect;
> +  if (!gfxUtils::GfxRectToIntRect(transformed, &intRect)) {

Comment that this should only fail if the rect is too big to fit in an int32.

@@ +1730,5 @@
> +    aTransform.TransformBounds(gfxRect(bounds.x, bounds.y, bounds.width, bounds.height));
> +  transformed.RoundOut();
> +  nsIntRect intRect;
> +  if (!gfxUtils::GfxRectToIntRect(transformed, &intRect)) {
> +    *aDest = nsIntRect(-(1 << 30), -(1 << 30), 1 << 31, 1 << 31);

Don't we have defines to use for these?
Attachment #8348003 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Comment on attachment 8348001 [details] [diff] [review]
> Part 6: Move ThebesLayerData to the top level so it can be referenced
> elsewhere
> 
> Review of attachment 8348001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +607,5 @@
> >     */
> >    ThebesLayerData* FindThebesLayerFor(nsDisplayItem* aItem,
> > +                                      const nsIntRect& aVisibleRect,
> > +                                      const nsIFrame* aAnimatedGeometryRoot,
> > +                                      const nsPoint& aTopLeft);
> 
> Did you mean to drop these two parameters?

I guess you did, as part of part 8.
Comment on attachment 8348003 [details] [diff] [review]
Part 8: Add FrameLayerBuilder support for capturing nsDisplayEventRegions data into ThebesLayers

>+GetTransformToRoot(Layer* aLayer)
>...
>+    transform = transform*l->GetTransform();

spaces around the '*' please (or alternatively *=)
Comment on attachment 8347999 [details] [diff] [review]
Part 4: Add nsDisplayEventRegions and build it when painting

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Probably we should flip the ShouldBuildEventRegions flag to false for the
> initial landing.

Yes, please do that.  I think it would be better to have it off initially
to be able to see the true perf impact of the final code in case there are
additional changes before we start using this (which might be hard to see
if divided over several checkins).  And also to verify that the off-switch
is working as intended.

>+class nsDisplayLayerEventRegions : public nsDisplayItem {

MOZ_FINAL?
Attachment #8347999 - Flags: review?(matspal) → review+
Out of curiosity, how hard would it be to uplift this to the 28 branch?
Flags: needinfo?(roc)
Off the top of my head, not very hard.
Flags: needinfo?(roc)
Btw on a recent master B2G build I turned on layer tree logging and it looks like the hitregion and dispatchtocontentregion are pretty much always empty. The only time I see it non-empty is on the B2G lockscreen, and there both regions are 1x1 in size. I haven't looked into it more yet but if there's anything obvious I should check or that I need to enable please let me know.
Turns out the code needed to be enabled by uncommenting the stuff in IsBuildingLayerEventRegions()
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.