866232 - Support having multiple APZC instances attached to the layer tree

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

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.

Comment 1

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

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.

Comment 2

[David Zbarsky (:dzbarsky)]

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.

Comment 3

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

Attachment: Part 1 - Update the GeckoContentController interface to support multiple APZCs

Comment 4

[Benoit Girard (:BenWa)]

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.

Comment 5

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

Attachment: Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

Not sure who is the more appropriate reviewer here.

Comment 6

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

Attachment: Part 2 - Add an APZCTreeManager class to encapsulate the APZC tree

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

Comment 7

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

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

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 9

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

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

Comment 10

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

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 11

[Benoit Girard (:BenWa)]

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.

Comment 12

[Benoit Girard (:BenWa)]

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.

Comment 13

[David Zbarsky (:dzbarsky)]

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

Comment 14

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

Attachment: Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

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.

Comment 15

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

Attachment: Part 2 - Flip APZC default behaviour

Comment 16

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

Attachment: Part 3 - Add an APZCTreeManager to manage the APZC tree

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.

Comment 17

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

Attachment: Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

Comment 18

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

Attachment: Part 2 - Flip APZC default behaviour

This makes future patches simpler since there's less stuff to pass around.

Comment 19

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

Attachment: Part 3 - Allow APZC instances to be created on the compositor thread

Comment 20

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

Attachment: Part 4 - Add an APZCTreeManager to encapsulate the tree of APZC instances

Not ready for review just yet but posting it as a WIP

Comment 21

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

Attachment: Part 5 - Implement the tree building and searching parts of APZCTreeManager

Comment 22

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

Attachment: Part 4 - Add an APZCTreeManager to encapsulate the tree of APZC instances

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.

Comment 23

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

Attachment: Part 5 - Implement the tree building and searching parts of APZCTreeManager

Comment 24

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

Attachment: Part 6 - Some cleanup in APZC, no functional changes

Comment 25

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

Attachment: Part 7 - Extract a GetCurrentAsyncTransform method in APZC

Comment 26

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

Attachment: Part 8 - Add visible region tracking and hit testing code

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 27

[Benoit Girard (:BenWa)]

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!=

Comment 28

[Benoit Girard (:BenWa)]

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?

Comment 29

[Benoit Girard (:BenWa)]

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?

Comment 30

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

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

Comment 31

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

Attachment: Part 1 - Allow the GeckoContentController interface to identify which layer is being operated upon

Updated with operator== and operator!= overloaded on ScrollableLayerGuid, carrying r+.

Comment 32

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

Attachment: Part 2 - Flip APZC default behaviour

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.

Comment 33

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

Attachment: Part 3 - Allow APZC instances to be created on the compositor thread

Rebased, carrying r+

Comment 34

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

Attachment: Part 4 - Allow APZC::Destroy() to be called on the compositor thread

Comment 35

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

Attachment: Part 5 - Add support for delaying APZC creation until the layer update

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.

Comment 36

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

Attachment: Part 6 - Add an APZCTreeManager to encapsulate the tree of APZC instances

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.

Comment 37

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

Attachment: Part 7 - Replace the placeholder std::map with an actual tree of APZC instances

Comment 38

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

Attachment: Part 8 - Some cleanup in APZC, no functional changes

Comment 39

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

Attachment: Part 9 - Extract a GetCurrentAsyncTransform method in APZC

Comment 40

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

Attachment: Part 10 - Add visible region tracking and hit-testing code in APZCTreeManager

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!

Comment 41

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

Attachment: Part 11 - Put back the test I commented out and rebase it

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.

Comment 42

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

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

Comment 43

[Matt Woodrow (:mattwoodrow)]

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.

Comment 44

[Matt Woodrow (:mattwoodrow)]

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

Comment 45

[Matt Woodrow (:mattwoodrow)]

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 46

[Matt Woodrow (:mattwoodrow)]

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 47

[Matt Woodrow (:mattwoodrow)]

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 48

[Matt Woodrow (:mattwoodrow)]

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.

Comment 49

[Matt Woodrow (:mattwoodrow)]

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

Comment 50

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

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

Comment 51

[Benoit Girard (:BenWa)]

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.

Comment 52

[Benoit Girard (:BenWa)]

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 53

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

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.

Comment 54

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

Attachment: Part 1 - Reserve a layers ID for the root layer

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+

Comment 55

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

Comment on attachment 778069 [details] [diff] [review]
Part 2 - Flip APZC default behaviour

This patch is not needed any more.

Comment 56

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

Attachment: Part 2 - Refactor to move all the main-thread APZC init stuff in one place

Rebased, carrying r+

Comment 57

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

Attachment: Part 3 - Allow APZC::Destroy() to be called on the compositor thread

Rebased, carrying r+

Comment 58

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

Attachment: Part 4 - Add support for delaying APZC creation until the layer update

Rebased, updated to address review comments. I moved SetFrameMetricsForTest out of APZC and into a TestAPZC class, so re-requesting review for that churn.

Comment 59

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

Attachment: Part 5 - Add an APZCTreeManager to encapsulate the tree of APZC instances

Comment 60

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

Attachment: Part 6 - Replace the placeholder std::map with an actual tree of APZC instances

Comment 61

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

Attachment: Part 7 - Some documentation/renaming cleanup

Comment 62

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

Attachment: Part 8 - Extract a GetCurrentAsyncTransform function

Comment 63

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

Attachment: Part 9 - Add visible region tracking and hit testing code to APZCTreeManager

Comment 64

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

Attachment: Part 10 - Fix up tests

Comment 65

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

