Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Description • 11 years ago

      No description provided.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 11 years ago

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.

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

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

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

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?

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

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?

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

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

	Mats Palmgren (inactive)
	Comment 14 • 11 years ago

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 *=)

	Mats Palmgren (inactive)
	Comment 15 • 11 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 11 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 17 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/58ae7af68da1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1fe524aba1
https://hg.mozilla.org/integration/mozilla-inbound/rev/772d415e8ca0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e20a8ceebb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcfe3e28c2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7a7130adf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/265f3a2040b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/722d9c9944c1

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

https://hg.mozilla.org/mozilla-central/rev/58ae7af68da1
https://hg.mozilla.org/mozilla-central/rev/7e1fe524aba1
https://hg.mozilla.org/mozilla-central/rev/772d415e8ca0
https://hg.mozilla.org/mozilla-central/rev/6e20a8ceebb7
https://hg.mozilla.org/mozilla-central/rev/0dcfe3e28c2e
https://hg.mozilla.org/mozilla-central/rev/3e7a7130adf2
https://hg.mozilla.org/mozilla-central/rev/265f3a2040b4
https://hg.mozilla.org/mozilla-central/rev/722d9c9944c1

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

Out of curiosity, how hard would it be to uplift this to the 28 branch?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 11 years ago

Off the top of my head, not very hard.

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

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.

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

Turns out the code needed to be enabled by uncommenting the stuff in IsBuildingLayerEventRegions()