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

As part of the work for bug 967844 we need to be able to store multiple FrameMetrics and APZC instances per layer. See the discussion at bug 967844 comment 39 onwards to comment 51 (and maybe beyond...).

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

If we have multiple FM objects per layer, then we need multiple handoff parents per layer, and so it makes more sense just to move the handoff parent into the FrameMetrics.

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

This was my idea to encapsulate the ugly that results from having multiple FrameMetrics per layer. I also updated the APZCTreeManager code to show how it would be used. For the most part it just replaces the Layer* with nsRefPtr<LayerMetricsWrapper>, provided that we have all the necessary delegator functions on LayerMetricsWrapper.

Also we can play around with making LayerMetricsWrapper less memory-intensive (it generates a lot of transient garbage while using it) but for now I just wanted it to be as close to a drop-in replacement as possible to assess if this is a viable way forward.

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

Propagating the LayerMetricsWrapper into more places that use the FrameMetrics.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 10 years ago

Comment on attachment 8476889 [details] [diff] [review]
Part 4 WIP - Add LayerMetricsWrapper to encapsulate multi-FrameMetrics ugliness

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

Looks like a reasonable idea.

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

Comment on attachment 8476890 [details] [diff] [review]
Part 5 WIP - More LayerMetricsWrapper propagation

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

::: gfx/layers/Layers.h
@@ +346,5 @@
>     * Returns a list of all descendant layers for which
>     * GetFrameMetrics().IsScrollable() is true and that
>     * do not already have an ancestor in the return list.
>     */
> +  void GetScrollableLayers(nsTArray<nsRefPtr<LayerMetricsWrapper> >& aArray);

I don't feel good about exposing LayerMetricsWrapper in Layers.h. Can we keep it private to the compositor/APZC?

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

I can put it in some other header file but i still would like to use it for the two functions in Layers.cpp. GetPrimaryScrollableFrame in particular is called from the tiling code and if I don't use it for that function then it's going to be hard to use LayerMetricsWrapper in the rest of the tiling code as well.

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

Put it in a separate header file, that's OK.

	Benoit Girard (:BenWa)
	Comment 12 • 10 years ago

Comment on attachment 8476887 [details] [diff] [review]
Part 2 WIP - Change Layers API

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

::: gfx/layers/Layers.h
@@ +1553,5 @@
>    void* mImplData;
>    nsRefPtr<Layer> mMaskLayer;
>    gfx::UserData mUserData;
>    nsIntRegion mVisibleRegion;
> +  nsTArray<FrameMetrics> mFrameMetrics;

Do we have somewhere appropriate to document what it means for a layer to have multiple FM. This is probably a good place to explain this. Better yet point to doxygen here.

Here's my attempt but I have only a partial understanding so perhaps you can write something better:

A layer can have multiple FM, this means that this layer has multiple scrollable ancestor. The same FM (matching scrollid) can be added to many layers, this means that the scrollable container has multiple active layer children. These may be interleaved with layers that don't match this FM which wont scroll. FM in the layer tree with the same scrollid should be equal.

Like I said on IRC it would be nice to have assertions to check that a layer doesn't have more than more FM with the same scroll id. And any FM in a layer tree with the same scroll id are equal.

	Benoit Girard (:BenWa)
	Comment 13 • 10 years ago

Comment on attachment 8476889 [details] [diff] [review]
Part 4 WIP - Add LayerMetricsWrapper to encapsulate multi-FrameMetrics ugliness

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

::: gfx/layers/Layers.h
@@ +1611,5 @@
> + * region and clip rect as the layer.
> + * This class purposely does not expose the target layer directly to avoid
> + * user code from accidentally calling functions directly on it. Instead
> + * any necessary functions should be wrapped in this class.
> + */

Maybe an ASCII art picture of what you sent me here would help. The comment explains this well but if you don't understand it's not evident what you're describing here.

http://asciiflow.com/ might help.

@@ +1637,5 @@
> +
> +  nsRefPtr<LayerMetricsWrapper> GetLastChild() const
> +  {
> +    if (!AtBottomLayer()) {
> +      return new LayerMetricsWrapper(mLayer, mIndex - 1);

iterating calls new which is slow and locks the arena. The iterator pattern would fix this but I'm not sure if it's worth it.

@@ +1655,5 @@
> +    }
> +    return nullptr;
> +  }
> +
> +  FrameMetrics Metrics() const

why not const&?

