Allow storing multiple FrameMetrics and APZC instances per layer

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla34
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(22 attachments, 21 obsolete attachments)

24.48 KB, patch
botond
: review+
Details | Diff | Splinter Review
11.20 KB, patch
Details | Diff | Splinter Review
4.05 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.57 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.87 KB, patch
BenWa
: review+
roc
: review+
Details | Diff | Splinter Review
24.97 KB, patch
BenWa
: review+
botond
: review+
Details | Diff | Splinter Review
31.37 KB, patch
botond
: review+
BenWa
: review+
Details | Diff | Splinter Review
7.78 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.45 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.65 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.63 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.63 KB, patch
BenWa
: review-
botond
: review+
Details | Diff | Splinter Review
2.00 KB, patch
BenWa
: review+
mchang
: review+
Details | Diff | Splinter Review
5.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.52 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.58 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
1.22 KB, patch
Details | Diff | Splinter Review
25.02 KB, patch
Details | Diff | Splinter Review
11.92 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
3.59 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
1.14 KB, patch
Details | Diff | Splinter Review
4.40 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
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...).
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.
Attachment #8476887 - Flags: feedback?(roc)
Attachment #8476887 - Flags: feedback?(matt.woodrow)
Attachment #8475978 - Attachment is obsolete: true
Attachment #8475979 - Attachment is obsolete: true
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.
Attachment #8476889 - Flags: feedback?(roc)
Attachment #8476889 - Flags: feedback?(matt.woodrow)
Attachment #8476889 - Flags: feedback?(bgirard)
Propagating the LayerMetricsWrapper into more places that use the FrameMetrics.
Attachment #8476890 - Flags: feedback?(roc)
Attachment #8476890 - Flags: feedback?(matt.woodrow)
Attachment #8476890 - Flags: feedback?(bgirard)
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.
Attachment #8476889 - Flags: feedback?(roc) → feedback+
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?
Attachment #8476890 - Flags: feedback?(roc) → feedback-
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.
Put it in a separate header file, that's OK.
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.
Attachment #8476887 - Flags: feedback?(bgirard) → feedback+
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().
Attachment #8476889 - Flags: feedback?(bgirard) → feedback+
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.
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.
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. :(
Bisection narrowed down the reftest failure to part 6 (cset b1bf7a5da754 in the try push from comment 18).
Posted patch Part 3 - Modify Layers API (obsolete) — Splinter Review
This still needs more documentation
Attachment #8476887 - Attachment is obsolete: true
Attachment #8476887 - Flags: feedback?(matt.woodrow)
This also needs more documentation
Attachment #8476889 - Attachment is obsolete: true
Attachment #8476889 - Flags: feedback?(matt.woodrow)
Attachment #8476890 - Attachment is obsolete: true
Attachment #8476890 - Flags: feedback?(matt.woodrow)
Attachment #8476890 - Flags: feedback?(bgirard)
Try push that should have everything good (fingers crossed): https://tbpl.mozilla.org/?tree=Try&rev=5885415783de
Attachment #8479142 - Attachment is obsolete: true
Attachment #8479267 - Flags: review?(roc)
Attachment #8479267 - Flags: review?(bgirard)
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.
Attachment #8479153 - Attachment is obsolete: true
Attachment #8479281 - Flags: review?(botond)
Attachment #8479281 - Flags: review?(bgirard)
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.
Attachment #8479155 - Attachment is obsolete: true
Attachment #8479285 - Flags: review?(roc)
Yeah this is kinda ugly. I'll file a follow-up to do this more elegantly.
Attachment #8479156 - Attachment is obsolete: true
Attachment #8479286 - Flags: review?(botond)
Attachment #8479138 - Flags: review?(botond) → review+
(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.
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) { ... }'
Attachment #8479141 - Flags: review+
Comment on attachment 8479140 [details] [diff] [review]
Part 2 - Rearrange some code (no functional changes)

See r+ in whitespace-ignoring patch.
Attachment #8479140 - Flags: review?(botond)
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"?
Attachment #8479266 - Flags: review?(botond) → review+
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.
Attachment #8479285 - Flags: review?(roc) → review+
(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.
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.
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*.
Attachment #8479269 - Flags: review?(botond) → review+
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?
Attachment #8479274 - Flags: review?(botond) → review+
(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.
Attachment #8479885 - Attachment description: Address review comments so far → Part 12 - Address review comments so far
Attachment #8479267 - Flags: review?(bgirard) → review+
Attachment #8479269 - Flags: review?(bgirard) → review+
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.
Attachment #8479274 - Flags: review?(bgirard) → review+
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?
Attachment #8479281 - Flags: review?(bgirard) → review-
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.
Attachment #8479283 - Flags: review?(mchang)
Attachment #8479283 - Flags: review?(bgirard)
Attachment #8479283 - Flags: review+
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
Attachment #8479287 - Flags: review?(bgirard) → review+
Attachment #8479283 - Flags: review?(mchang) → review+
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.
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.
I can fold this into previous patches if needed but leaving it on top for easier review right now.
Attachment #8480003 - Flags: review?(bgirard)
Attachment #8480004 - Attachment is obsolete: true
Attachment #8480004 - Flags: review?(bgirard)
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.
Attachment #8479276 - Flags: review?(botond) → review+
Attachment #8480003 - Attachment description: Part 12b - Address BenWa's comment about GetPrimaryScrollableLayer → Part 12b - Address BenWa's comment about GetPrimaryScrollableLayer (comment 62)
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?
Attachment #8479277 - Flags: review?(botond) → review+
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.
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
Attachment #8479279 - Flags: review?(botond) → review+
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?
Attachment #8479280 - Flags: review?(botond) → review+
Attachment #8479281 - Flags: review?(botond) → review+
Attachment #8480003 - Flags: review?(bgirard) → review+
Attachment #8479286 - Flags: review?(botond) → review+
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.
Attachment #8480011 - Flags: review?(bgirard) → review-
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.
Attachment #8480005 - Flags: review?(bgirard) → review+
(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.
(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.
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.
Attachment #8480011 - Flags: review- → review+
(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?
Hm, good point; for some reason I didn't read that comment. I'll see if I can rewrite it to use IsScrollInfoLayer instead.
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.
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
(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.
Looks like Windows doesn't like the strongly-typed enum. It's using the wrong constructor I think.
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
(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).
Depends on: 1109315
You need to log in before you can comment on or make changes to this bug.