Closed Bug 866232 Opened 7 years ago Closed 6 years ago

Support having multiple APZC instances attached to the layer tree

Categories

(Core :: Graphics: Layers, defect)

23 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(15 files, 31 obsolete files)

1.81 KB, patch
kats
: review+
Details | Diff | Splinter Review
9.24 KB, patch
kats
: review+
Details | Diff | Splinter Review
11.83 KB, patch
kats
: review+
Details | Diff | Splinter Review
13.40 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
53.08 KB, patch
BenWa
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
28.24 KB, patch
BenWa
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
8.61 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
15.39 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
25.33 KB, patch
BenWa
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
22.28 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
10.90 KB, patch
BenWa
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
9.96 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
19.32 KB, patch
BenWa
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
7.04 KB, patch
BenWa
: review+
bbondy
: review+
Details | Diff | Splinter Review
2.20 KB, patch
vingtetun
: review+
Details | Diff | Splinter Review
For the multi-apzc bug we will need to support having multiple instances of APZC, such that there is one attached to each layer that we are asynchronously controlling. I believe that currently trying to have more than one APZC will not work and some work will be needed to make this work.
Looking around the code, here are some things that will need to be fixed:

- The code at http://hg.mozilla.org/mozilla-central/file/23ce4eab8fb1/dom/browser-element/BrowserElementPanning.js#l143 and http://hg.mozilla.org/mozilla-central/file/23ce4eab8fb1/dom/browser-element/BrowserElementPanning.js#l267 will need to be ripped out and folded into the APZC hit-detection code. This effectively means killing off the public interface at http://hg.mozilla.org/mozilla-central/file/23ce4eab8fb1/layout/ipc/RenderFrameChild.h