Attachment: Part 11 - Update TabChild to handle moving subframes

Comment 66

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

Attachment: Part 12 - Prevent BrowserElementPanning from interfering with multi-APZC

Comment 67

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

Attachment: Part 13 - Backwards compatibility for B2G browser

Comment 68

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

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

Comment 69

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

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

Comment 70

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

Attachment: Part 13 - Backwards compatibility for B2G browser

Small compilation fix for android.

Comment 71

[Botond Ballo [:botond]]

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.

Comment 72

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

Attachment: Part 14 - Break assumption of a global root layer

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.

Comment 73

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

Attachment: Part 15 - Update metro code to use APZCTreeManager

Try build in progress with this patch at https://tbpl.mozilla.org/?tree=Try&rev=64ce5c2c5226

Comment 74

[Brian R. Bondy [:bbondy]]

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.

Comment 75

[Brian R. Bondy [:bbondy]]

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

Comment 76

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

Attachment: Part 14 - Break assumption of a global root layer

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

Comment 77

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

Attachment: Part 15 - Update metro code to use APZCTreeManager

Oops, I uploaded the wrong patch. The pref-enabling was just for try purposes. This is the actual patch to the metro code.

Comment 78

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 79

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 80

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

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

[Brian R. Bondy [:bbondy]]

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!

Comment 82

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 83

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

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.

Comment 84

[Benoit Girard (:BenWa)]

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

Comment 85

[Benoit Girard (:BenWa)]

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.

Comment 86

[Benoit Girard (:BenWa)]

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?

Comment 87

[Benoit Girard (:BenWa)]

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 88

[Benoit Girard (:BenWa)]

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.

Comment 89

[Benoit Girard (:BenWa)]

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.

Comment 90

[Benoit Girard (:BenWa)]

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?

Comment 91

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

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

Comment 92

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

Filed bug 899336 as a follow-up for the #include cleanup and assertion inlining.

Comment 93

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

Attachment: Part 12 - Prevent BrowserElementPanning from interfering with multi-APZC (v2)

Updated to not register the event listeners at all, which should avoid some unnecessary event dispatching.

Comment 95

[Matt Woodrow (:mattwoodrow)]

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.

Comment 96

[Matt Woodrow (:mattwoodrow)]

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

Comment 97

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 98

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

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

Comment 99

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

*crosses fingers*

https://hg.mozilla.org/projects/birch/e0e4277435b4
https://hg.mozilla.org/projects/birch/ef02c1c1d6d5
https://hg.mozilla.org/projects/birch/30beb4f3d321
https://hg.mozilla.org/projects/birch/4fd2b66ff996
https://hg.mozilla.org/projects/birch/f856d234e9b2
https://hg.mozilla.org/projects/birch/bda5c115afd9
https://hg.mozilla.org/projects/birch/c3c079a743c0
https://hg.mozilla.org/projects/birch/9a4478e65fd6
https://hg.mozilla.org/projects/birch/f69d34718660
https://hg.mozilla.org/projects/birch/a960ee129d91
https://hg.mozilla.org/projects/birch/6323f687fa3c
https://hg.mozilla.org/projects/birch/4751c26f302a
https://hg.mozilla.org/projects/birch/2d30f705e17f
https://hg.mozilla.org/projects/birch/bdf483d094bf
https://hg.mozilla.org/projects/birch/ca9bf5ddb290

Comment 100

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

Follow-up for build breakage:
https://hg.mozilla.org/projects/birch/rev/433279660add

Comment 101

[Michael Wu [:mwu]]

This appears to break panning in the b2g browser.

Comment 102

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0e4277435b4
https://hg.mozilla.org/mozilla-central/rev/ef02c1c1d6d5
https://hg.mozilla.org/mozilla-central/rev/30beb4f3d321
https://hg.mozilla.org/mozilla-central/rev/4fd2b66ff996
https://hg.mozilla.org/mozilla-central/rev/f856d234e9b2
https://hg.mozilla.org/mozilla-central/rev/bda5c115afd9
https://hg.mozilla.org/mozilla-central/rev/c3c079a743c0
https://hg.mozilla.org/mozilla-central/rev/9a4478e65fd6
https://hg.mozilla.org/mozilla-central/rev/f69d34718660
https://hg.mozilla.org/mozilla-central/rev/a960ee129d91
https://hg.mozilla.org/mozilla-central/rev/6323f687fa3c
https://hg.mozilla.org/mozilla-central/rev/4751c26f302a
https://hg.mozilla.org/mozilla-central/rev/2d30f705e17f
https://hg.mozilla.org/mozilla-central/rev/bdf483d094bf
https://hg.mozilla.org/mozilla-central/rev/ca9bf5ddb290
https://hg.mozilla.org/mozilla-central/rev/433279660add

Comment 103

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

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

Comment 104

[Julien Wajsberg [:julienw]]

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.

Comment 105

[Julien Wajsberg [:julienw]]

So, yes, the problem happens with revision 433279660add.

I tink this should be backed out because it badly breaks FxOS browser.

Comment 106

[Jason Smith [:jsmith]]

Smoketest regression confirmed by two people on the b2g browser.

Ryan - Can you back this out?

Comment 107

[Julien Wajsberg [:julienw]]

you can try http://everlong.org/mozilla/notifications/ to press buttons as well (they're bigger ;) )

Comment 108

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 109

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

If I can't make any progress with reproducing/fixing these issues tomorrow I'll back it out.

Comment 110

[Julien Wajsberg [:julienw]]

Just tried the nightly Firefox for Android and it works ok.

Comment 111

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

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.