@@ +1708,5 @@
> +  }
> +
> +  nsIntRegion GetVisibleRegion() const
> +  {
> +    if (AtBottomLayer()) {

I don't understand why this is a special case but FN 1..n are not. This could use a comment.

@@ +1726,5 @@
> +  {
> +    return mLayer->GetContentDescription();
> +  }
> +
> +  void* LayerPointer() const

Not a fan of starting to use <X>Pointer. Normally we use Layer()/GetLayer().

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 14 • 10 years ago
red try

Try push with my latest set of patches: https://tbpl.mozilla.org/?tree=Try&rev=8e535f0634e6

I think this has all the code changes needed, but other than running the gtests I haven't tested it at all. It also needs documentation, and I'm going to convert the LayerMetricsWrapper to a MOZ_STACK_CLASS so we don't allocate gobs of them on the heap while walking around in the layer tree.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 10 years ago
red try

Updated to hopefully not break so badly. https://tbpl.mozilla.org/?tree=Try&rev=41ab4f1c9aed

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 10 years ago
orange try

This one should be even better: https://tbpl.mozilla.org/?tree=Try&rev=85370fb53718

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

That last try push seems to be doing well, except for a bunch of android reftest failures which look worrisome. I also did some sanity checks locally on B2G and Fennec and neither had any obvious problems.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 18 • 10 years ago
build-only try

Now with LayerMetricsWrapper changed to be a MOZ_STACK_CLASS: https://tbpl.mozilla.org/?tree=Try&rev=380cf525a4e3

I'm able to repro the android reftest failures locally but having a hard time isolating and debugging them because for some reason my changes to the reftest harness are being ignored. :(

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

Bisection narrowed down the reftest failure to part 6 (cset b1bf7a5da754 in the try push from comment 18).

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

In hindsight https://bugzilla.mozilla.org/show_bug.cgi?id=982888#c36 was a poor decision on my part :)

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

This still needs more documentation

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

This also needs more documentation

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 36 • 10 years ago
green try

Try push that should have everything good (fingers crossed): https://tbpl.mozilla.org/?tree=Try&rev=5885415783de

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

Since I was touching this code I figured I might as well fix https://bugzilla.mozilla.org/show_bug.cgi?id=1056159#c9

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

I think some of the bits of code in ContainerRender now need to be moved out because they assume container layers are the only scrollable layers which is no longer true. However it's mostly debug/diagnostic tool code so I'll file a follow-up bug for that. I don't want it to hold up landing this in 34.

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

It looks like the patch you have on bug 967844 rewrites most of this code. Let me know if you'd rather I didn't land this patch.

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

Yeah this is kinda ugly. I'll file a follow-up to do this more elegantly.

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45)
> I think some of the bits of code in ContainerRender now need to be moved out
> because they assume container layers are the only scrollable layers which is
> no longer true. However it's mostly debug/diagnostic tool code so I'll file
> a follow-up bug for that. I don't want it to hold up landing this in 34.

Filed bug 1058884 for this.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> Yeah this is kinda ugly. I'll file a follow-up to do this more elegantly.

Filed bug 1058886 for this.

	Botond Ballo [:botond]
	Comment 52 • 10 years ago

Comment on attachment 8479141 [details] [diff] [review]
Part 2 (diff ignoring whitespace changes)

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +258,5 @@
> +  // need to recurse any deeper because the fixed layers are relative to their
> +  // nearest scrollable layer. ApplyAsyncContentTransformToTree will call this
> +  // function again for nested scrollable layers.
> +  if (aLayer == aTransformedSubtreeRoot || !needsAlignment) {
> +    if (aLayer == aTransformedSubtreeRoot || !aLayer->GetFrameMetrics().IsScrollable()) {

'if (A || B) { if (A || C) { ... } }'  can be folded into 'if (A || B || C) { ... }'

	Botond Ballo [:botond]
	Comment 53 • 10 years ago

Comment on attachment 8479140 [details] [diff] [review]
Part 2 - Rearrange some code (no functional changes)

See r+ in whitespace-ignoring patch.

	Botond Ballo [:botond]
	Comment 54 • 10 years ago

Comment on attachment 8479141 [details] [diff] [review]
Part 2 (diff ignoring whitespace changes)

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +253,5 @@
>        aTransformedSubtreeRoot->GetFrameMetrics().GetScrollId();
> +  bool needsAlignment = (isRootFixed || isStickyForSubtree);
> +
> +  // We want to process all the fixed and sticky children of
> +  // aTransformedSubtreeRoot. Also, once we do encounter such a child, we don't

Should this say "once we encounter a scrollable child"?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 55 • 10 years ago

Comment on attachment 8479285 [details] [diff] [review]
Part 9 - Update SetAsyncScrollOffset API

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

I'll change this again in my patch. (I've already done the rebase.) This is fine to land as-is.

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

(In reply to Botond Ballo [:botond] from comment #52)
> Comment on attachment 8479141 [details] [diff] [review]
> 
> 'if (A || B) { if (A || C) { ... } }'  can be folded into 'if (A || B || C)
> { ... }'

There's a return statement inside the first if but outside the second that prevents that from happening.

	Botond Ballo [:botond]
	Comment 57 • 10 years ago

Comment on attachment 8479141 [details] [diff] [review]
Part 2 (diff ignoring whitespace changes)

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> (In reply to Botond Ballo [:botond] from comment #52)
> > Comment on attachment 8479141 [details] [diff] [review]
> > 
> > 'if (A || B) { if (A || C) { ... } }'  can be folded into 'if (A || B || C)
> > { ... }'
> 
> There's a return statement inside the first if but outside the second that
> prevents that from happening.

Oops, you're right! Ignore that.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +250,5 @@
>      !aLayer->GetParent()->GetIsFixedPosition();
>    bool isStickyForSubtree = aLayer->GetIsStickyPosition() &&
>      aLayer->GetStickyScrollContainerId() ==
>        aTransformedSubtreeRoot->GetFrameMetrics().GetScrollId();
> +  bool needsAlignment = (isRootFixed || isStickyForSubtree);

I think it would enhance the readability of this code (particularly in conjunction with the comment below) if this were called isFixedOrSticky.

@@ +253,5 @@
>        aTransformedSubtreeRoot->GetFrameMetrics().GetScrollId();
> +  bool needsAlignment = (isRootFixed || isStickyForSubtree);
> +
> +  // We want to process all the fixed and sticky children of
> +  // aTransformedSubtreeRoot. Also, once we do encounter such a child, we don't

I think I answered my own question - no it shouldn't, we mean "a fixed or sticky child", and that's clear.

@@ +256,5 @@
> +  // We want to process all the fixed and sticky children of
> +  // aTransformedSubtreeRoot. Also, once we do encounter such a child, we don't
> +  // need to recurse any deeper because the fixed layers are relative to their
> +  // nearest scrollable layer. ApplyAsyncContentTransformToTree will call this
> +  // function again for nested scrollable layers.

Please move the last sentence inside the outer 'if', as that pertains to why we are skipping scrollable layers.

	Botond Ballo [:botond]
	Comment 58 • 10 years ago

Comment on attachment 8479269 [details] [diff] [review]
Part 4 - Update APZCTreeManager code and add a LayerMetricsWrapper class

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

r+ with comments addressed. Nice work at abstracting away the multi-FrameMetrics messiness from APZCTreeManager!

::: gfx/layers/LayerMetricsWrapper.h
@@ +103,5 @@
> + *
> + * It is possible to wrap a nullptr in a LayerMetricsWrapper, in which case
> + * the IsValid() function will return false. This is required to allow
> + * LayerMetricsWrapper to be a MOZ_STACK_CLASS (desirable because it is used
> + * in loops and recursion).

The Maybe class might be a good fit here. We can leave any exploration of this to a later bug, though.

@@ +109,5 @@
> + * This class purposely does not expose the wrapped layer directly to avoid
> + * user code from accidentally calling functions directly on it. Instead
> + * any necessary functions should be wrapped in this class. It does expose
> + * the wrapped layer as a void* for printf purposes, and in case there is
> + * some use case where the underlying layer is actually required.

I'd remove "and in case ... actually required". Let's not encourage people to cast void*'s to things :)

@@ +153,5 @@
> +  {
> +    return mLayer != nullptr;
> +  }
> +
> +  operator bool() const

Please add MOZ_EXPLICIT_CONVERSION in front of this. Classes with implicit conversions to bool are Evil(tm).

(The use case you're going for, which is using a LayerMetricsWrapper object in the test-expression of a for loop, will still work.)

@@ +182,5 @@
> +  FrameMetrics Metrics() const
> +  {
> +    MOZ_ASSERT(IsValid());
> +
> +    if (mIndex >= mLayer->GetFrameMetricsCount()) {

Should this ever be called under these conditions? If not, perhaps we should assert instead. (As a bonus, the function can then return by reference-to-const).

@@ +192,5 @@
> +  AsyncPanZoomController* Apzc() const
> +  {
> +    MOZ_ASSERT(IsValid());
> +
> +    if (mIndex >= mLayer->GetFrameMetricsCount()) {

See comment about Metrics().

If, however, 'nullptr' is in fact a valid return value, the function should be called GetApzc().

@@ +195,5 @@
> +
> +    if (mIndex >= mLayer->GetFrameMetricsCount()) {
> +      return nullptr;
> +    }
> +    return mLayer->GetAsyncPanZoomController(mIndex);

Should we assert GetFrameMetrics(mIndex).IsScrollable()?

@@ +257,5 @@
> +  const nsIntRect* GetClipRect() const
> +  {
> +    MOZ_ASSERT(IsValid());
> +
> +    return mLayer->GetClipRect();

Just to make sure I understand this right - is the reason we apply the transform in GetVisibleRegion() (if we're not AtBottomLayer()), but not here, that the clip is applied after the transform, but the visible region is a pre-transform region?

@@ +270,5 @@
> +
> +  // Expose an opaque pointer to the layer. Mostly used for printf
> +  // purposes. This is not intended to be a general-purpose accessor
> +  // for the underlying layer.
> +  void* GetLayer() const

Might as well make it const void*.

	Botond Ballo [:botond]
	Comment 59 • 10 years ago

Comment on attachment 8479274 [details] [diff] [review]
Part 5 - Update tiling code to use LayerMetricsWrapper

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

::: gfx/layers/LayerMetricsWrapper.h
@@ +120,5 @@
>   * to cast to uint32_t all over the place.
>   */
>  class MOZ_STACK_CLASS LayerMetricsWrapper {
>  public:
> +  enum StartAt {

Please use MOZ_BEGIN_NESTED_ENUM_CLASS, especially as we're overloading the constructor on StartAt vs. uint32_t.

@@ +181,5 @@
> +    if (!AtTopLayer()) {
> +      return LayerMetricsWrapper(mLayer, mIndex + 1);
> +    }
> +    if (mLayer->GetParent()) {
> +      return LayerMetricsWrapper((Layer*)mLayer->GetParent(), BOTTOM);

You don't need to cast from derived to base explicitly.

@@ +350,5 @@
> +  {
> +    return !(*this == aOther);
> +  }
> +
> +  static bool HasScrollableMetrics(Layer* aLayer)

Any reason this isn't a method of Layer?

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

(In reply to Botond Ballo [:botond] from comment #57)
> I think it would enhance the readability of this code (particularly in
> conjunction with the comment below) if this were called isFixedOrSticky.

Done.

> Please move the last sentence inside the outer 'if', as that pertains to why
> we are skipping scrollable layers.

Done.

(In reply to Botond Ballo [:botond] from comment #58)
> I'd remove "and in case ... actually required". Let's not encourage people
> to cast void*'s to things :)

Done.

> Please add MOZ_EXPLICIT_CONVERSION in front of this. Classes with implicit
> conversions to bool are Evil(tm).

There's a few other places I use the implicit casting feature. I suppose I can make it explicit in those places but I might as well call IsValid() then for clarity.

> @@ +182,5 @@
> > +  FrameMetrics Metrics() const
> > +  {
> > +    MOZ_ASSERT(IsValid());
> > +
> > +    if (mIndex >= mLayer->GetFrameMetricsCount()) {
> 
> Should this ever be called under these conditions? If not, perhaps we should
> assert instead. (As a bonus, the function can then return by
> reference-to-const).

Yes, it might be if GetFrameMetricsCount() == mIndex == 0.

> @@ +192,5 @@
> See comment about Metrics().
> 
> If, however, 'nullptr' is in fact a valid return value, the function should
> be called GetApzc().

Yeah it is a valid return value, I'll rename the function.

> > +    return mLayer->GetAsyncPanZoomController(mIndex);
> 
> Should we assert GetFrameMetrics(mIndex).IsScrollable()?

It's valid to call this function with an mIndex that points to a non-scrollable metrics. It's just that should always return null.

> @@ +257,5 @@
> Just to make sure I understand this right - is the reason we apply the
> transform in GetVisibleRegion() (if we're not AtBottomLayer()), but not
> here, that the clip is applied after the transform, but the visible region
> is a pre-transform region?

Yup, exactly.

> @@ +270,5 @@
> > +  void* GetLayer() const
> 
> Might as well make it const void*.

Done.

(In reply to Botond Ballo [:botond] from comment #59)
> Please use MOZ_BEGIN_NESTED_ENUM_CLASS, especially as we're overloading the
> constructor on StartAt vs. uint32_t.

Done.

> @@ +181,5 @@
> > +      return LayerMetricsWrapper((Layer*)mLayer->GetParent(), BOTTOM);
> 
> You don't need to cast from derived to base explicitly.

Whoops, fixed. Left over from a previous version of the code.

> @@ +350,5 @@
> > +  static bool HasScrollableMetrics(Layer* aLayer)
> 
> Any reason this isn't a method of Layer?

Not really, I moved it.

	Benoit Girard (:BenWa)
	Comment 62 • 10 years ago

Comment on attachment 8479274 [details] [diff] [review]
Part 5 - Update tiling code to use LayerMetricsWrapper

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

::: gfx/layers/LayerMetricsWrapper.h
@@ +131,5 @@
>      , mIndex(0)
>    {
>    }
>  
> +  explicit LayerMetricsWrapper(Layer* aRoot, StartAt aStart = StartAt::TOP)

Optional if it causes too much rot to fix something minor:
I don't really think the StartAt is something that belongs in the constructor. Omitting it doesn't leave the object in an inconsistent state. Looks like the other constructor also has that. Perhaps we should have a default position and have a separate helper call or let the user do it.

::: gfx/layers/Layers.cpp
@@ +57,1 @@
>  LayerManager::GetPrimaryScrollableLayer()

Unrelated to this patch (so no need to fix it here) but I think this makes even less sense with all these changes. For instance you might have two sibling thebes layer with the same scrollid.

	Benoit Girard (:BenWa)
	Comment 63 • 10 years ago

Comment on attachment 8479281 [details] [diff] [review]
Part 7 - Various other bits of compositor code

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

r-: I'd like to check the correction.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +256,5 @@
>    // with the background color by drawing a rectangle of the background color
>    // over the entire composited area before drawing the container contents.
> +  // Make sure not to do this on a "scrollinfo" layer (one with an empty visible
> +  // region) because it's just a placeholder for APZ purposes.
> +  if (LayerMetricsWrapper::HasScrollableMetrics(aContainer) && !aContainer->GetVisibleRegion().IsEmpty()) {

I don't think it's correct to have that 'a container layer is a scrollinfo layer iff it's has an empty region'. I think we should at the very least for now use IsScrollInfoLayer() so it's obvious what parts of the code have this assumption. I would rather that we have a better signal however for a scrollinfo layer.

I'm concerned because a culled container layer with possibly valid content can have an empty visible region.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +800,5 @@
> +  FrameMetrics rootMetrics = LayerMetricsWrapper::TopmostScrollableMetrics(root);
> +  if (!rootMetrics.IsScrollable()) {
> +    // The root may not have any scrollable metrics, in which case rootMetrics
> +    // will just be an empty FrameMetrics. Instead use the actual metrics from
> +    // the root layer.

We jump from the topmost to the bottom? It leaves the reader wondering why we're skip the middle FM?

	Benoit Girard (:BenWa)
	Comment 64 • 10 years ago

Comment on attachment 8479283 [details] [diff] [review]
Part 8 - Update frame uniformity code to use shadow transform

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

This significantly changes the behavior which will impact how the user of the code see it. Instead of seeing the scrolling data it will see the transform data. There's trade offs here (like we talked on IRC). If mchang can't weight in on time I say this will do for now and we can fix it in bug 1058884.

	Mason Chang [Inactive] [:mchang]
	Comment 65 • 10 years ago

Comment on attachment 8479283 [details] [diff] [review]
Part 8 - Update frame uniformity code to use shadow transform

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

LGTM

	Botond Ballo [:botond]
	Comment 66 • 10 years ago

Comment on attachment 8479267 [details] [diff] [review]
Part 3 - Modify Layers API

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

::: gfx/layers/Layers.h
@@ +1489,5 @@
>    // and can be used anytime.
> +  // A layer has an APZC at index aIndex only-if GetFrameMetrics(aIndex).IsScrollable().
> +  // The aIndex for these functions must be less than GetFrameMetricsCount().
> +  void SetAsyncPanZoomController(uint32_t aIndex, AsyncPanZoomController *controller);
> +  AsyncPanZoomController* GetAsyncPanZoomController(uint32_t aIndex) const;

I would mention explicitly that it's valid to call this function with an aIndex such that !GetFrameMetrics(aIndex).IsScrollable(), and in such a case, nullptr is returned.

	Botond Ballo [:botond]
	Comment 67 • 10 years ago

Comment on attachment 8479269 [details] [diff] [review]
Part 4 - Update APZCTreeManager code and add a LayerMetricsWrapper class

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

::: gfx/layers/LayerMetricsWrapper.h
@@ +183,5 @@
> +  {
> +    MOZ_ASSERT(IsValid());
> +
> +    if (mIndex >= mLayer->GetFrameMetricsCount()) {
> +      return FrameMetrics();

It might make sense to have a 'static const FrameMetrics LayerMetricsWrapper::sNullMetrics' (or FrameMetrics::sNullMetrics, or wherever), and return either that or GetFrameMetrics(mIndex) by reference-to-const. I don't feel strongly about this, though.

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

I can fold this into previous patches if needed but leaving it on top for easier review right now.

	Botond Ballo [:botond]
	Comment 70 • 10 years ago

Comment on attachment 8479276 [details] [diff] [review]
Part 6a - AsyncCompositionManager transform code updates

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

::: gfx/layers/LayerMetricsWrapper.h
@@ +362,5 @@
>    }
>  
> +  static FrameMetrics TopmostScrollableMetrics(Layer* aLayer)
> +  {
> +    for (uint32_t i = aLayer->GetFrameMetricsCount(); i-- > 0;) {

I find the 'i-- > 0' hard to read. I would move the 'i--' into the update-expression, and use 'i - 1' in the loop body.

@@ +387,5 @@
> +      return aLayer->GetFrameMetrics(0);
> +    }
> +    return FrameMetrics();
> +  }
> +

Consider making these methods of Layer as well. Also, if you introduce the sNullMetrics, have these return by reference-to-const as well.

	Botond Ballo [:botond]
	Comment 74 • 10 years ago

Comment on attachment 8479277 [details] [diff] [review]
Part 6b - AsyncCompositionManager scrollbar code updates

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +630,5 @@
>  
>      appliedTransform = true;
>    }
>  
> +  if (aLayer->GetScrollbarDirection() != Layer::NONE) {

Is the scrollbar not having to be a ContainerLayer related to multi-layer-apz?

@@ +666,5 @@
>  
>  static void
> +ApplyAsyncTransformToScrollbarForContent(Layer* aScrollbar,
> +                                         const LayerMetricsWrapper& aContent,
> +                                         bool aScrollbarIsChild)

Unrelated to this change, but sounds like this parameter should be named aScrollBarIsDescendant.

@@ +673,5 @@
>    // children (i.e. when it has some possibly-visible content). This is to
>    // avoid moving scroll-bars in the situation that only a scroll information
>    // layer has been built for a scroll frame, as this would result in a
>    // disparity between scrollbars and visible content.
> +  if (!LayerHasNonContainerDescendants(aContent)) {

Given the Part 12c patch, do we want to introduce LayerMetricsWrapper::IsScrollInfoLayer() (which forwards to Layer::IsScrollInfoLayer()), and replace this check with that?

	Botond Ballo [:botond]
	Comment 75 • 10 years ago

Comment on attachment 8479267 [details] [diff] [review]
Part 3 - Modify Layers API

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

::: gfx/layers/Layers.h
@@ +1186,5 @@
>    uint32_t GetContentFlags() { return mContentFlags; }
>    const nsIntRegion& GetVisibleRegion() const { return mVisibleRegion; }
> +  const FrameMetrics& GetFrameMetrics(uint32_t aIndex) const;
> +  uint32_t GetFrameMetricsCount() const { return mFrameMetrics.Length(); }
> +  nsTArray<FrameMetrics>& GetAllFrameMetrics() { return mFrameMetrics; }

Is there any reason this returns by reference-to-nonconst? The only usage I see in the patch set is the one in EndTransaction(), and returning by reference-to-const suffices for that.

	Botond Ballo [:botond]
	Comment 76 • 10 years ago

Comment on attachment 8479279 [details] [diff] [review]
Part 6c - AsyncCompositionManager fixed/sticky layer code updates

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +626,5 @@
>      // parameter. This ensures that the overscroll transform is not unapplied,
>      // and therefore that the visual effect applies to fixed and sticky layers.
>      Matrix4x4 transformWithoutOverscroll = AdjustAndCombineWithCSSTransform(
>          combinedAsyncTransformWithoutOverscroll, aLayer);
> +    AlignFixedAndStickyLayers(aLayer, aLayer, bottom.GetScrollId(), oldTransform,

Let's add a comment saying why it's appropriate to use the bottom-most metrics' scroll id here.

For reference, some relevant IRC discussion:

[14:08] <kats> botond: we only care about the most recent scrolling ancestor to the sticky layer
[14:09] <kats> botond: which is the bottommost scrollable metrics on the scrollable layer
...
[14:10] <botond> kats: the scenario i'm wondering about is where layout gives us a layer with say 3 scrollable framemetrics, with ids (top-to-bottom) of 1, 2, and 3
[14:10] <botond> kats: and then some sticky layer with GetStickyScrollContainerId() of 2
[14:10] <botond> kats: are you saying that will never happen?
[14:11] <kats> botond: with the old code, the sticky layer would have to be a descendant of 2 but not of 3 for it to work
[14:11] <kats> and that can't happen if 1, 2, and 3 are on the same layer
[14:12] <kats> botond: so basically yeah, that should never happen. if it does then it was probably broken in the old code too

	Botond Ballo [:botond]
	Comment 77 • 10 years ago

Comment on attachment 8479280 [details] [diff] [review]
Part 6d - AsyncCompositionManager animation sampling code updates

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +968,5 @@
>    // find a frame that is async scrollable.  Note that the fallback
>    // code also includes Fennec which is rendered async.  Fennec uses
>    // its own platform-specific async rendering that is done partially
>    // in Gecko and partially in Java.
> +  wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root, LayerMetricsWrapper::TOP), aCurrentFrame);

TOP is the default, right?

	Benoit Girard (:BenWa)
	Comment 78 • 10 years ago

Comment on attachment 8480011 [details] [diff] [review]
Part 12e - Fix a rather large omission to store multiple APZCs

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

::: gfx/layers/Layers.cpp
@@ +474,5 @@
>  void
>  Layer::SetAsyncPanZoomController(uint32_t aIndex, AsyncPanZoomController *controller)
>  {
>    MOZ_ASSERT(aIndex < GetFrameMetricsCount());
> +  mApzcs[aIndex] = controller;

One nit that I have is that FM are manipulated in one part of the code and the APZC are set elsewhere. You have two 'associative' arrays here where their index must match. It feels too easy for the code that modifies the FM the second time around to modify the FM and break the associative match between the two arrays leaving the state inconsistent.

::: gfx/layers/Layers.h
@@ +865,5 @@
>    {
>      if (mFrameMetrics != aMetricsArray) {
>        MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) FrameMetrics", this));
>        mFrameMetrics = aMetricsArray;
> +      FrameMetricsChanged();

For instance when you assigned a new FM array the scrollid may no longer associate with the APZC.

	Benoit Girard (:BenWa)
	Comment 79 • 10 years ago

Comment on attachment 8480005 [details] [diff] [review]
Part 12c - Fix check for scrollinfo layer (comment 63)

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

r+, but I would of though we had code conditional on ScrollInfo layer that could of been updated.

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

(In reply to Botond Ballo [:botond] from comment #66)
> I would mention explicitly that it's valid to call this function with an
> aIndex such that !GetFrameMetrics(aIndex).IsScrollable(), and in such a
> case, nullptr is returned.

I did this in part 12e.

(In reply to Botond Ballo [:botond] from comment #67)
> It might make sense to have a 'static const FrameMetrics
> LayerMetricsWrapper::sNullMetrics' (or FrameMetrics::sNullMetrics, or
> wherever), and return either that or GetFrameMetrics(mIndex) by
> reference-to-const. I don't feel strongly about this, though.

Yeah I was debating whether or not to do this at one point. It makes sense, I'll add a FrameMetrics::sNullMetrics and update the return values to be ref-to-const.

(In reply to Botond Ballo [:botond] from comment #70)
> I find the 'i-- > 0' hard to read. I would move the 'i--' into the
> update-expression, and use 'i - 1' in the loop body.

Ok, will do.

> @@ +387,5 @@
> Consider making these methods of Layer as well. Also, if you introduce the
> sNullMetrics, have these return by reference-to-const as well.

Yup, will do.

(In reply to Botond Ballo [:botond] from comment #74)
> > +  if (aLayer->GetScrollbarDirection() != Layer::NONE) {
> 
> Is the scrollbar not having to be a ContainerLayer related to
> multi-layer-apz?

Not really. I think it will still be a ContainerLayer, but since GetScrollbarDirection() exists on Layer, and we don't actually need it as a ContainerLayer* anywhere, it didn't seem worth it to do the whole AsContainerLayer() business.

> @@ +666,5 @@
> Unrelated to this change, but sounds like this parameter should be named
> aScrollBarIsDescendant.

True, I'll change it.

> @@ +673,5 @@
> > +  if (!LayerHasNonContainerDescendants(aContent)) {
> 
> Given the Part 12c patch, do we want to introduce
> LayerMetricsWrapper::IsScrollInfoLayer() (which forwards to
> Layer::IsScrollInfoLayer()), and replace this check with that?

I'm not sure they're equivalent. This function checks the entire descendant subtree whereas IsScrollInfoLayer just checks for direct children. I suppose I could probably write one in terms of the other but it seems like it wouldn't be that clear to use a function called IsScrollInfoLayer in this context.

(In reply to Botond Ballo [:botond] from comment #75)
> Is there any reason this returns by reference-to-nonconst? The only usage I
> see in the patch set is the one in EndTransaction(), and returning by
> reference-to-const suffices for that.

Not that I can recall, will fix.

(In reply to Botond Ballo [:botond] from comment #76)
> > +    AlignFixedAndStickyLayers(aLayer, aLayer, bottom.GetScrollId(), oldTransform,
> 
> Let's add a comment saying why it's appropriate to use the bottom-most
> metrics' scroll id here.

Will do.

(In reply to Botond Ballo [:botond] from comment #77)
> > +  wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root, LayerMetricsWrapper::TOP), aCurrentFrame);
> 
> TOP is the default, right?

Yeah. At one point I was considering not giving it a default value; this is probably left over from that. I'll remove it.

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

(In reply to Benoit Girard (:BenWa) from comment #78)
> >    MOZ_ASSERT(aIndex < GetFrameMetricsCount());
> > +  mApzcs[aIndex] = controller;
> 
> One nit that I have is that FM are manipulated in one part of the code and
> the APZC are set elsewhere. You have two 'associative' arrays here where
> their index must match. It feels too easy for the code that modifies the FM
> the second time around to modify the FM and break the associative match
> between the two arrays leaving the state inconsistent.

Their indices don't have to match at all times. Right after a transaction, the FrameMetrics will have been updated with the new version from the client side, but the APZCs will still be the old ones, and the indices might be wrong/out-of-date. Only after the APZCTreeManager::UpdatePanZoomControllerTree code runs will they get re-synced.

This used to be the case with the old code as well; the mFrameMetrics on the layer would get updated and the mAPZC would only get updated during the UpdatePanZoomControllerTree function which happened layer. The code in UpdatePZCTree is robust enough to handle this.

The only thing UpdatePZCTree assumes is that the APZC array in the layer is the same length as the FM array, because it tries to set APZCs at indices matching the scrollable FMs. The assumption also means it doesn't null out APZCs beyond GetFrameMetricsCount() so if I didn't resize the array in SetFrameMetrics then those nsRefPtrs<AsyncPanZoomController>s would never get cleared and they would leak.

> >      if (mFrameMetrics != aMetricsArray) {
> >        MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) FrameMetrics", this));
> >        mFrameMetrics = aMetricsArray;
> > +      FrameMetricsChanged();
> 
> For instance when you assigned a new FM array the scrollid may no longer
> associate with the APZC.

Yup, as mentioned above this is fine. With the old code this happened as well, the mAPZC gets updated indpendently of the mFrameMetrics.

	Benoit Girard (:BenWa)
	Comment 82 • 10 years ago

Comment on attachment 8480011 [details] [diff] [review]
Part 12e - Fix a rather large omission to store multiple APZCs

This isn't great but it's left inconsistent during the same event loop event only. Kats say he has a plan for cleaning this up later.

It's not easy to add a debug assert without messing with how the apzc are recycled.

	Botond Ballo [:botond]
	Comment 83 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #80)
> > @@ +673,5 @@
> > > +  if (!LayerHasNonContainerDescendants(aContent)) {
> > 
> > Given the Part 12c patch, do we want to introduce
> > LayerMetricsWrapper::IsScrollInfoLayer() (which forwards to
> > Layer::IsScrollInfoLayer()), and replace this check with that?
> 
> I'm not sure they're equivalent. This function checks the entire descendant
> subtree whereas IsScrollInfoLayer just checks for direct children. I suppose
> I could probably write one in terms of the other but it seems like it
> wouldn't be that clear to use a function called IsScrollInfoLayer in this
> context.

The comment here suggests that the purpose of this check is to avoid transforming scrollbars for scroll-info layers. Is there a case where the layer is not a scroll-info layer, but we still don't want to draw the scrollbars?

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

Hm, good point; for some reason I didn't read that comment. I'll see if I can rewrite it to use IsScrollInfoLayer instead.

	Botond Ballo [:botond]
	Comment 85 • 10 years ago

Comment on attachment 8480011 [details] [diff] [review]
Part 12e - Fix a rather large omission to store multiple APZCs

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

::: gfx/layers/Layers.cpp
@@ +481,5 @@
>  AsyncPanZoomController*
>  Layer::GetAsyncPanZoomController(uint32_t aIndex) const
>  {
>    MOZ_ASSERT(aIndex < GetFrameMetricsCount());
>  #ifdef DEBUG

What's the purpose of the '#ifdef DEBUG' here? MOZ_ASSERT is already conditional on DEBUG.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 86 • 10 years ago
orange try

I addressed all the above review comments and squashed the fixes into their respective patches. The only one I left in a patch of its own was the GetPrimaryScrollableLayer change. And I rebased the whole set on master. Here's a try push:

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

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #84)
> Hm, good point; for some reason I didn't read that comment. I'll see if I
> can rewrite it to use IsScrollInfoLayer instead.

I put a IsScrollInfoLayer() on LayerMetricsWrapper and used that, although I couldn't make it just forward the call to mLayer->IsScrollInfoLayer. I implemented it directly like this: https://hg.mozilla.org/try/rev/757cdf36bc41#l1.12

The reason is that if you have a scroll info layer with FM[0], FM[1] where FM[1] is scrollable and FM[0] is not, then the LayerMetricsWrapper for FM[0] needs to return false while Layer::IsScrollInfoLayer needs to return true.

Still, adding IsScrollInfoLayer on LayerMetricsWrapper made the code a lot cleaner so it was still worth it.

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

Looks like Windows doesn't like the strongly-typed enum. It's using the wrong constructor I think.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 89 • 10 years ago
green try

https://tbpl.mozilla.org/?tree=Try&rev=331c534b0884 looks better

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

MOZ_BEGIN_NESTED_ENUM_CLASS, you have failed me for the last time!

I rolled it back to "enum" and landed on b2g-inbound:

https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=4a7c80b9ae9c

	Botond Ballo [:botond]
	Comment 91 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #88)
> Looks like Windows doesn't like the strongly-typed enum. It's using the
> wrong constructor I think.

Ironic, that a change I asked for out of concern that we might call the wrong constructor, caused us to call the wrong constructor :/

I'm going to stop asking in reviews for class-scope enums to be defined using MOZ_BEGIN_NESTED_ENUM_CLASS until we have real 'enum class' support (which, given the activity around upgrading our GCC and MSVC toolchains, hopefully isn't very far off).

	Carsten Book [:Tomcat]
	Comment 92 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/33bbbab21337
https://hg.mozilla.org/mozilla-central/rev/33fad6086b32
https://hg.mozilla.org/mozilla-central/rev/933f499c2586
https://hg.mozilla.org/mozilla-central/rev/2b593e85dbdb
https://hg.mozilla.org/mozilla-central/rev/dae1f96f88af
https://hg.mozilla.org/mozilla-central/rev/97d5ad0a7743
https://hg.mozilla.org/mozilla-central/rev/c017465be449
https://hg.mozilla.org/mozilla-central/rev/434f551aae4d
https://hg.mozilla.org/mozilla-central/rev/d6f6214621ef
https://hg.mozilla.org/mozilla-central/rev/ff3695004803
https://hg.mozilla.org/mozilla-central/rev/8e218c04840d
https://hg.mozilla.org/mozilla-central/rev/d331d1b70652
https://hg.mozilla.org/mozilla-central/rev/504e6547f98c
https://hg.mozilla.org/mozilla-central/rev/986b2273be29
https://hg.mozilla.org/mozilla-central/rev/4a1907885d78
https://hg.mozilla.org/mozilla-central/rev/4a7c80b9ae9c