- The GeckoContentController interface (used for callbacks from APZC) will need to now support being called back by multiple APZCs. The functions on this interface will need some way to identify which APZC is doing the callback. I think passing the mPresShellId and mScrollId from the APZC's FrameMetrics should do the trick. They should uniquely identify the layer that is being operated on without introducing cross-thread or cross-process object access, and can be mapped to the relevant scroll frame on the layout side of things.
Assignee: nobody → bugmail.mozilla
In bug 879475 I'm going to allow us to run RenderFrameParent::RenderFrameParent in a child process.  I'm planning to add a IPDL message to allow a content process to ask the main process to reserve a layer tree id for the renderframe, but I'm not sure what to do about the APZC.  I think it will conflict with this bug.
I think we might be ok since we aren't going to modify the layer tree. The only thing to keep in mind is that scrollid are only unique within a content process.
Not sure who is the more appropriate reviewer here.
Attachment #772159 - Attachment is obsolete: true
Attachment #772826 - Flags: review?(bgirard)
Attachment #772826 - Flags: review?(ajones)
This is approaching the code in my WIPs on bug 890280 from the other side so that the patch is cleaner. (i.e. I'm creating the APZCTreeManager and then I will flesh it out, rather than adding a mountain of code to CompositorParent and then extracting it).
Attachment #772827 - Flags: review?(bgirard)
Attachment #772827 - Flags: review?(ajones)
Comment on attachment 772826 [details] [diff] [review]
Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

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

::: gfx/layers/ipc/GeckoContentController.h
@@ +25,5 @@
> + */
> +struct ScrollableLayerGuid {
> +  uint64_t mLayersId;
> +  uint32_t mPresShellId;
> +  FrameMetrics::ViewID mScrollId;

Do we get a new layer ID when we go to a new page? If so then perhaps we could get rid of mPressShellId. I also don't yet understand why we need both mLayersId and mScrollId.
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #8)
> Do we get a new layer ID when we go to a new page? If so then perhaps we
> could get rid of mPressShellId. I also don't yet understand why we need both
> mLayersId and mScrollId.

No, for a given tab the layer ID is always the same. The layer ID is different for different tabs. So the layer ID is basically identifying the tab. The presShellId is tab-specific, we get a new one of those every time a tab loads a new page. Different tabs may have pages with the same presShellId though. The mScrollId identifies the scrollable frame within the page. In theory we might be able to do away with presShellId and just use the scrollId but then there's special scrollIds like ROOT_SCROLL_ID and such that we'd have to disambiguate. I felt it was better to just keep all three.
Attachment #772826 - Flags: review?(ajones) → review+
That being said, if the comment I put above the struct definition is unclear let me know so I can fix it. I want that documentation to be able to stand on its own.
Comment on attachment 772826 [details] [diff] [review]
Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +667,5 @@
>      CSSPoint point = WidgetSpaceToCompensatedViewportSpace(
>        ScreenPoint::FromUnknownPoint(gfx::Point(
>          aEvent.mPoint.x, aEvent.mPoint.y)),
>        resolution);
> +    mGeckoContentController->HandleLongTap(ScrollableLayerGuid(

I don't think we should store this information in the APZC and pass it to the content controller but rather just store this information in the content controller. APZC shouldn't need to bother about these details. Let's keep it separate. Ditto for the changes below.

Looking further in the patch looks like it invalidate most of the patch. Let me know if you disagree with this.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +68,5 @@
>     * touches moving the screen.
>     */
>    static float GetTouchStartTolerance();
>  
> +  AsyncPanZoomController(uint64_t aLayersId,

The layer ID is not always relevant like in OMTC Fennec. I wonder if we should make this optional.
Attachment #772826 - Flags: review?(bgirard) → review-
Comment on attachment 772827 [details] [diff] [review]
Part 2 - Add an APZCTreeManager class to encapsulate the APZC tree

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

Minor stuff, would like to see more defensive asserts.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +70,5 @@
>            }
>          } else {
> +          // XXX this function doesn't properly detach nested referent
> +          // layers, since detaching the layer here will prevent walking
> +          // through the subtree rooted at this point.

I don't understand this. If we detach a parent then we usually want to detach the whole substree?

::: gfx/layers/ipc/APZCTreeManager.cpp
@@ +14,5 @@
> +    : mLayersId(aLayersId)
> +    , mTreeLock("APZCTreeLock")
> +    , mController(aController)
> +    , mGestureBehavior(aGestures)
> +{

Let's add MOZ_COUNT_CTOR/DTOR. This class will be involved in some reference cycle so if we leak it will be easier to debug.

@@ +20,5 @@
> +
> +void
> +APZCTreeManager::SetCompositorParent(CompositorParent* aParent)
> +{
> +  mCompositorParent = aParent;

IMO I wouldn't keep a reference to the compositor but rather just pass it in UpdateAPZCTree since it's only required there.

@@ +26,5 @@
> +
> +void
> +APZCTreeManager::UpdatePanZoomControllerTree(Layer* aRoot, bool aIsFirstPaint)
> +{
> +  if (aRoot && aRoot->AsContainerLayer()) {

Assert that we're on the compositor thread. Maybe we should also assert that if aRoot != nullptr then aRoot is a container layer?

::: gfx/layers/ipc/APZCTreeManager.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +class APZCTreeManager MOZ_FINAL {

Class comment would be nice. Great method documentation.

@@ +44,5 @@
> +   * basde on what type of input it is. For example, a PinchGestureEvent will
> +   * cause scaling. This should only be called externally to this class.
> +   * HandleInputEvent() should be used internally.
> +   */
> +  nsEventStatus ReceiveInputEvent(const InputData& aEvent);

Should we require this to be called only from the UI thread?

@@ +54,5 @@
> +   * touches can be passed through the DOM and content can handle them.
> +   *
> +   * NOTE: Be careful of invoking the nsInputEvent variant. This can only be
> +   * called on the main thread. See widget/InputData.h for more information on
> +   * why we have InputData and nsInputEvent separated.

Assert for the main thread

::: widget/android/nsWindow.cpp
@@ +2501,5 @@
>                              int aSurfaceWidth, int aSurfaceHeight)
>          : CompositorParent(aWidget, aRenderToEGLSurface, aSurfaceHeight, aSurfaceHeight)
>      {
> +        if (nsWindow::GetAPZCTreeManager()) {
> +            nsWindow::GetAPZCTreeManager()->SetCompositorParent(this);

This change the behavior. We're no longer calling SetCompositorParent on APZC. Is this intentional?

@@ +2541,2 @@
>  {
> +    return sApzcTree;

This is a bit scary because it's called from 3 different thread and it's not clear from the patch diff that there's no race.
Attachment #772827 - Flags: review?(bgirard) → review-
(In reply to Benoit Girard (:BenWa) from comment #12)
> Comment on attachment 772827 [details] [diff] [review]
> Part 2 - Add an APZCTreeManager class to encapsulate the APZC tree
> 
> Review of attachment 772827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minor stuff, would like to see more defensive asserts.
> 
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +70,5 @@
> >            }
> >          } else {
> > +          // XXX this function doesn't properly detach nested referent
> > +          // layers, since detaching the layer here will prevent walking
> > +          // through the subtree rooted at this point.
> 
> I don't understand this. If we detach a parent then we usually want to
> detach the whole substree?
> 

Just ignore this.  We currently don't create nested ref layers and I already have a patch fixing this.
Slightly modified from the original to reserve a layer id for the root layer. As per discussion with BenWa I'm keeping the ScrollableLayerGuid as-is.
Attachment #772826 - Attachment is obsolete: true
Updated so that now there is a single APZCTreeManager instance, owned by the CompositorParent. The next patch will do the actual tree building which should clean up some of the loose ends in this patch.
Attachment #772827 - Attachment is obsolete: true
Attachment #772827 - Flags: review?(ajones)
This makes future patches simpler since there's less stuff to pass around.
Attachment #774201 - Attachment is obsolete: true
Attachment #775725 - Flags: review?(bgirard)
Not ready for review just yet but posting it as a WIP
Attachment #774203 - Attachment is obsolete: true
Here I add an std::map to the APZCTreeManager as a temporary thing to make this patch roughly standalone. It is removed in the next patch, once the tree-building code is put into place. I need a map here because there are still multiple APZC instances (one per RenderFrameParent) and only a single APZCTreeManager.
Attachment #775728 - Attachment is obsolete: true
I need to fix up the test code here, and generally add more tests for the tree building and tree management stuff in APZCTreeManager. I also need some lifetime tests to make sure the APZC instances are created and destroyed when they're supposed to be.
Comment on attachment 775724 [details] [diff] [review]
Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

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

Reluctant r+ for exposing details that the APZC doesn't need.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +487,5 @@
>     * instead use: |mState = NEWSTATE;|
>     */
>    void SetState(PanZoomState aState);
>  
> +  uint64_t mLayersId;

:(

::: gfx/layers/ipc/CompositorParent.cpp
@@ +683,5 @@
>  CompositorParent::AllocateLayerTreeId()
>  {
>    MOZ_ASSERT(CompositorLoop());
>    MOZ_ASSERT(NS_IsMainThread());
> +  static uint64_t ids = ROOT_LAYER_TREE_ID;

Are you reserving 0 and 1 on purpose? Initializing this to 0 lets us use the .bss and with pre-increment gives us a starting ID of 1.

::: gfx/layers/ipc/GeckoContentController.h
@@ +38,5 @@
> +
> +  ScrollableLayerGuid(uint64_t aLayersId, const FrameMetrics& aMetrics)
> +    : mLayersId(aLayersId)
> +    , mPresShellId(aMetrics.mPresShellId)
> +    , mScrollId(aMetrics.mScrollId)

optional: IMO This seems like the type of object that should have operator==/operator!=
Attachment #775724 - Flags: review?(bgirard) → review+
Comment on attachment 775725 [details] [diff] [review]
Part 2 - Flip APZC default behaviour

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

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ -69,5 @@
>    static float GetTouchStartTolerance();
>  
>    AsyncPanZoomController(uint64_t aLayersId,
>                           GeckoContentController* aController,
> -                         GestureBehavior aGestures = DEFAULT_GESTURES);

I don't really like that DEFAULT_GUESTURES is no longer the default. Should it maybe be renamed to CUSTOM_GUESTURES or something similar?
Attachment #775725 - Flags: review?(bgirard) → review+
Comment on attachment 775727 [details] [diff] [review]
Part 3 - Allow APZC instances to be created on the compositor thread

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ -137,2 @@
>  
> -class ReadAZPCPref MOZ_FINAL : public nsRunnable {

Yay. Thanks for removing this.

@@ +189,3 @@
>    MOZ_COUNT_CTOR(AsyncPanZoomController);
>  
> +  InitializeGlobalState();

This means we can only create APZC from the main thread. Is this really a limitation we want?
Attachment #775727 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #27)
> Reluctant r+ for exposing details that the APZC doesn't need.
> 

I think I'm starting to agree with you that this might not be needed in APZC. I think I will be able to take it out in a later patch.

> >    MOZ_ASSERT(NS_IsMainThread());
> > +  static uint64_t ids = ROOT_LAYER_TREE_ID;
> 
> Are you reserving 0 and 1 on purpose? Initializing this to 0 lets us use the
> .bss and with pre-increment gives us a starting ID of 1.

Sort of, yeah. I generally like starting counters at 1 so that if they ever have a zero value you know something went wrong somewhere or was not initialized properly. That's why I started ROOT_LAYER_TREE_ID at 1 and all other tree IDs are higher than that. I can change it so that ROOT_LAYER_TREE_ID is zero if you prefer but I don't know if the .bss optimization is worth it.

> optional: IMO This seems like the type of object that should have
> operator==/operator!=

Good point; as it turned out I needed that later for testing so I updated this patch to include that.

(In reply to Benoit Girard (:BenWa) from comment #28)
> I don't really like that DEFAULT_GUESTURES is no longer the default. Should
> it maybe be renamed to CUSTOM_GUESTURES or something similar?

Yup, will rename them.

(In reply to Benoit Girard (:BenWa) from comment #29)
> @@ +189,3 @@
> >    MOZ_COUNT_CTOR(AsyncPanZoomController);
> >  
> > +  InitializeGlobalState();
> 
> This means we can only create APZC from the main thread. Is this really a
> limitation we want?

That limitation was already there. The purpose of this patch is to move all the stuff that requires main-thread to a single function, and then in a future patch I will move the call to InitializeGlobalState() out of the APZC constructor so that APZC can be created on the compositor thread.

New patch series coming shortly.
Updated with operator== and operator!= overloaded on ScrollableLayerGuid, carrying r+.
Attachment #775724 - Attachment is obsolete: true
Attachment #778068 - Flags: review+
Updated to rename the enum fields, and also updated TestAsyncPanZoomController to specify NO_GESTURE_DETECTOR. There's a couple of problems with using USE_GESTURE_DETECTOR in the test code:

1) There is a ref cycle between APZC <-> GestureEventListener that isn't broken unless you call Destroy() on the APZC, which the tests currently don't do.

2) Some of the tests (e.g. Pan) will trigger a call to PostDelayedTask when there is a GestureEventListener in use, and we'd have to update the test to deal with that.

To avoid these problems I figure it was best to just keep the tests using NO_GESTURE_DETECTOR.
Attachment #775725 - Attachment is obsolete: true
Attachment #778069 - Flags: review?(bgirard)
This is a workaround for that problem I was talking to you about. Basically, when we move the APZC creation from RenderFrameParent (which runs on the main thread) to the shadow layers update (which runs on the compositor thread), there are some messages directed to the APZC which are dropped because they are sent in between. Of those messages there is only one that actually matters, and that one triggers a repaint of the area owned by the APZC. So this change puts that behaviour back in so that it happens on the first call to NotifyLayersUpdate. I admit this patch is a bit hacky but it was the best option I could come up with. I might need to revisit this later.
Attachment #778078 - Flags: review?(bgirard)
Would like feedback from Matt too since Anthony Jones is on PTO right now and I feel like some of these patches could use extra eyes.
Attachment #776551 - Attachment is obsolete: true
Attachment #778080 - Flags: review?(bgirard)
Attachment #778080 - Flags: feedback?(matt.woodrow)
Attachment #776552 - Attachment is obsolete: true
Attachment #778081 - Flags: review?(bgirard)
Attachment #778081 - Flags: feedback?(matt.woodrow)
The test that is commented out is updated in the next patch. Also, if there were a region class that used floats I would totally use it here. But I'm too lazy to write one so I'm just using an array of rects. Damn you nsRegion!
Attachment #776558 - Attachment is obsolete: true
Attachment #778086 - Flags: review?(bgirard)
Attachment #778086 - Flags: feedback?(matt.woodrow)
I left the "expected" hit points in as comments because they're actually not correct (see bug 890280). I need to figure out how to arrange some of the code so that only the APZC transform is unapplied to the incoming input events, and the right coordinates are passed out to the GeckoContentController. Once I do that I can update this test so that it just simulates tap events at the right places and checks that the MockContentController gets the right values in its tap callback.
Attachment #778090 - Flags: review?(bgirard)
Attachment #778090 - Flags: feedback?(matt.woodrow)
Comment on attachment 778080 [details] [diff] [review]
Part 6 - Add an APZCTreeManager to encapsulate the tree of APZC instances

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

Looks good, just a few comments that may or may not be useful.

::: gfx/layers/composite/APZCTreeManager.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +class APZCTreeManager MOZ_FINAL {

I think I'd prefer if this class was called AsyncPanZoomController, and rename the old one to AsyncLayerScoller or something.

'Controller' implies that it manages all the things, in my mind at least.

Some general comments about the purpose of this class would be good here.

@@ +57,5 @@
> +   * general, this will just be:
> +   * { x = 0, y = 0, width = surface.width, height = surface.height }, however
> +   * there is no hard requirement for this.
> +   */
> +  void UpdateCompositionBounds(const ScrollableLayerGuid& aGuid,

This is called from the main thread, right?

It would be nice to separate the functions by which thread they are called from, and document it (and maybe assert it too).

::: gfx/layers/ipc/CompositorParent.cpp
@@ +152,5 @@
>    CompositorLoop()->PostTask(FROM_HERE, NewRunnableFunction(&AddCompositor,
>                                                            this, &mCompositorID));
>  
>    if (!sCurrentCompositor) {
> +    mApzcTreeManager = new APZCTreeManager();

I just got rid of sCurrentCompositor on trunk

@@ +746,5 @@
> +  if (sCurrentCompositor) {
> +    return sCurrentCompositor->mApzcTreeManager.get();
> +  }
> +  return nullptr;
> +}

This won't be able to be static without sCurrentCompositor.
Attachment #778080 - Flags: feedback?(matt.woodrow) → feedback+
Comment on attachment 778081 [details] [diff] [review]
Part 7 - Replace the placeholder std::map with an actual tree of APZC instances

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +32,5 @@
>  {
>    MonitorAutoLock lock(mTreeLock);
> +
> +  nsTArray< nsRefPtr<AsyncPanZoomController> > apzcsToDestroy;
> +  Collect(mRootApzc, &apzcsToDestroy);

What's the reason for collecting a 'to destroy' array upfront and then removing items from it?

Can't we just destroy ones directly as we find they aren't needed within UpsatePanZoomControllerTree?

It seems like no changes would be the common case.

Add a comment explaining why we do this at least.

@@ +245,5 @@
>  
> +  nsTArray< nsRefPtr<AsyncPanZoomController> > apzcsToDestroy;
> +  Collect(mRootApzc, &apzcsToDestroy);
> +  for (int i = apzcsToDestroy.Length() - 1; i >= 0; i--) {
> +    apzcsToDestroy[i]->Destroy();

Same as above, isn't this walking everything twice? Can't we just call destroy while we walk the tree, or do you need this for specific ordering?

@@ +279,5 @@
> +  }
> +  AsyncPanZoomController* ret = FindTargetAPZC(aApzc->GetLastChild(), aGuid);
> +  if (!ret) {
> +    ret = FindTargetAPZC(aApzc->GetPrevSibling(), aGuid);
> +  }

Comment that this is walking the tree depth first in reverse order (front to back on the screen).
Attachment #778081 - Flags: feedback?(matt.woodrow) → feedback+
Comment on attachment 778081 [details] [diff] [review]
Part 7 - Replace the placeholder std::map with an actual tree of APZC instances

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +280,5 @@
> +  AsyncPanZoomController* ret = FindTargetAPZC(aApzc->GetLastChild(), aGuid);
> +  if (!ret) {
> +    ret = FindTargetAPZC(aApzc->GetPrevSibling(), aGuid);
> +  }
> +  return ret;

We also seem to have this code for walking the tree in hit-test order twice already, and I assume there will be a third in the currently-unimplemented GetTargetAPZC.

It would be nice if we could share this somehow.
Comment on attachment 778082 [details] [diff] [review]
Part 8 - Some cleanup in APZC, no functional changes

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

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +178,5 @@
>     * Return value indicates whether or not any currently running animation
>     * should continue. That is, if true, the compositor should schedule another
>     * composite.
>     */
>    bool SampleContentTransformForFrame(const TimeStamp& aSampleTime,

Drive-by comment.

Each APZC only handles a single scrollable frame doesn't it? So we shouldn't need the aLayer parameter?
Comment on attachment 778083 [details] [diff] [review]
Part 9 - Extract a GetCurrentAsyncTransform method in APZC

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

Ignore my previous comment :)
Comment on attachment 778086 [details] [diff] [review]
Part 10 - Add visible region tracking and hit-testing code in APZCTreeManager

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +319,5 @@
> +AsyncPanZoomController*
> +APZCTreeManager::GetAPZCAtPoint(AsyncPanZoomController* aApzc, gfxPoint aHitTestPoint)
> +{
> +  ViewTransform apzcTransform = aApzc->GetCurrentAsyncTransform();
> +  gfxPoint untransformed = gfx3DMatrix(apzcTransform).Inverse().Transform(aHitTestPoint);

Use Invert().ProjectPoint() here, since it handles recovering the depth/perspective components if the transform is 3d.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#4097

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +652,5 @@
> +    return false;
> +  }
> +
> +private:
> +  nsTArray<gfxRect> mVisibleRegion;

Is this because we don't have a proper region class for gfxRects?

The real region classes have the property that rects are guaranteed to not overlap, and are sorted. Makes the Contains() function significantly cheaper. We still tend to have frequent performance issues with them.

A TODO about fixing this would be useful.
Attachment #778086 - Flags: feedback?(matt.woodrow) → feedback+
Comment on attachment 778090 [details] [diff] [review]
Part 11 - Put back the test I commented out and rebase it

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

::: gfx/layers/ipc/CompositorParent.h
@@ +37,5 @@
>  class LayerManagerComposite;
>  class AsyncCompositionManager;
>  struct TextureFactoryIdentifier;
>  
> +struct LayerTreeAutoRegistration

Do we need this outside of the test?

Might be better to define it there
Attachment #778090 - Flags: feedback?(matt.woodrow) → feedback+
(In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> I think I'd prefer if this class was called AsyncPanZoomController, and
> rename the old one to AsyncLayerScoller or something.
> 
> 'Controller' implies that it manages all the things, in my mind at least.

I don't mind doing this but would like to defer this until later. Changing names now will make it confusing to talk about this code.

> Some general comments about the purpose of this class would be good here.

Ok, will add.

> @@ +57,5 @@
> This is called from the main thread, right?
> 
> It would be nice to separate the functions by which thread they are called
> from, and document it (and maybe assert it too).

I can assert main-thread calls but AFAIK there is no method to assert that something is running on the compositor thread. I can add one, but just want to confirm that it doesn't exist someplace I didn't see it.

> 
> I just got rid of sCurrentCompositor on trunk

Yeah.. that's gonna be a bit painful but I'll update to take this into account. There will be one APZCTreeManager per CompositorParent.

(In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> >    MonitorAutoLock lock(mTreeLock);
> > +
> > +  nsTArray< nsRefPtr<AsyncPanZoomController> > apzcsToDestroy;
> > +  Collect(mRootApzc, &apzcsToDestroy);
> 
> What's the reason for collecting a 'to destroy' array upfront and then
> removing items from it?
> 
> Can't we just destroy ones directly as we find they aren't needed within
> UpsatePanZoomControllerTree?
> 
> It seems like no changes would be the common case.
> 
> Add a comment explaining why we do this at least.

The main reason I did this was because I couldn't figure out a way to destroy the unneeded APZC instances during the tree walk. There are two scenarios: (a) a layer with an APZC is removed from the layer tree and (b) a layer with an APZC is moved in the layer tree from one place to a completely different place. In scenario (a) we would want to destroy the APZC while walking the layer tree and noticing that the layer/APZC is no longer there. But if we do that then we run into a problem in scenario (b) because we might encounter that layer later during the walk. To handle both of these we have to 'remember' that the layer was not found, and then do the destroy only at the end of the tree walk after we are sure that the layer was removed and not just transplanted elsewhere. Doing that as part of a recursive tree walk seemed unnecessarily complicated so I pulled it out with the collection step.

If that reasoning seems flawed let me know; otherwise I will add a comment to that effect to explain why I wrote it that way.

> > +  nsTArray< nsRefPtr<AsyncPanZoomController> > apzcsToDestroy;
> > +  Collect(mRootApzc, &apzcsToDestroy);
> > +  for (int i = apzcsToDestroy.Length() - 1; i >= 0; i--) {
> > +    apzcsToDestroy[i]->Destroy();
> 
> Same as above, isn't this walking everything twice? Can't we just call
> destroy while we walk the tree, or do you need this for specific ordering?

Yeah I can do this with a tree walk. It just seemed easier to re-use the Collect method since I already wrote it. Specific ordering is not needed, so I can change this.

> @@ +279,5 @@
> > +  }
> > +  AsyncPanZoomController* ret = FindTargetAPZC(aApzc->GetLastChild(), aGuid);
> > +  if (!ret) {
> > +    ret = FindTargetAPZC(aApzc->GetPrevSibling(), aGuid);
> > +  }
> 
> Comment that this is walking the tree depth first in reverse order (front to
> back on the screen).

Will do.

(In reply to Matt Woodrow (:mattwoodrow) from comment #45)
> We also seem to have this code for walking the tree in hit-test order twice
> already, and I assume there will be a third in the currently-unimplemented
> GetTargetAPZC.
> 
> It would be nice if we could share this somehow.

I'll think about some way to share it. I'd rather avoid using macros or templates (a la WalkTheTree<OP>) if possible although that might end up being the cleanest way.

(In reply to Matt Woodrow (:mattwoodrow) from comment #48)
> > +  ViewTransform apzcTransform = aApzc->GetCurrentAsyncTransform();
> > +  gfxPoint untransformed = gfx3DMatrix(apzcTransform).Inverse().Transform(aHitTestPoint);
> 
> Use Invert().ProjectPoint() here, since it handles recovering the
> depth/perspective components if the transform is 3d.

Will do, thanks.

> > +
> > +private:
> > +  nsTArray<gfxRect> mVisibleRegion;
> 
> Is this because we don't have a proper region class for gfxRects?

Yes.

> The real region classes have the property that rects are guaranteed to not
> overlap, and are sorted. Makes the Contains() function significantly
> cheaper. We still tend to have frequent performance issues with them.
> 
> A TODO about fixing this would be useful.

Ok, I'll add a TODO.

(In reply to Matt Woodrow (:mattwoodrow) from comment #49)
> > +struct LayerTreeAutoRegistration
> 
> Do we need this outside of the test?
> 
> Might be better to define it there

We don't use it outside the test, but the implementation of this needs to access sIndirectLayerTrees which is static inside CompositorParent.cpp, so that's where I ended up putting this.
Attachment #778069 - Flags: review?(bgirard) → review+
Attachment #778072 - Flags: review?(bgirard) → review+
Comment on attachment 778078 [details] [diff] [review]
Part 5 - Add support for delaying APZC creation until the layer update

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1213,5 @@
>  
>      mX.CancelTouch();
>      mY.CancelTouch();
>  
> +    needContentRepaint |= (mFrameMetrics.IsDefault() && !aViewportFrame.IsDefault());

Reading this line makes me wonder why this isn't just needContentRepaint |= (mFrameMetrics == aViewportFrame). I think this should have a comment to explain this logic.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +249,5 @@
>    /**
> +   * This method is for testing purposes only. It sets the mFrameMetrics field
> +   * on the APZC.
> +   */
> +  void SetFrameMetricsForTest(const FrameMetrics& aMetrics);

I'm not a fan of shipping code we only use for testing. You can make this virtual and just overwrite it in a TestAsyncPanZoomController. I don't know if this will actually provide a saving because we will need a vtable and this method is small so feel free to ignore this.
Attachment #778078 - Flags: review?(bgirard) → review+
Comment on attachment 778080 [details] [diff] [review]
Part 6 - Add an APZCTreeManager to encapsulate the tree of APZC instances

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +48,5 @@
> +      controller->NotifyLayersUpdated(container->GetFrameMetrics(), aIsFirstPaint);
> +    }
> +  }
> +
> +  if (controller) {

you're performing add/erase even if the element is in the map which requires a lookup. I think this should be moved into the above if and done only when required.

::: gfx/layers/composite/APZCTreeManager.h
@@ +14,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +class APZCTreeManager MOZ_FINAL {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(APZCTreeManager)

Should we be adding unit tests for this class as well?
Comment on attachment 778080 [details] [diff] [review]
Part 6 - Add an APZCTreeManager to encapsulate the tree of APZC instances

Probably not worth reviewing this revision any more; I am working on some updates and will upload a new patch queue soonish.
Attachment #778080 - Flags: review?(bgirard)
This is a small piece of the patch it's obsoleting. Half of the rest is merged into a later patch and the other half is not needed. Carrying r+
Attachment #778068 - Attachment is obsolete: true
Attachment #781462 - Flags: review+
Comment on attachment 778069 [details] [diff] [review]
Part 2 - Flip APZC default behaviour

This patch is not needed any more.
Attachment #778069 - Attachment is obsolete: true
Rebased, updated to address review comments. I moved SetFrameMetricsForTest out of APZC and into a TestAPZC class, so re-requesting review for that churn.
Attachment #778078 - Attachment is obsolete: true
Attachment #781467 - Flags: review?(bgirard)
Attachment #778090 - Attachment is obsolete: true
Attachment #781473 - Flags: review?(bgirard)
Small compilation fix for android.
Attachment #781476 - Attachment is obsolete: true
Attachment #781476 - Flags: review?(bgirard)
Attachment #781719 - Flags: review?(bgirard)
Blocks: 895905
Comment on attachment 781467 [details] [diff] [review]
Part 4 - Add support for delaying APZC creation until the layer update

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

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +522,5 @@
>    // monitor. Do not read from or modify either of them without locking.
>    FrameMetrics mFrameMetrics;
> +
> +  // Protects |mFrameMetrics|, |mLastContentPaintMetrics|, |mState| and
> +  // |mMetaViewportInfo|. Before manipulating |mFrameMetrics| or

There is no longer a field named mMetaViewportInfo in APZC.
I screwed up in part 1 - I made the assumption that there is a global root layer, when in fact there is one per CompositorParent. Since I don't want to fix up part one and rebase everything all over again I'm tacking on this extra patch at the end to clean up the mess. This is required in order to deal with the metro code that bbondy landed recently.
Attachment #782151 - Flags: review?(matt.woodrow)
Attachment #782151 - Flags: review?(bgirard)
Try build in progress with this patch at https://tbpl.mozilla.org/?tree=Try&rev=64ce5c2c5226
Attachment #782152 - Flags: review?(netzen)
Attachment #782152 - Flags: review?(bgirard)
Comment on attachment 782152 [details] [diff] [review]
Part 15 - Update metro code to use APZCTreeManager

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

::: browser/metro/profile/metro.js
@@ +24,5 @@
>  pref("metro.debug.selection.dumpEvents", false);
>  
>  // Enable off main thread compositing
>  pref("layers.offmainthreadcomposition.enabled", true);
> +pref("layers.async-pan-zoom.enabled", true);

I don't think we want to turn this on until bug 886321's dependencies are mostly resolved.  Please do test with this on an off though when pushing to try.
Attachment #782152 - Flags: review?(netzen)
By the way I think there is also a bunch of other code not included in this patch that maybe you meant to include, you sent me a diff on irc on Friday.  I wasn't sure if that was intentionally left out of that patch and maybe included in a different patch or not (I didn't check the other patches).
Small fix to address the metro orange in the above try build. With this modified patch it is green: https://tbpl.mozilla.org/?tree=Try&rev=e9932d576f6a

I also pushed the full set rebased on latest m-c to try again: https://tbpl.mozilla.org/?tree=Try&rev=b4e3a86d98a6
Attachment #782151 - Attachment is obsolete: true
Attachment #782151 - Flags: review?(matt.woodrow)
Attachment #782151 - Flags: review?(bgirard)
Attachment #782248 - Flags: review?(matt.woodrow)
Attachment #782248 - Flags: review?(bgirard)
Oops, I uploaded the wrong patch. The pref-enabling was just for try purposes. This is the actual patch to the metro code.
Attachment #782152 - Attachment is obsolete: true
Attachment #782152 - Flags: review?(bgirard)
Attachment #782250 - Flags: review?(netzen)
Attachment #782250 - Flags: review?(bgirard)
Comment on attachment 781469 [details] [diff] [review]
Part 6 - Replace the placeholder std::map with an actual tree of APZC instances

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +98,5 @@
> +          controller->SetLastChild(nullptr);
> +        }
> +
> +        controller->NotifyLayersUpdated(container->GetFrameMetrics(),
> +                                        (aLayersId == aFirstPaintLayersId ? aIsFirstPaint : false));

Did you mean aLayersId == aFirstPaintLayersId && aIsFirstPaint?
Comment on attachment 781472 [details] [diff] [review]
Part 9 - Add visible region tracking and hit testing code to APZCTreeManager

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +354,5 @@
>    return nullptr;
>  }
>  
> +AsyncPanZoomController*
> +APZCTreeManager::GetAPZCAtPoint(AsyncPanZoomController* aApzc, gfxPoint aHitTestPoint)

ScreenPoint?

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +338,5 @@
>    };
>    return CreateLayerTree(layerTreeSyntax, layerVisibleRegion, transforms, aLayerManager, aLayers);
>  }
>  
> +//TEST(AsyncPanZoomController, GetAPZCAtPoint) {

What happened here?
(In reply to :Ms2ger from comment #78)
> Did you mean aLayersId == aFirstPaintLayersId && aIsFirstPaint?

Not really. That's equivalent, but harder to understand.

(In reply to :Ms2ger from comment #79)
> > +AsyncPanZoomController*
> > +APZCTreeManager::GetAPZCAtPoint(AsyncPanZoomController* aApzc, gfxPoint aHitTestPoint)
> 
> ScreenPoint?

Yeah I was wondering if I should do that or not. I have to keep converting it to a gfxPoint though for the gfx3DMatrix transform.

> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
> >  
> > +//TEST(AsyncPanZoomController, GetAPZCAtPoint) {
> 
> What happened here?

See part 10.
Comment on attachment 782250 [details] [diff] [review]
Part 15 - Update metro code to use APZCTreeManager

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

Looks good, thanks for updating the metro bits!
Attachment #782250 - Flags: review?(netzen) → review+
Comment on attachment 781475 [details] [diff] [review]
Part 12 - Prevent BrowserElementPanning from interfering with multi-APZC

Does any of the touch handler makes any sense at all if the parent frame use azpc ? If not I tend to think I suggest to add a method to remove the mouse/touch listeners instead.
Attachment #781475 - Flags: review?(21)
Attachment #781467 - Flags: review?(bgirard) → review+
So the second try push listed in comment 76 has a purple on win8 opt mc, which is puzzling.

For the first try push, I used an m-c base of fb48c7d58b8b, applied all my patches, and pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=e9932d576f6a. This was green.

For the second try push, I used an m-c base of 73b69c146ca6, applied all the same patches (with some stuff qfolded together), and pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=b4e3a86d98a6. This has the purple on win8.

The diff from fb48c7d58b8b to e9932d576f6a (i.e. the first push) is identical to the diff from 73b69c146ca6 to b4e3a86d98a6 (i.e. the second push). I verified this by cloning try and comparing the output of hg diff -r <cset1> -r <cset2> for the two pushes, so I don't think this is caused by my mucking up qfolding.

I also pushed 73b69c146ca6 (m-c base from the failing push) with APZC enabled to https://tbpl.mozilla.org/?tree=Try&rev=bb5e3ab0169a and it was green. So it's not a problem in the m-c base that I was using. Unless I'm mistaken, the problem must lie in some interaction between the changes in fb48c7d58b8b..73b69c146ca6 and my patches.

The patches can be quickly applied by doing:
mkdir <tmp dir>; cd <tmp dir>
wget http://people.mozilla.com/~kgupta/tmp/apzc-patches.tgz
tar xzf apzc-patches.tgz
cd <hg-directory>
hg qimport <tmp dir>/*
hg qgoto 17-apzc-multi-safe-8f2905d

Note that the last patch in the series, 17-apzc-multi-safe-8f2905d, is the one that turns on APZC on metro. Patches 2..16 correspond to parts 1..15 attached to this bug.
Flags: needinfo?(netzen)
Comment on attachment 781468 [details] [diff] [review]
Part 5 - Add an APZCTreeManager to encapsulate the tree of APZC instances

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

::: gfx/layers/Compositor.h
@@ +389,5 @@
>    /**
> +   * Debug-build assertion that can be called to ensure code is running on the
> +   * compositor thread.
> +   */
> +  static void AssertOnCompositorThread();

optional: I wonder if it's better to inline the code here. I don't know if LTO will optimize away this call in optimized build.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +53,5 @@
> +    }
> +  }
> +
> +  if (controller) {
> +    mApzcs[aLayersId] = controller;

I think maybe my review comment for this got lost. Here we might be performing redundant insert/deletes into the map. The insert should maybe only be performed when the controller is inserted and likewise the erase should only be done if we're not creating a controller?

Could be worth asserting that the controller is in the map up to one time in case later the layer id where to change for a container layer which this code would not support.

::: gfx/layers/composite/APZCTreeManager.h
@@ +31,5 @@
> +                      FrameMetrics::ViewID aScrollId)
> +    : mLayersId(aLayersId)
> +    , mPresShellId(aPresShellId)
> +    , mScrollId(aScrollId)
> +  {

optional: I'm a big fan of MOZ_COUNT_CTOR/DTOR.

@@ +84,5 @@
> + * area, and changes to pan/zoom constraints.
> + *
> + * Note that the ClearTree function MUST be called when this class is no longer needed;
> + * see the method documentation for details.
> + */

Excellent class description.

@@ +103,5 @@
> +                                   uint64_t aLayersId, bool aIsFirstPaint);
> +
> +  /**
> +   * General handler for incoming input events. Manipulates the frame metrics
> +   * basde on what type of input it is. For example, a PinchGestureEvent will

typo: based
Attachment #781468 - Flags: review?(bgirard) → review+
Comment on attachment 781469 [details] [diff] [review]
Part 6 - Replace the placeholder std::map with an actual tree of APZC instances

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

::: gfx/layers/composite/APZCTreeManager.h
@@ +101,5 @@
>     *
>     * This must be called on the compositor thread as it walks the layer tree.
>     */
>    void UpdatePanZoomControllerTree(CompositorParent* aCompositor, Layer* aRoot,
> +                                   uint64_t aFirstPaintLayersId, bool aIsFirstPaint);

I think this name is a bit ambigious. I though it meant the layerID of this tree as it was went it was first painted and not the id of the layer that just had it's first paint. Maybe we should document this in the method comment.

Is that argument only checked if aIsFirstPaint is true? Then I think it's better style to have it come after.

@@ +235,4 @@
>     * This lock does not need to be held while manipulating a single APZC instance in
> +   * isolation (that is, if its tree pointers are not being accessed or mutated). The
> +   * lock also needs to be held when accessing the mRootApzc instance variable, as that
> +   * is considered part of the APZC tree management state. */

Total aside note: I was just thinking it would be cool to have a template like MustLock<...> mRootApzc under debug to guarantee these properties.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +13,5 @@
>  #include "mozilla/RefPtr.h"
>  #include "InputData.h"
>  #include "Axis.h"
>  #include "TaskThrottler.h"
> +#include "mozilla/layers/APZCTreeManager.h"

Forward declare please. But I don't even see where it's needed.
Attachment #781469 - Flags: review?(bgirard) → review+
Attachment #781470 - Flags: review?(bgirard) → review+
Comment on attachment 781471 [details] [diff] [review]
Part 8 - Extract a GetCurrentAsyncTransform function

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1151,5 @@
>  }
>  
> +ViewTransform AsyncPanZoomController::GetCurrentAsyncTransform() {
> +  MonitorAutoLock mon(mMonitor);
> +  return GetCurrentAsyncTransformInternal();

What's the point of this? I assumed our monitor were re-entrant?
Attachment #781471 - Flags: review?(bgirard) → review+
Comment on attachment 781472 [details] [diff] [review]
Part 9 - Add visible region tracking and hit testing code to APZCTreeManager

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +9,3 @@
>  #include "Compositor.h"
>  
> +#define APZC_LOG(...)

Looks like this can only be used if someone implements this. Perhaps leave the code commented out or behind an ifdef?

@@ +63,5 @@
>                                  &apzcsToDestroy);
>    }
>  
>    for (int i = apzcsToDestroy.Length() - 1; i >= 0; i--) {
> +    APZC_LOG("Destroying APZC at %p", apzcsToDestroy[i].get());

\n

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +338,5 @@
>    };
>    return CreateLayerTree(layerTreeSyntax, layerVisibleRegion, transforms, aLayerManager, aLayers);
>  }
>  
> +//TEST(AsyncPanZoomController, GetAPZCAtPoint) {

Will you port this test to APZCTreeManager?
Comment on attachment 781473 [details] [diff] [review]
Part 10 - Fix up tests

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +20,5 @@
>    AsyncPanZoomController::InitializeGlobalState();
>  }
>  
> +void
> +APZCTreeManager::AssertOnCompositorThread()

I don't see much value in this change but I don't necessarily opposed.

::: gfx/layers/composite/APZCTreeManager.h
@@ +209,5 @@
> +   * compositor thread.
> +   */
> +  virtual void AssertOnCompositorThread();
> +
> +public:

We could make this protected and not have to expose these publicly but that's fine as-is.

::: gfx/layers/ipc/CompositorParent.h
@@ +37,5 @@
>  class LayerManagerComposite;
>  class AsyncCompositionManager;
>  struct TextureFactoryIdentifier;
>  
> +struct LayerTreeAutoRegistration

s/Auto/Scoped is better IMO because auto doesn't imply it will de-register.
Attachment #781473 - Flags: review?(bgirard) → review+
Comment on attachment 781474 [details] [diff] [review]
Part 11 - Update TabChild to handle moving subframes

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

::: dom/ipc/TabChild.h
@@ +423,5 @@
>                                bool* aWindowIsNew,
>                                nsIDOMWindow** aReturn);
>  
>      already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils();
> +    already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils(nsIContent* content);

aContent. I don't understand when you wouldn't want to use the default one. Might be worth documenting.
Attachment #781474 - Flags: review?(bgirard) → review+
Comment on attachment 781475 [details] [diff] [review]
Part 12 - Prevent BrowserElementPanning from interfering with multi-APZC

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

I really don't know this code very well. Will this not interfere with js touch event listeners on sub-frames?
Attachment #781719 - Flags: review?(bgirard) → review+
Attachment #782248 - Flags: review?(bgirard) → review+
Attachment #782250 - Flags: review?(bgirard) → review+
Attachment #781472 - Flags: review?(bgirard) → review+
Attachment #781475 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #84)
> > +  static void AssertOnCompositorThread();
> 
> optional: I wonder if it's better to inline the code here. I don't know if
> LTO will optimize away this call in optimized build.

Tried doing this, but I needed to include CompositorParent.h into Compositor.h which broke the world. I can do this as a follow-up.

> > +  if (controller) {
> > +    mApzcs[aLayersId] = controller;
> 
> I think maybe my review comment for this got lost. Here we might be
> performing redundant insert/deletes into the map. The insert should maybe
> only be performed when the controller is inserted and likewise the erase
> should only be done if we're not creating a controller?
> 
> Could be worth asserting that the controller is in the map up to one time in
> case later the layer id where to change for a container layer which this
> code would not support.

This code was mostly "placeholder" stuff for the purposes of having a cleaner patch queue - this was removed in part 6. I don't think it's really worth fixing it up since it's an intermediate piece of code.

> > +  {
> 
> optional: I'm a big fan of MOZ_COUNT_CTOR/DTOR.

Added.

> 
> typo: based

Fixed.

(In reply to Benoit Girard (:BenWa) from comment #85)
> >    void UpdatePanZoomControllerTree(CompositorParent* aCompositor, Layer* aRoot,
> > +                                   uint64_t aFirstPaintLayersId, bool aIsFirstPaint);
> 
> I think this name is a bit ambigious. I though it meant the layerID of this
> tree as it was went it was first painted and not the id of the layer that
> just had it's first paint. Maybe we should document this in the method
> comment.
> 
> Is that argument only checked if aIsFirstPaint is true? Then I think it's
> better style to have it come after.

I added documentation to this method signature to explain the parameters and reversed the order of the bool and layers id.

> @@ +235,4 @@
> >  #include "TaskThrottler.h"
> > +#include "mozilla/layers/APZCTreeManager.h"
> 
> Forward declare please. But I don't even see where it's needed.

I will have a followup to try and prune these includes. Removing that one caused breakage that I couldn't easily fix.

(In reply to Benoit Girard (:BenWa) from comment #86)
> > +ViewTransform AsyncPanZoomController::GetCurrentAsyncTransform() {
> > +  MonitorAutoLock mon(mMonitor);
> > +  return GetCurrentAsyncTransformInternal();
> 
> What's the point of this? I assumed our monitor were re-entrant?

Unfortunately, no. I can make the monitor a ReentrantMonitor as part of fixing bug 890932 and remove the Internal method here.

(In reply to Benoit Girard (:BenWa) from comment #87)
> Comment on attachment 781472 [details] [diff] [review]
> > +#define APZC_LOG(...)
> 
> Looks like this can only be used if someone implements this. Perhaps leave
> the code commented out or behind an ifdef?

Done, I added a commented-out version of it implemented.

> @@ +63,5 @@
> > +    APZC_LOG("Destroying APZC at %p", apzcsToDestroy[i].get());
> 
> \n

Fixed.

> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
> >  
> > +//TEST(AsyncPanZoomController, GetAPZCAtPoint) {
> 
> Will you port this test to APZCTreeManager?

Yeah, part 10.

(In reply to Benoit Girard (:BenWa) from comment #88)
> > +void
> > +APZCTreeManager::AssertOnCompositorThread()
> 
> I don't see much value in this change but I don't necessarily opposed.

As discussed, this was needed to no-op the assertion in the gtest code.

> ::: gfx/layers/composite/APZCTreeManager.h
> > +public:
> 
> We could make this protected and not have to expose these publicly but
> that's fine as-is.

I plan on making these private again (and restoring the full functionality of this test) as part of bug 890280. The idea there is instead of calling GetTargetAPZC all the time in the test, I'll just send a click event to the APZCTreeManager and see what the MockContentController gets as a CSSPoint in its HandleSingleTap callback. That will test both the hit-testing and the point coordinate conversion.

> ::: gfx/layers/ipc/CompositorParent.h
> >  
> > +struct LayerTreeAutoRegistration
> 
> s/Auto/Scoped is better IMO because auto doesn't imply it will de-register.

Renamed to ScopedLayerTreeRegistration. I had originally modelled this after MonitorAutoLock but I like scoped better too.

(In reply to Benoit Girard (:BenWa) from comment #89)
> >      already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils();
> > +    already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils(nsIContent* content);
> 
> aContent. I don't understand when you wouldn't want to use the default one.
> Might be worth documenting.

Fixed and documented. This is to get the DOMWindowUtils for the right window if the content belongs to an iframe.

(In reply to Benoit Girard (:BenWa) from comment #90)
> I really don't know this code very well. Will this not interfere with js
> touch event listeners on sub-frames?

I have a new patch for this for vivien to review. But the goal of this patch is to disable the js touch event listeners on sub-frames, in cases where the page is being controlled by the APZC. That way it won't interfere with nested APZCs.
Filed bug 899336 as a follow-up for the #include cleanup and assertion inlining.
Updated to not register the event listeners at all, which should avoid some unnecessary event dispatching.
Attachment #781475 - Attachment is obsolete: true
Attachment #782814 - Flags: review?(21)
Attachment #781468 - Flags: review?(matt.woodrow) → review+
Comment on attachment 781469 [details] [diff] [review]
Part 6 - Replace the placeholder std::map with an actual tree of APZC instances

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +88,1 @@
>                                                    AsyncPanZoomController::USE_GESTURE_DETECTOR);

Won't this leak? It's a refcounted object and we're allocating it into a raw pointer. What will release that intial reference?

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +634,5 @@
> +  AsyncPanZoomController* GetLastChild() const { return mLastChild; }
> +  AsyncPanZoomController* GetPrevSibling() const { return mPrevSibling; }
> +private:
> +  nsRefPtr<AsyncPanZoomController> mLastChild;
> +  nsRefPtr<AsyncPanZoomController> mPrevSibling;

Do these need to be strong references? I assume the layer is what actually owns the APZC.

Having weak pointers forces us to ensure that the lifetime is managed correctly. Though the penalty for getting it wrong is use-after-free rather than things just living longer than expected. Your call.
Attachment #781469 - Flags: review?(matt.woodrow) → review+
Comment on attachment 781472 [details] [diff] [review]
Part 9 - Add visible region tracking and hit testing code to APZCTreeManager

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +9,3 @@
>  #include "Compositor.h"
>  
> +#define APZC_LOG(...)

I think this is ok. It's pretty easy for someone to implement their preferred logging when they need it.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +643,5 @@
> +   * area that this APZC instance is responsible for. This is used when
> +   * hit-testing to see which APZC instance should handle touch events.
> +   */
> +public:
> +  void SetVisibleRegion(gfxRect rect) { mVisibleRegion = rect; }

Comment that this is the viewport of the layer, post-transform (in the coordinate space of its parent layer).
Attachment #781472 - Flags: review?(matt.woodrow) → review+
Attachment #781474 - Flags: review?(matt.woodrow) → review+
Comment on attachment 782814 [details] [diff] [review]
Part 12 - Prevent BrowserElementPanning from interfering with multi-APZC (v2)

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

::: dom/browser-element/BrowserElementPanning.js
@@ +47,5 @@
>  
> +    // If we are using an AsyncPanZoomController for the parent frame,
> +    // it will handle subframe scrolling too. We don't need to listen for
> +    // these events.
> +    if (! this._asyncPanZoomForViewportFrame) {

nit: remove the extra space between ! and this._asyncPanZoomForViewportFrame
Attachment #782814 - Flags: review?(21) → review+
Attachment #782248 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #95)
> ::: gfx/layers/composite/APZCTreeManager.cpp
AsyncPanZoomController::USE_GESTURE_DETECTOR);
> 
> Won't this leak? It's a refcounted object and we're allocating it into a raw
> pointer. What will release that intial reference?

As per discussion, it should start with a refcount of 0 and so not leak.

> ::: gfx/layers/ipc/AsyncPanZoomController.h
> Do these need to be strong references? I assume the layer is what actually
> owns the APZC.
> 
> Having weak pointers forces us to ensure that the lifetime is managed
> correctly. Though the penalty for getting it wrong is use-after-free rather
> than things just living longer than expected. Your call.

I would rather have the strong references, because if the only strong refs are from the Layer, then the APZC instances will get destroyed right on a layer update as the old layers are discarded. That means we can't call the Destroy() function on them before they go away, which is currently required. If we get rid of the Destroy() function then we can revisit this; I think that's doable but nontrivial.

(In reply to Matt Woodrow (:mattwoodrow) from comment #96)
> > +#define APZC_LOG(...)
> 
> I think this is ok. It's pretty easy for someone to implement their
> preferred logging when they need it.

I already added a commented-out line just below this that defines it to be printf_stderr. People can redefine it as they wish, so I left that as-is.

> ::: gfx/layers/ipc/AsyncPanZoomController.h
> > +public:
> > +  void SetVisibleRegion(gfxRect rect) { mVisibleRegion = rect; }
> 
> Comment that this is the viewport of the layer, post-transform (in the
> coordinate space of its parent layer).

Done. (I added the comment on the mVisibleRegion field declaration).

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #97)
> > +    if (! this._asyncPanZoomForViewportFrame) {
> 
> nit: remove the extra space between ! and this._asyncPanZoomForViewportFrame

Done.
This appears to break panning in the b2g browser.
Depends on: 899810
Flags: needinfo?(netzen)
(In reply to Michael Wu [:mwu] from comment #101)
> This appears to break panning in the b2g browser.

Can you file a bug with specific STR? I did test panning on my Peak and it worked. At least it worked as well as it did before.
http://ratp.fr can't be zoomed in with current gecko. I tried with a gecko just before this patch and it works correctly.

Similarly, merely pressing the button in http://everlong.org/mozilla/testcase-button/ doesn't work in current gecko, and works before this patch.

I'm currently building a gecko at revision 433279660add and I'll report then.
So, yes, the problem happens with revision 433279660add.

I tink this should be backed out because it badly breaks FxOS browser.
Smoketest regression confirmed by two people on the b2g browser.

Ryan - Can you back this out?
Flags: needinfo?(ryanvm)
you can try http://everlong.org/mozilla/notifications/ to press buttons as well (they're bigger ;) )
This already has 3 days' worth of commits piled on top. It doesn't backout cleanly and I don't think I'm the right person to figure out the conflicts.
Flags: needinfo?(ryanvm)
If I can't make any progress with reproducing/fixing these issues tomorrow I'll back it out.
Just tried the nightly Firefox for Android and it works ok.
There's actually a few separate issues here that were all sort of introduced by this patch. However I'd like to leave it in the tree and land additional fixes on top rather than back it out. I have fixes locally that address the blocker issues and I am making good progress on the (less severe) remaining problems, and should have a patch for those soon as well.

One blocker issue reported by bbondy in bug 899810 was that the browser would get stuck in a blank screen. This happens on B2G as well in some cases (such as if you scroll down the page, turn off the screen, and then turn it back on and go into the browser). I have a patch for this issue on bug 899810 and will land that shortly.

The other blocker issue was reported by nhirata in bug 900742 where he said it was impossible to scroll or click on links. I have a patch for that as well which is awaiting review.

There are a couple of other smaller issues that I'm investigating. One is that the focus point for pinches seems off (also covered by bug 899810) and the other is that sometimes after a large pinch it's possible to get stuck in a bad state - this is aggravated by the focus point problem and will be mostly eliminated once I fix that.
Depends on: 900742
No longer blocks: 901340
Depends on: 901340
No longer blocks: 901339
Depends on: 901339
Depends on: 903498
Blocks: 906877
You need to log in before you can comment on or make changes to this bug.