Thinker Li [:sinker] Assignee
	Comment 1 • 10 years ago

For unknown reason, Windows XP is totally broken.  But, it works well for other platforms.  Some test cases need to be fixed for the changes of AA and residual translation.

	Thinker Li [:sinker] Assignee
	Comment 2 • 10 years ago

Fix errors for d3d9, and hit test.

Have passed all reftests.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d1528d94d39

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

Comment on attachment 8548121 [details] [diff] [review]
wip-v2

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

If we can split out the layer-system changes into their own patch, that would be ideal. But we should only do that if tests pass with the layer-system changes and without the layout changes.

::: gfx/layers/Layers.cpp
@@ +557,5 @@
> +        !matrix2D.HasNonTranslation() &&
> +        matrix2D.HasNonIntegerTranslation()) ||
> +       !aTransform.IsSingular())) {
> +    if (!is2d) {
> +      // not signular

"not singular"

I don't understand this change. can't the matrix be is2D *and* singular?

Some comments explaining what you're trying to do here would be very helpful.

I think this would be easier to understand if you did something like
  if (mManager->IsSnappingEffectiveTransforms()) { 
    if (is2d) {
      ...
    } else {
    }
  }

::: gfx/layers/Layers.h
@@ -1710,5 @@
>      if (!gfx::ThebesPoint(residual.GetTranslation()).WithinEpsilonOf(mResidualTranslation, 1e-3f)) {
>        mResidualTranslation = gfx::ThebesPoint(residual.GetTranslation());
> -      NS_ASSERTION(-0.5 <= mResidualTranslation.x && mResidualTranslation.x < 0.5 &&
> -                   -0.5 <= mResidualTranslation.y && mResidualTranslation.y < 0.5,
> -                   "Residual translation out of range");

Why did you have to remove this?

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +199,5 @@
>  
> +  float GetEffectiveOpacity() {
> +    ContainerLayer *parent = mLayer->GetParent();
> +    if (parent &&
> +        mLayer->AsContainerLayer() == nullptr &&

!mLayer->AsContainingLayer()

@@ +202,5 @@
> +    if (parent &&
> +        mLayer->AsContainerLayer() == nullptr &&
> +        parent->IsPreserves3D()) {
> +      // Since the layer would apply the effective opacity for
> +      // it-self, BasicLayerManager does not need to do for it.

"itself"

I don't understand this comment, or why you need to make this change.

@@ +789,5 @@
>  
>      CompositionOp op = GetEffectiveOperator(aPaintContext.mLayer);
>      AutoSetOperator setOperator(aPaintContext.mTarget, ThebesOp(op));
>  
> +    PaintWithMask(aPaintContext.mTarget, aPaintContext.GetEffectiveOpacity(),

Could this change be moved to its own patch?

I'd like to find ways to break this patch up into smaller pieces...

::: gfx/layers/composite/ColorLayerComposite.cpp
@@ +44,5 @@
> +  gfx::Matrix4x4 transform = GetEffectiveTransform();
> +  transform._13 = 0;
> +  transform._23 = 0;
> +  transform._33 = 1;
> +  transform._43 = 0;

Put this in its own patch

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +149,5 @@
>  
>    for (uint32_t i = 0; i < children.Length(); i++) {
>      LayerComposite* layerToRender = static_cast<LayerComposite*>(children.ElementAt(i)->ImplData());
> +    RenderTargetIntRect clipRect;
> +    clipRect = layerToRender->GetLayer()->CalculateScissorRect(aClipRect);

Make this
  RenderTargetIntRect clipRect =
     layerToRender->GetLayer()->CalculateScissorRect(aClipRect);

@@ +226,4 @@
>        aManager->GetCompositor()->DrawQuad(gfx::Rect(layerBounds.x, layerBounds.y, layerBounds.width, layerBounds.height),
>                                            gfx::Rect(clipRect.ToUnknownRect()),
>                                            effectChain, layer->GetEffectiveOpacity(),
> +                                          transform);

Take this out of the main patch too. You could put all these compositor transform fixes in a single patch.

::: layout/base/nsDisplayList.cpp
@@ +1571,5 @@
>      bool snap;
>      nsRect r = item->GetBounds(aBuilder, &snap).Intersect(aRect);
> +    bool alwaysIntersect =
> +      item->GetType() == nsDisplayItem::TYPE_TRANSFORM &&
> +      item->Frame()->Preserves3D();

Test item->Frame()->Preserve3D() first since it doesnt require a virtual call AFAIK.

@@ +3188,5 @@
>  {
>    MOZ_COUNT_CTOR(nsDisplayWrapList);
>  
>    mList.AppendToTop(aItem);
> +  if (!mFrame->Preserves3D() && !mFrame->Preserves3DChildren()) {

Move this check into UpdateBounds?

@@ +3221,5 @@
>  nsRect
>  nsDisplayWrapList::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap) {
> +  if (mFrame->Preserves3D() || mFrame->Preserves3DChildren()) {
> +    return mList.GetBounds(aBuilder);
> +  }

Why have you made this change?

::: layout/base/nsDisplayList.h
@@ +116,5 @@
>   * available from the prescontext/presshell, but we copy them into the builder
>   * for faster/more convenient access.
>   */
>  class nsDisplayListBuilder {
> +  class Preserves3DContext {

Add a comment explaining what this is for.

@@ +658,5 @@
>    };
>  
> +  class AutoAccumulateTransform;
> +  friend class AutoAccumulateTransform;
> +  class AutoAccumulateTransform {

And add a comment explaining what this is for.

@@ +690,5 @@
> +  };
> +
> +  class AutoAccumulateRect;
> +  friend class AutoAccumulateRect;
> +  class AutoAccumulateRect {

And this.

@@ +820,5 @@
>    bool IsInWillChangeBudget(nsIFrame* aFrame) const;
>  
> +  class AutoPreserves3DContext;
> +  friend class AutoPreserves3DContext;
> +  class AutoPreserves3DContext {

And this.

@@ +834,5 @@
> +    nsDisplayListBuilder *mBuilder;
> +    Preserves3DContext mSavedCtx;
> +  };
> +
> +  const nsRect GetPreserves3DDirtyRect(const nsIFrame *aFrame) const {

And this.

Don't return "const nsRect". Just return a regular nsRect.

@@ +3469,5 @@
> +  /**
> +   * Return the transform that is aggregation of all transform ons the
> +   * preserves3d chain.
> +   */
> +  const Matrix4x4& GetTransformP3D();

Call this GetAccumulatedPreserved3DTransform?

::: layout/generic/nsFrame.cpp
@@ +2228,5 @@
> +    || nsSVGIntegrationUtils::UsingEffectsForFrame(child)
> +    // In the stacking context, force it to generate a transform item
> +    // for checking backface.
> +    || (child->Preserves3D()
> +        && disp->mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN);

I don't think it's correct for backface-visibility to generate a stacking context. See bug 968555.

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

Also, have you tested this with async-animations of preserve-3d transforms?

	Thinker Li [:sinker] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8548121 [details] [diff] [review]
wip-v2

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

::: gfx/layers/Layers.h
@@ -1710,5 @@
>      if (!gfx::ThebesPoint(residual.GetTranslation()).WithinEpsilonOf(mResidualTranslation, 1e-3f)) {
>        mResidualTranslation = gfx::ThebesPoint(residual.GetTranslation());
> -      NS_ASSERTION(-0.5 <= mResidualTranslation.x && mResidualTranslation.x < 0.5 &&
> -                   -0.5 <= mResidualTranslation.y && mResidualTranslation.y < 0.5,
> -                   "Residual translation out of range");

The change of SnapTransformTranslation() allow to do snapping for any 3d transform without singluar, this assertion is not always true.

I am not sure if I should do it here, although it looks to get better rendering result.  Maybe I should put it in a separated patch.

::: layout/base/nsDisplayList.cpp
@@ +3188,5 @@
>  {
>    MOZ_COUNT_CTOR(nsDisplayWrapList);
>  
>    mList.AppendToTop(aItem);
> +  if (!mFrame->Preserves3D() && !mFrame->Preserves3DChildren()) {

To benefited from no virtual calls, I would move this code to another inlined function.

@@ +3221,5 @@
>  nsRect
>  nsDisplayWrapList::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap) {
> +  if (mFrame->Preserves3D() || mFrame->Preserves3DChildren()) {
> +    return mList.GetBounds(aBuilder);
> +  }

We don't compulte mBounds when running nsDisplayWrapList::Init() for we need accumulated transform to compute right bounds.  We call GetBounds() here to trigger the computation of bounds with accumulated transform.

I would explain the idea at nsDisplayTransform::GetBounds() in next patch.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 6 • 9 years ago

Thinker, are you currently working on this bug (and if so is there an ETA)? It looks like it could block desktop APZ, unless we decide to make APZ HWA-only.

	Thinker Li [:sinker] Assignee
	Comment 11 • 9 years ago

David, could you give me the bug of desktop APZ?

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 12 • 9 years ago

(In reply to Thinker Li [:sinker] from comment #11)
> David, could you give me the bug of desktop APZ?

I'm using bug 1086162 to track Windows-specific issues. There's also bug 1013364 which is more general.

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

Comment on attachment 8562091 [details] [diff] [review]
Part 1: Remove value of Z axis from 3D transforms

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

Otherwise looks good, but I'd like Matt to review it too.

::: gfx/layers/composite/ColorLayerComposite.cpp
@@ +44,5 @@
> +  gfx::Matrix4x4 transform = GetEffectiveTransform();
> +  transform._13 = 0;
> +  transform._23 = 0;
> +  transform._33 = 1;
> +  transform._43 = 0;

Let's add a function to Matrix4x4 which does this. Call it "RemoveZComponent"?

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

Comment on attachment 8562092 [details] [diff] [review]
Part 2: Snap translation for 3d transforms

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

::: gfx/layers/Layers.cpp
@@ +597,3 @@
>    }
> +
> +  if(aTransform.IsSingular()) {

Space before (

@@ +622,5 @@
> +  }
> +
> +  // Compute the snap from the transformed snap.
> +  Point3D snap = inverse * transformedSnap;
> +  if (snap.z > 0.001 || snap.z < -0.001) {

fabs(snap.z) > 0.001

@@ +636,5 @@
> +    *aResidualTransform = Matrix::Translation(-snap.x, -snap.y);
> +  }
> +
> +  // Translate transformed origin to transformed snap since the
> +  // residual transform would trnslate the snap to the origin.

"translate"

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 9 years ago

Comment on attachment 8562091 [details] [diff] [review]
Part 1: Remove value of Z axis from 3D transforms

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

Why do we need to do this?

Our compositors all use a viewport matrix that scales the z axis by 0, so I wouldn't expect this to have any effect.

We have Matrix4x4::ProjectTo2D, which should do the same thing as this.

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

Comment on attachment 8562094 [details] [diff] [review]
Part 4: Handle preserves-3d by compositor

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

I think it would be very helpful if either in this patch, or (actually better) a patch you insert before this patch, we rename identifiers to match the way the spec is now written:
dev.w3.org/csswg/css-transforms/

So on Layer I think we need a flag CONTENT_ESTABLISHES_3D_CONTEXT. I *think* we should be able to make that the only preserve-3d related layer flag, though I'm not sure.

Something similar should apply to nsIFrame.

	Matt Woodrow (:mattwoodrow)
	Comment 17 • 9 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Comment on attachment 8562091 [details] [diff] [review]
> Part 1: Remove value of Z axis from 3D transforms
> 
> Review of attachment 8562091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we need to do this?
> 
> Our compositors all use a viewport matrix that scales the z axis by 0, so I
> wouldn't expect this to have any effect.
> 
> We have Matrix4x4::ProjectTo2D, which should do the same thing as this.

The CompositorD3D9 implementation doesn't appear to do this, so that's the issue.

Compare [1] to [2].

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/CompositorD3D9.cpp#676
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#1170

	Thinker Li [:sinker] Assignee
	Comment 18 • 9 years ago

--
Attachment #8562091 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 19 • 9 years ago

--
Attachment #8562093 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 20 • 9 years ago

--
Attachment #8562094 [details] [diff] - Attachment is obsolete: true

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

Comment on attachment 8569044 [details] [diff] [review]
Part 4: Fix preserve3d wording for layer flags

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

::: gfx/layers/Layers.h
@@ +781,5 @@
>      /**
>       * If this is set then this layer is part of a preserve-3d group, and should
>       * be sorted with sibling layers that are also part of the same group.
>       */
> +    CONTENT_EXTEND_3D_CONTEXT = 0x08,

Actually, this needs more documentation.

Say something like
"When this is set, this layer extends its parent's 3D rendering context. When not set, this layer is the root of its own 3D rendering context."

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

Comment on attachment 8569042 [details] [diff] [review]
Part 5: Handle preserves-3d by compositor, v2

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

::: gfx/layers/Layers.h
@@ +798,5 @@
> +
> +    /**
> +     * This layer creates a new 3D context for descendants.
> +     */
> +    CONTENT_CREATE_3D_CONTEXT = 0x40

I don't understand what it means for a layer to have CONTENT_EXTEND_3D_CONTEXT set, and CONTENT_CREATE_3D_CONTEXT also set. Likewise it's not really clear what it means for a layer to have CONTENT_EXTEND_3D_CONTEXT not set, and CONTENT_CREATE_3D_CONTEXT also not set. I think we need to explain this very carefully.

In the spec (http://dev.w3.org/csswg/css-transforms-1/#3d-rendering-contexts) there are two bits of relevant state:
1) An element either flattens, creating a new 3D rendering context for its descendants, or extends the 3D rendering context of its parent.
2) Whether content is "without 3D transforms" according to section 6.1.2 step B. (There is an open issue about the spec: in the spec, 2D-transformed elements are rendered at z=0 in their 3D rendering context, but probably it's better (more Web-compatible, and more efficient) to render them directly onto the element background, so it makes sense to have a special flag to indicate that).

So I'd really like flags that correspond to those two bits of state, with names to match. I guess the default values should indicate that an element flattens and is without 3D transforms. So, the flags could be CONTENT_EXTEND_3D_CONTEXT and CONTENT_IS_3D_TRANSFORMED. (Note that it's possible for an element to have CONTENT_IS_3D_TRANSFORMED but for the transform to be only 2D because of animations or a 3D transform being specified that results in zero Z values.) A layer with CONTENT_EXTEND_3D_CONTEXT must also have CONTENT_IS_3D_TRANSFORMED, but it's OK to have CONTENT_IS_3D_TRANSFORMED without CONTENT_EXTEND_3D_CONTEXT.

Would those flags work instead of your current CONTENT_EXTEND_3D_CONTEXT and CONTENT_CREATE_3D_CONTEXT? What is the relationship between those flags and the flags I'm suggesting?

Whatever we do, we should change nsIFrame::Preserves3D/Children/Leaf to be consistent with the definitions we come up with here.

We can discuss this in #gfx to save time :-)

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

Comment on attachment 8569042 [details] [diff] [review]
Part 5: Handle preserves-3d by compositor, v2

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

Sorry, didn't mean to r+ this yet

	Thinker Li [:sinker] Assignee
	Comment 25 • 9 years ago

--
Attachment #8569041 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 26 • 9 years ago

--
Attachment #8569042 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 28 • 9 years ago

--
Attachment #8573758 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 29 • 9 years ago

(In reply to Thinker Li [:sinker] from comment #26)
> Created attachment 8573757 [details] [diff] [review]
> Part 5: Handle preserves-3d by compositor, v3
> 
> --
> Attachment #8569042 [details] [diff] [diff] - Attachment is obsolete: true

Remove CONTENT_CREATE_3D_CONTEXT flag from layers.

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

Comment on attachment 8573757 [details] [diff] [review]
Part 5: Handle preserves-3d by compositor, v3

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

::: gfx/layers/Layers.h
@@ +1412,5 @@
> +  }
> +  bool IsCreate3DContext() {
> +    return GetType() == TYPE_CONTAINER &&
> +      !IsExtend3DContext() && GetParent() &&
> +      reinterpret_cast<Layer*>(GetParent())->IsExtend3DContext();

Why do we need to check the parent here?

Isn't it true that every layer which does not have CONTENT_EXTEND_3D_CONTEXT actually creates a 3D context, according to the spec?

@@ +1419,5 @@
> +    // For non-leaf container in a preserves3d group, visible region
> +    // is meaningless, since they are only intermediate result of
> +    // content.
> +    return !GetEffectiveVisibleRegion().IsEmpty() ||
> +      (IsExtend3DContext() && !IsCreate3DContext());

I don't quite understand this either. Why are we checking IsCreate3DContext here? Can we just check IsExtend3DContext? Is the issue that IsVisible must return true when GetEffectiveVisibleRegion().IsEmpty() and the layer may have child layers which also extend the same 3D context and apply their own 3D transforms which make them visible again?

If so, could we just check IsExtend3DContext && GetType() == TYPE_CONTAINER?

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

Also, it's not really fair to pile extra work onto you, but it would be really very helpful to have a patch that renames the nsIFrame::IsPreserve3D* methods to use names that reflect the concepts in the specification.

	Thinker Li [:sinker] Assignee
	Comment 32 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> 
> Why do we need to check the parent here?
> 
> Isn't it true that every layer which does not have CONTENT_EXTEND_3D_CONTEXT
> actually creates a 3D context, according to the spec?

I think it should be rename to a better name (Is3DContextLeaf?) to reflect that it's intention is to test if a layer is a leaf layer of a 3d rendering context.

> 
> I don't quite understand this either. Why are we checking IsCreate3DContext
> here? Can we just check IsExtend3DContext? Is the issue that IsVisible must
> return true when GetEffectiveVisibleRegion().IsEmpty() and the layer may
> have child layers which also extend the same 3D context and apply their own
> 3D transforms which make them visible again?
> 
> If so, could we just check IsExtend3DContext && GetType() == TYPE_CONTAINER?

This is something I should check, but not, after changing meaning of CONTENT_EXTEND_3D_CONTEXT flag.  I think it is enough to remove !IsCreate3DContext() from the statement since CONTENT_ExtEND_3D_CONTEXT flags is never applied on non-container layers.

	Thinker Li [:sinker] Assignee
	Comment 34 • 9 years ago

--
Attachment #8573757 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 35 • 9 years ago

--
Attachment #8573761 [details] [diff] - Attachment is obsolete: true

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

Comment on attachment 8575234 [details] [diff] [review]
Part 5: Fix preserve3d wording for nsIFrame

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

this is good... thanks.

::: layout/generic/nsIFrame.h
@@ +1222,4 @@
>     * children. This requires transform-style: preserve-3d, as well as no clipping
>     * or svg effects.
>     */
> +  bool IsExtend3DContext() const;

Call this Extends3DContext().

@@ +1229,4 @@
>     * its own transform (or hidden backface) to be combined with the parent's
>     * transform.
>     */
> +  bool IsExtend3DContextChild() const;

This name isn't as clear as it should be.

This method determines if this frame has a transform that may need to be combined with some ancestor's transform. Maybe we'll need this method less after your patches in this bug.

How about we call this "Combines3DTransformWithAncestors"?

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

Comment on attachment 8575236 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v4

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

::: gfx/layers/LayerSorter.cpp
@@ +58,5 @@
>      return n/d;
>  }
>  
> +static gfx3DMatrix
> +GetAccumulatedTransformPreverse3DSlow(Layer* aLayer) {

"Preserve"

@@ +72,5 @@
> +/**
> + * Get accumulated transform in preserves3d chain.
> + */
> +static gfx3DMatrix
> +GetAccumulatedTransformPreverse3D(Layer* aLayer) {

"Preserve"

@@ +73,5 @@
> + * Get accumulated transform in preserves3d chain.
> + */
> +static gfx3DMatrix
> +GetAccumulatedTransformPreverse3D(Layer* aLayer) {
> +  // Context root is the parent of the container that closet to root

"closest"

@@ +94,5 @@
> +  if (!root_invert.Invert()) {
> +    return GetAccumulatedTransformPreverse3DSlow(aLayer);
> +  }
> +
> +  return To3DMatrix(aLayer->GetEffectiveTransform() * root_invert);

Can we simplify this by requiring that every layer which establishes its own 3D context, and has children which extend that 3D context, has an intermediate surface? Clearly children which extend their parent's 3D context can't have an intermediate surface. Then I think each layer's accumulated 3D transform will just be its GetEffectiveTransform. Is that right?

::: gfx/layers/Layers.cpp
@@ +1212,5 @@
>          SortLayersBy3DZOrder(toSort);
>          aArray.MoveElementsFrom(toSort);
>        }
>        aArray.AppendElement(l);
>      }

This loop doesn't match what's in the spec. The spec says that all the children should be 3D-sorted together no matter which ones extend the 3D context. To implement that, we would call this->Collect3DContextLeaves and then call SortLayersBy3DZOrder on the result.

That means that a container with 3D transforms on its children and no preserve-3d anywhere would allow those children to intersect in 3D space. But that's not what we do, or what Chrome does. I'll followup with Simon about the spec. In the meantime, let's keep on doing what you're doing here.

@@ +1226,5 @@
>  {
>    Matrix residual;
>    Matrix4x4 idealTransform = GetLocalTransform() * aTransformToSurface;
> +  if (!IsExtend3DContext() && !Is3DContextLeaf()) {
> +    idealTransform.ProjectTo2D();

Can't we just make this "if (!IsExtend3DContext())"? I'm not sure why we need to check Is3DContextLeaf here.

@@ +1244,5 @@
>      CompositionOp blendMode = GetEffectiveMixBlendMode();
>      if ((opacity != 1.0f || blendMode != CompositionOp::OP_OVER) && HasMultipleChildren()) {
>        useIntermediateSurface = true;
> +    } else if (Is3DContextLeaf()) {
> +      useIntermediateSurface = true;

I'm not sure why we need this code here.

::: gfx/layers/Layers.h
@@ +1415,5 @@
>    // accounting for this layer possibly being a shadow.
>    const nsIntRect* GetEffectiveClipRect();
>    const nsIntRegion& GetEffectiveVisibleRegion();
>  
> +  bool IsExtend3DContext() {

Let's call this Extends3DContext().

@@ +1421,5 @@
> +  }
> +  bool Is3DContextLeaf() {
> +    return GetType() == TYPE_CONTAINER &&
> +      !IsExtend3DContext() && GetParent() &&
> +      reinterpret_cast<Layer*>(GetParent())->IsExtend3DContext();

This method still doesn't feel good to me. Really, every layer that is !IsExtend3DContext() is a "left" in its parent's 3D context.

I'll comment where we use it, to try to figure out what we really want.

::: gfx/layers/basic/BasicContainerLayer.cpp
@@ +45,5 @@
> +    mEffectiveTransform = idealTransform;
> +    ComputeEffectiveTransformsForChildren(idealTransform);
> +    ComputeEffectiveTransformForMaskLayer(idealTransform);
> +    // Non-leaf Prserves3D containers never use intermediate surface
> +    // since all children are composited separated in the the context.

// Don't use an intermediate surface since all children are composited
// into our parent's 3D context.

@@ +50,5 @@
> +    return;
> +  }
> +
> +  if (!Is3DContextLeaf()) {
> +    idealTransform.ProjectTo2D();

Again, I don't know why we need to check Is3DContextLeaf here.

@@ +53,5 @@
> +  if (!Is3DContextLeaf()) {
> +    idealTransform.ProjectTo2D();
> +  }
> +
> +  if (Is3DContextLeaf() || !idealTransform.CanDraw2D()) {

Or here.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +150,5 @@
>    bool Setup2DTransform()
>    {
> +    if (mLayer->IsExtend3DContext() || mLayer->Is3DContextLeaf()) {
> +      return false;
> +    }

I don't know why we need these checks here either.

@@ +855,5 @@
> +    if (inPreserves3DChain) {
> +      InstallLayerClipPreserves3D(aTarget, aLayer);
> +      for (Layer *l = parent; l && l->IsExtend3DContext(); l = l->GetParent()) {
> +        InstallLayerClipPreserves3D(aTarget, l);
> +      }

I don't understand this ... It seems to me that whenever a layer extends its parent's 3D context, it should not have a clip rect.

::: layout/base/FrameLayerBuilder.cpp
@@ +570,5 @@
>    // container reference frame.
>    nsIntRegion mVisibleRegion;
>    nsIntRegion mOpaqueRegion;
> +  // These visible regions are relative to the associated frame before
> +  // transform.

This isn't quite clear enough. What is "the associated frame"?

@@ +716,5 @@
>     */
>    void SetOuterVisibleRegionForLayer(Layer* aLayer,
>                                       const nsIntRegion& aOuterVisibleRegion,
> +                                     const nsIntRect* aLayerContentsVisibleRect = nullptr,
> +                                     bool aOuterUntransformed = false) const;

Please document this parameter

::: layout/base/nsLayoutUtils.cpp
@@ +6356,5 @@
>  nsLayoutUtils::GetReferenceFrame(nsIFrame* aFrame)
>  {
>    nsIFrame *f = aFrame;
>    for (;;) {
> +    if (f->IsTransformed() || f->Preserves3DLeaf() || IsPopup(f)) {

Seems to me the call to Preserves3DLeaf here should not be needed.

::: layout/generic/nsFrame.cpp
@@ +2389,5 @@
> +        WrapTransformIfNot(aBuilder, child, aLists.BlockBorderBackgrounds());
> +        WrapTransformIfNot(aBuilder, child, aLists.Floats());
> +        WrapTransformIfNot(aBuilder, child, aLists.PositionedDescendants());
> +        WrapTransformIfNot(aBuilder, child, aLists.Outlines());
> +        WrapTransformIfNot(aBuilder, child, aLists.Content());

Why do you still need to wrap display items in transforms here?

::: layout/generic/nsIFrame.h
@@ +1231,5 @@
>     */
>    bool IsExtend3DContextChild() const;
>  
> +  bool Preserves3DLeaf() const {
> +    return IsExtend3DContextChild() && !IsExtend3DContext();

So if the frame has a preserve-3d parent, is not preserve-3d itself, but has only a 2D transform, this will return false. Is that really OK?

I wonder if we can replace this with just !IsExtend3DContext().

	Thinker Li [:sinker] Assignee
	Comment 38 • 9 years ago

--
Attachment #8575236 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 39 • 9 years ago

This revision creates container layers only for frames with transforms while earlier revision creates container layers for every frame in a preserve 3d chain.

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

Comment on attachment 8606966 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v5

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

Getting closer! :-)

::: gfx/layers/Layers.cpp
@@ +736,5 @@
> +    }
> +    clipLayer = clipLayer->GetParent();
> +  }
> +  if (container == clipLayer) {
> +    clipLayer = this;

This isn't very clear. Can you explain more? In particular it seems to me that no layer should have Extend3DContext and a clip rect, but you've written code here and elsewhere that tries to handle that. Right?

@@ +1298,5 @@
>        }
>      }
>    }
>  
> +  if (!Extend3DContext()) {

Should this be Is3DContextLeaf()? Then we could avoid doing ProjectTo2D twice.

::: gfx/layers/Layers.h
@@ +1420,5 @@
> +  }
> +  bool Is3DContextLeaf() {
> +    return GetType() == TYPE_CONTAINER &&
> +      !Extend3DContext() && GetParent() &&
> +      reinterpret_cast<Layer*>(GetParent())->Extend3DContext();

We don't need this cast. You can mark this method inline and provide the implementation in Layers.h, below the definition of ContainerLayer.

@@ +2057,5 @@
> +  /**
> +   * True for if the container start a new 3D context extended by one
> +   * or more children.
> +   */
> +  bool IsCreating3DContextAndExtended();

Call this Creates3DContextWithExtendingChildren?

::: gfx/layers/basic/BasicContainerLayer.cpp
@@ +85,5 @@
>      (GetMixBlendMode() != CompositionOp::OP_OVER && HasMultipleChildren()) ||
>      (GetEffectiveOpacity() != 1.0 && (HasMultipleChildren() || hasSingleBlendingChild));
> +
> +  if (!Extend3DContext()) {
> +    idealTransform.ProjectTo2D();

Why is this needed? Haven't we already done it above?

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +854,5 @@
> +  if (!clipRect) {
> +    return;
> +  }
> +
> +  Layer *parent = aLayer->GetParent();

Layer*

@@ +920,2 @@
>  
> +  Layer *parent = aLayer->GetParent();

Layer*

@@ +926,5 @@
>    if (needsSaveRestore) {
>      contextSR.SetContext(aTarget);
>  
> +    InstallLayerClipPreserves3D(aTarget, aLayer);
> +    for (Layer *l = parent; l && l->Extend3DContext(); l = l->GetParent()) {

Layer*

::: layout/base/FrameLayerBuilder.cpp
@@ +1002,5 @@
>     */
>    void SetOuterVisibleRegionForLayer(Layer* aLayer,
>                                       const nsIntRegion& aOuterVisibleRegion,
> +                                     const nsIntRect* aLayerContentsVisibleRect = nullptr,
> +                                     bool aOuterUntransformed = false) const;

Please document what this means.

@@ +3738,5 @@
> +             item->Frame()->Combines3DTransformWithAncestors())) {
> +          // Give untransformed visible region as outer visible region
> +          // to avoid failure caused by singular transforms.
> +          newLayerEntry->mChildrenVisibleRegion =
> +            item->GetVisibleRectForChildren().ToNearestPixels(mAppUnitsPerDevPixel);

ToOutsidePixels

@@ +3750,5 @@
> +          item->GetType() == nsDisplayItem::TYPE_TRANSFORM &&
> +          item->Frame()->Preserves3DLeaf();
> +        const nsIntRegion &visible = !useChildrenVisible ?
> +          itemVisibleRect :
> +          item->GetVisibleRectForChildren().ToNearestPixels(mAppUnitsPerDevPixel);

Remove ! from the condition and swap the ? : cases.

ToOutsidePixels

::: layout/base/nsDisplayList.cpp
@@ +4614,5 @@
> +nsDisplayTransform::nsDisplayTransform(nsDisplayListBuilder* aBuilder,
> +                                       nsIFrame *aFrame, nsDisplayList *aList,
> +                                       const nsRect& aChildrenVisibleRect,
> +                                       const Matrix4x4& aTransform,
> +                                       uint32_t aIndex) 

trailing whitespace

@@ +5650,5 @@
>  }
>  
> +/**
> + * Remove all children that is backface hidden if backface of the
> + * transform is visible.

"is not visible"?

I'm not sure we should do this here. Consider the case where we have a compositor animation of an element where the animation changes which side of the element is shown. It seems to me we should always render to layers as if backface-visibility was 'visible' and then let the compositor be responsible for backface-visibility. Is that right?

::: layout/base/nsDisplayList.h
@@ +128,5 @@
> +
> +    // Accmulate transforms of ancestors on the preserves-3d chain.
> +    Matrix4x4 mAccumulatedTransform;
> +    // Accmulate visible rect of descendants in the preserves-3d context.
> +    nsRect mAccumulatedRect;

What's this relative to?

@@ +130,5 @@
> +    Matrix4x4 mAccumulatedTransform;
> +    // Accmulate visible rect of descendants in the preserves-3d context.
> +    nsRect mAccumulatedRect;
> +    // How many ancestors on the preserves-3d chain is for current context.
> +    int mAccumulatedRectLevels;

This comment is isn't very clear

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

Comment on attachment 8575237 [details] [diff] [review]
Part 7: Rendering elements w/o 3d-transforms on z=0 plane, v3

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

The patch looks OK but I'm not sure we should do this. There is significant compatibility risk. Let's hold off on this for now unless you have a good reason to do it other than align with the spec.

	Markus Stange [:mstange]
	Comment 42 • 9 years ago

The patches in this bug don't seem to have to do anything with subpixel AA flattening. I'm renaming this bug and filing a new one.

	Thinker Li [:sinker] Assignee
	Comment 43 • 9 years ago

Comment on attachment 8606966 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v5

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

::: gfx/layers/Layers.cpp
@@ +736,5 @@
> +    }
> +    clipLayer = clipLayer->GetParent();
> +  }
> +  if (container == clipLayer) {
> +    clipLayer = this;

ContainerState::SetupScrollingMetadata() may install a clip on container layer.  So, it is possible that the children of the layer creating the context are with a clip and extending context.

@@ +1298,5 @@
>        }
>      }
>    }
>  
> +  if (!Extend3DContext()) {

I think we could remove the earlier line of ProjectTo2D() since we always insert an additional container layer, as a leaf, between consecutive extended contexts now.

::: gfx/layers/basic/BasicContainerLayer.cpp
@@ +85,5 @@
>      (GetMixBlendMode() != CompositionOp::OP_OVER && HasMultipleChildren()) ||
>      (GetEffectiveOpacity() != 1.0 && (HasMultipleChildren() || hasSingleBlendingChild));
> +
> +  if (!Extend3DContext()) {
> +    idealTransform.ProjectTo2D();

see the comment for DefaultComputeEffectiveTransforms().

::: layout/base/nsDisplayList.cpp
@@ +5650,5 @@
>  }
>  
> +/**
> + * Remove all children that is backface hidden if backface of the
> + * transform is visible.

If "backface is visible", the content should be hidden.

You are right, I would add a new flag for layers for this purpose.

::: layout/base/nsDisplayList.h
@@ +128,5 @@
> +
> +    // Accmulate transforms of ancestors on the preserves-3d chain.
> +    Matrix4x4 mAccumulatedTransform;
> +    // Accmulate visible rect of descendants in the preserves-3d context.
> +    nsRect mAccumulatedRect;

It is here to keep rects surviving through singular intermediate preserves 3d container layers.

	Thinker Li [:sinker] Assignee
	Comment 44 • 9 years ago

--
Attachment #8606966 [details] [diff] - Attachment is obsolete: true

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

Comment on attachment 8624631 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v6

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

I think this is getting pretty close :-)

::: gfx/layers/LayerSorter.cpp
@@ +83,5 @@
> +             aTwo->GetParent() && aTwo->GetParent()->Extend3DContext());
> +  // Effective transform of leaves may had been projected to 2D.
> +  gfx3DMatrix ourTransform =
> +    To3DMatrix(aOne->GetLocalTransform() *
> +               aOne->GetParent()->GetEffectiveTransform());

Why can't we just use aOne->GetEffectiveTransform()?

::: gfx/layers/Layers.cpp
@@ +1194,5 @@
> +/**
> + * Collect all leaf descendants of the current 3D context.
> + */
> +void
> +ContainerLayer::Collect3DContextLeaves(nsTArray<Layer*>& aToSort) {

{ goes on new line

@@ +1229,5 @@
>    }
>  }
>  
> +bool
> +ContainerLayer::Creates3DContextWithExtendingChildren() {

{ goes on new line

@@ +1302,5 @@
> +  }
> +
> +  if (!Extend3DContext()) {
> +    // For layers extending 3d context, its ideal transform should be
> +    // applied on children.  Without this project, non-container

"Without this projection"

@@ +1303,5 @@
> +
> +  if (!Extend3DContext()) {
> +    // For layers extending 3d context, its ideal transform should be
> +    // applied on children.  Without this project, non-container
> +    // children would get a 3D transform while 2D is expect.

"is expected"

::: gfx/layers/Layers.h
@@ +792,5 @@
> +    CONTENT_DISABLE_SUBPIXEL_AA = 0x20,
> +
> +    /**
> +     * This layer is hidden if the backface of the layer is visible
> +     * from user.

"visible to user"

@@ +1432,5 @@
> +   * It is true if the user can see the back of the layer and the
> +   * backface is hidden.  The compositor should skip the layer if the
> +   * result is true.
> +   */
> +  bool SeeBackfaceHidden() {

IsBackfaceHidden

@@ +1435,5 @@
> +   */
> +  bool SeeBackfaceHidden() {
> +    if (GetContentFlags() & CONTENT_BACKFACE_HIDDEN) {
> +      Layer* container = AsContainerLayer() ?
> +        this : reinterpret_cast<Layer*>(GetParent());

We should be able to avoid this cast by moving this function to Layers.cpp.

@@ +1440,5 @@
> +      if (container) {
> +        if (container->Extend3DContext() || container->Is3DContextLeaf()) {
> +          return container->GetEffectiveTransform().IsBackfaceVisible();
> +        }
> +        return container->GetBaseTransform().IsBackfaceVisible();

Why can't we just use, for all layer types,
  return GetEffectiveTransform().IsBackfaceVisible();
?

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +854,5 @@
>    }
>  }
>  
> +static void
> +InstallLayerClipPreserves3D(gfxContext* aTarget, Layer* aLayer)

Please add a comment explaining what this does.

@@ +869,5 @@
> +    gfx::To3DMatrix(parent->GetEffectiveTransform()) :
> +    gfx3DMatrix();
> +  gfxMatrix transform;
> +  if (!transform3d.CanDraw2D(&transform)) {
> +    return;

It doesn't seem right to just return here. Can we assert that this path is never taken?

::: layout/base/FrameLayerBuilder.cpp
@@ +1004,5 @@
>    nsIFrame* GetContainerFrame() const { return mContainerFrame; }
>    nsDisplayListBuilder* Builder() const { return mBuilder; }
>  
>    /**
> +   * Sets aOuterVisibleRegion as aLayer's visible region. 

trailing whitespace

@@ +3104,5 @@
> +    data->mLayer->SetContentFlags(data->mLayer->GetContentFlags() |
> +                                  Layer::CONTENT_BACKFACE_HIDDEN);
> +  } else {
> +    data->mLayer->SetContentFlags(data->mLayer->GetContentFlags() &
> +                                  ~Layer::CONTENT_BACKFACE_HIDDEN);

Let's have a helper function SetLayerBackfaceHidden(Layer*, bool) that you can call twice here and also below

@@ +3776,5 @@
>                     "Transform items must set layerContentsVisibleRect!");
>        if (mLayerBuilder->IsBuildingRetainedLayers()) {
>          newLayerEntry->mLayerContentsVisibleRect = layerContentsVisibleRect;
>          newLayerEntry->mVisibleRegion = itemVisibleRect;
> +        if (item->GetType() == nsDisplayItem::TYPE_TRANSFORM &&

use itemType

@@ +3783,5 @@
> +          // Give untransformed visible region as outer visible region
> +          // to avoid failure caused by singular transforms.
> +          newLayerEntry->mChildrenVisibleRegion =
> +            item->GetVisibleRectForChildren().ToOutsidePixels(mAppUnitsPerDevPixel);
> +        }

Shouldn't we set mChildrenVisibleRegion to mVisibleRegion in the else case here? That might simplify code below.

::: layout/base/nsDisplayList.cpp
@@ +552,2 @@
>        nsIFrame* referenceFrame =
> +        nsLayoutUtils::GetReferenceFrame(aFrame);

probably don't need a line break here

@@ +4612,5 @@
> +nsDisplayTransform::nsDisplayTransform(nsDisplayListBuilder* aBuilder,
> +                                       nsIFrame *aFrame, nsDisplayList *aList,
> +                                       const nsRect& aChildrenVisibleRect,
> +                                       const Matrix4x4& aTransform,
> +                                       uint32_t aIndex) 

remove trailing whitespace

@@ +5125,5 @@
> +const Matrix4x4&
> +nsDisplayTransform::GetAccumulatedPreserved3DTransform()
> +{
> +  // XXX: should go back to fix mTransformGetter.
> +  if (mTransformP3D.IsIdentity()) {

Maybe we should use a bool flag so we don't keep going through the slow path when mTransformP3D happens to be the identity?

::: layout/base/nsDisplayList.h
@@ +3771,5 @@
>    uint32_t mIndex;
>    // We wont know if we pre-render until the layer building phase where we can
>    // check layers will-change budget.
>    bool mMaybePrerender;
> +  // True for as 3D context separator.

I'm not sure what this comment means.

@@ +3773,5 @@
>    // check layers will-change budget.
>    bool mMaybePrerender;
> +  // True for as 3D context separator.
> +  bool mNoExtendContext;
> +  bool mHasPresetTransform;

This needs a comment too

	:kip (Kearwood Gilbert)
	Comment 46 • 9 years ago

I have applied this patch set to my local copy and did some tests.  It makes my work on fixing the CSS transformed element sorting much easier; I have rebased my WIP sorting fixes onto these patches.

tlee - Are you still actively working on this issue?  I'd be glad to help.

	Thinker Li [:sinker] Assignee
	Comment 47 • 9 years ago

Hi Gilbert,
I am still working on this bug, although I am shifting between projects.  So, I keep to update the patch.

	:kip (Kearwood Gilbert)
	Comment 48 • 9 years ago

Thanks, Thinker!  It is looking great so far.  Please ask any time if i can help with anything.

	Matt Woodrow (:mattwoodrow)
	Comment 49 • 9 years ago

Thinker, any idea when this bug will be finished?

I need it done before I can fix bug 1168263, so this is a high(ish) priority for me now.

	Thinker Li [:sinker] Assignee
	Comment 50 • 9 years ago

Hi Matt, I would upload another patch in this week.

	Matt Woodrow (:mattwoodrow)
	Comment 51 • 9 years ago

Comment on attachment 8624631 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v6

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

::: gfx/layers/Layers.h
@@ +1432,5 @@
> +   * It is true if the user can see the back of the layer and the
> +   * backface is hidden.  The compositor should skip the layer if the
> +   * result is true.
> +   */
> +  bool SeeBackfaceHidden() {

Maybe IsShowingHiddenBackface()? This function requires the BACKFACE_HIDDEN flag to be set, and the current transform to be such that the backface would be visible.

@@ +1440,5 @@
> +      if (container) {
> +        if (container->Extend3DContext() || container->Is3DContextLeaf()) {
> +          return container->GetEffectiveTransform().IsBackfaceVisible();
> +        }
> +        return container->GetBaseTransform().IsBackfaceVisible();

The effective transform can include non-preserve-3d parent transforms, since we don't always require an intermediate.

::: layout/base/nsDisplayList.h
@@ +3719,5 @@
>                                                   const nsPoint& aOrigin,
>                                                   float aAppUnitsPerPixel,
>                                                   const nsRect* aBoundsOverride = nullptr,
>                                                   nsIFrame** aOutAncestor = nullptr);
> +  static gfx3DMatrix GetResultingTransformMatrixP3D(const nsIFrame* aFrame,

Rather than adding this extra method, can we instead turn aOffsetByOrigin into a flags word that specifies what components to include in the transform?

Something like:

enum {
LOCAL_TRANSFORM_ONLY = 0x1,
INCLUDE_PRESERVE_3D_ANCESTORS = 0x2,
OFFSET_BY_ORIGIN = 0x3,
};

Assert that exactly one of the first two flags is specified.

TransformRect/UntransformRect can just pass the flags through instead of having the ternary.

This will be helpful for me, since I also want to conditionally include the perspective transform.

::: layout/generic/nsFrame.cpp
@@ -1775,5 @@
>  }
>  #endif
>  
> -static nsresult
> -WrapPreserve3DListInternal(nsIFrame* aFrame, nsDisplayListBuilder *aBuilder,

\o/

@@ +2078,5 @@
> +    resultList.AppendNewToTop(transformItem);
> +
> +    /*
> +     * Create an additional transform item as a separator layer
> +     * between current and parent's 3D context if necessary.

I think we could just have a layer flag to specify when a layer is the 'root' of a new preserve-3d context rather than extending the parents one.

That might be simpler than inserting dummy layers, but it's not a big deal.

::: layout/generic/nsIFrame.h
@@ +1227,5 @@
>     * transform.
>     */
>    bool Combines3DTransformWithAncestors() const;
>  
> +  bool Preserves3DLeaf() const {

IsPreserve3DLeaf()

	Matt Woodrow (:mattwoodrow)
	Comment 52 • 9 years ago

My first two comments there were in reply to/addition to roc's comments, they read better in the splinter view :)

That sounds good, thanks Thinker!

I ended up rebasing the patches, and pushing the first 5 to try.

Try push if you want to take the changesets: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26828d090e67

I can upload the rebased part 6 too if you'd like.

	Thinker Li [:sinker] Assignee
	Comment 53 • 9 years ago

Comment on attachment 8624631 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v6

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

::: gfx/layers/LayerSorter.cpp
@@ +83,5 @@
> +             aTwo->GetParent() && aTwo->GetParent()->Extend3DContext());
> +  // Effective transform of leaves may had been projected to 2D.
> +  gfx3DMatrix ourTransform =
> +    To3DMatrix(aOne->GetLocalTransform() *
> +               aOne->GetParent()->GetEffectiveTransform());

At leaves, its effective transform might be projected to 2D.  So, we need to recover it at here.

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +869,5 @@
> +    gfx::To3DMatrix(parent->GetEffectiveTransform()) :
> +    gfx3DMatrix();
> +  gfxMatrix transform;
> +  if (!transform3d.CanDraw2D(&transform)) {
> +    return;

yes

	Thinker Li [:sinker] Assignee
	Comment 54 • 9 years ago

Comment on attachment 8624631 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v6

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

::: layout/base/nsDisplayList.h
@@ +3719,5 @@
>                                                   const nsPoint& aOrigin,
>                                                   float aAppUnitsPerPixel,
>                                                   const nsRect* aBoundsOverride = nullptr,
>                                                   nsIFrame** aOutAncestor = nullptr);
> +  static gfx3DMatrix GetResultingTransformMatrixP3D(const nsIFrame* aFrame,

I don't quite understand what you want here.  The choice of local-only or not is not exclusive with aOffsetByOrigin. Do you like to have an integer including two bitwise flags?

::: layout/generic/nsFrame.cpp
@@ +2078,5 @@
> +    resultList.AppendNewToTop(transformItem);
> +
> +    /*
> +     * Create an additional transform item as a separator layer
> +     * between current and parent's 3D context if necessary.

I had considered this option.  This option would pollute the code of compositors with the concern here.

	Thinker Li [:sinker] Assignee
	Comment 55 • 9 years ago

--
Attachment #8624631 [details] [diff] - Attachment is obsolete: true

	Matt Woodrow (:mattwoodrow)
	Comment 56 • 9 years ago

(In reply to Thinker Li [:sinker] from comment #54)
> Comment on attachment 8624631 [details] [diff] [review]
> Part 6: Handle preserves-3d by compositor, v6
> 
> Review of attachment 8624631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.h
> @@ +3719,5 @@
> >                                                   const nsPoint& aOrigin,
> >                                                   float aAppUnitsPerPixel,
> >                                                   const nsRect* aBoundsOverride = nullptr,
> >                                                   nsIFrame** aOutAncestor = nullptr);
> > +  static gfx3DMatrix GetResultingTransformMatrixP3D(const nsIFrame* aFrame,
> 
> I don't quite understand what you want here.  The choice of local-only or
> not is not exclusive with aOffsetByOrigin. Do you like to have an integer
> including two bitwise flags?
> 

Yes, exactly.

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

Comment on attachment 8656998 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v7

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

A big scary patch, but I'm happy with it, modulo these cosmetic issues. Congratulations!

::: gfx/layers/Layers.cpp
@@ +1252,5 @@
>    Matrix residual;
>    Matrix4x4 idealTransform = GetLocalTransform() * aTransformToSurface;
> +
> +  if (!Extend3DContext() && !Is3DContextLeaf()) {
> +    // Keep 3D transforms for leaves to keep z-order sorting correct.

Move the comment outside the "if" statement since it doesn't apply to this code path.

@@ +1305,5 @@
> +
> +  if (!Extend3DContext()) {
> +    // For layers extending 3d context, its ideal transform should be
> +    // applied on children.  Without this projection, non-container
> +    // children would get a 3D transform while 2D is expected.

The first sentence of this comment does not apply to the code path it's in, so it's confusing. Move it outside the "if".

@@ +1856,5 @@
> +Layer::IsBackfaceHidden()
> +{
> +  if (GetContentFlags() & CONTENT_BACKFACE_HIDDEN) {
> +    Layer* container = AsContainerLayer() ?
> +      this : reinterpret_cast<Layer*>(GetParent());

This reinterpret_cast should not be needed.

@@ +1861,5 @@
> +    if (container) {
> +      if (container->Extend3DContext() || container->Is3DContextLeaf()) {
> +        return container->GetEffectiveTransform().IsBackfaceVisible();
> +      }
> +      return container->GetBaseTransform().IsBackfaceVisible();

Add a comment like what Matt said.

::: layout/base/nsDisplayList.cpp
@@ +4623,5 @@
> +  , mChildrenVisibleRect(aChildrenVisibleRect)
> +  , mIndex(aIndex)
> +  , mNoExtendContext(false)
> +  , mHasPresetTransform(true)
> +  , mTransformP3DInited(false)

Better to spell out Preserves3D here and elsewhere.

::: layout/base/nsDisplayList.h
@@ +770,5 @@
>    };
>  
> +  class AutoAccumulateTransform;
> +  friend class AutoAccumulateTransform;
> +  class AutoAccumulateTransform {

Need a comment explaining what this class does

@@ +802,5 @@
> +  };
> +
> +  class AutoAccumulateRect;
> +  friend class AutoAccumulateRect;
> +  class AutoAccumulateRect {

Need a comment explaining what this class does

@@ +804,5 @@
> +  class AutoAccumulateRect;
> +  friend class AutoAccumulateRect;
> +  class AutoAccumulateRect {
> +  public:
> +    explicit AutoAccumulateRect(nsDisplayListBuilder *aBuilder)

nsDisplayListBuilder* here and elsewhere

@@ +823,5 @@
> +
> +  void AccumulateRect(const nsRect &aRect) {
> +    mPreserves3DCtx.mAccumulatedRect.UnionRect(mPreserves3DCtx.mAccumulatedRect, aRect);
> +  }
> +  const nsRect &GetAccumulatedRect() {

nsRect&

@@ +943,5 @@
>                                       nsIFrame** aOutResult);
>  
> +  class AutoPreserves3DContext;
> +  friend class AutoPreserves3DContext;
> +  class AutoPreserves3DContext {

Need a comment explaining what this class does

@@ +3774,5 @@
>    // We wont know if we pre-render until the layer building phase where we can
>    // check layers will-change budget.
>    bool mMaybePrerender;
> +  // Be forced not to extend 3D context.
> +  bool mNoExtendContext;

Please document this more clearly.

	Matt Woodrow (:mattwoodrow)
	Comment 58 • 9 years ago

Comment on attachment 8656998 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v7

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

Looks good!

::: gfx/layers/Layers.cpp
@@ +731,5 @@
> +  }
> +
> +  // Find the nearest layer with a clip, or this layer.
> +  // ContainerState::SetupScrollingMetadata() may install a clip on
> +  // the layer.

Can you explain this whole block a little more please? It's not very obvious what we're doing here (to me at least).

::: layout/base/nsDisplayList.cpp
@@ +896,5 @@
>        nsIFrame *child = childFrames.get();
>        if (child->Combines3DTransformWithAncestors()) {
>          mFramesMarkedForDisplay.AppendElement(child);
> +        nsRect dirty;
> +        if (!nsDisplayTransform::UntransformRect(aDirtyRect,

Why do we untransform and then re-transform?

	Thinker Li [:sinker] Assignee
	Comment 59 • 9 years ago

trying to fix two reftest cases.
see https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a339aaad5b4

 1. filter-html-01-extref.xhtml  for Android 2.3
 2. 1035611-1.html for windows

	Thinker Li [:sinker] Assignee
	Comment 60 • 9 years ago

Comment on attachment 8656998 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v7

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

::: layout/base/nsDisplayList.cpp
@@ +896,5 @@
>        nsIFrame *child = childFrames.get();
>        if (child->Combines3DTransformWithAncestors()) {
>          mFramesMarkedForDisplay.AppendElement(child);
> +        nsRect dirty;
> +        if (!nsDisplayTransform::UntransformRect(aDirtyRect,

After re-study the code, I can do it in a simpler way.

	Thinker Li [:sinker] Assignee
	Comment 61 • 9 years ago

--
Attachment #8562092 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 62 • 9 years ago

Comment on attachment 8661602 [details] [diff] [review]
Part 2: Snap translation for 3d transforms, v2

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

::: gfx/layers/Layers.cpp
@@ +625,5 @@
>    }
> +
> +  if(aTransform.IsSingular() ||
> +     (aTransform._14 != 0 || aTransform._24 != 0 || aTransform._34 != 0)) {
> +    // For a singular transform, there is no reversed matrix, so we

A tiny change to stop snapping for perspective transforms since they are non-linear.

	Thinker Li [:sinker] Assignee
	Comment 63 • 9 years ago

--
Attachment #8656998 [details] [diff] - Attachment is obsolete: true

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

Comment on attachment 8661604 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v8

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

::: gfx/layers/Layers.cpp
@@ +1999,5 @@
>  
> +bool
> +Layer::IsBackfaceHidden()
> +{
> +  if (GetContentFlags() & CONTENT_BACKFACE_HIDDEN) {

Where did the definition of CONTENT_BACKFACE_HIDDEN go?

	Thinker Li [:sinker] Assignee
	Comment 65 • 9 years ago

Comment on attachment 8661604 [details] [diff] [review]
Part 6: Handle preserves-3d by compositor, v8

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

rebasing & a minor change.

::: layout/base/nsDisplayList.cpp
@@ +937,1 @@
>          child->Properties().Set(nsDisplayListBuilder::Preserve3DDirtyRectProperty(),

Just remove changes here.  With reminding from matt, I find it could be removed.

	Thinker Li [:sinker] Assignee
	Comment 66 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> Comment on attachment 8661604 [details] [diff] [review]
> Part 6: Handle preserves-3d by compositor, v8
> 
> Review of attachment 8661604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.cpp
> @@ +1999,5 @@
> >  
> > +bool
> > +Layer::IsBackfaceHidden()
> > +{
> > +  if (GetContentFlags() & CONTENT_BACKFACE_HIDDEN) {
> 
> Where did the definition of CONTENT_BACKFACE_HIDDEN go?

Sorry!  I moved it to part 4 for an accident during rebasing.  I would move it back.

	Thinker Li [:sinker] Assignee
	Comment 67 • 9 years ago

--
Attachment #8573756 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 68 • 9 years ago

--
Attachment #8569044 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 69 • 9 years ago

--
Attachment #8575234 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 70 • 9 years ago

--
Attachment #8661604 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 71 • 9 years ago

--
Attachment #8661665 [details] [diff] - Attachment is obsolete: true

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

Comment on attachment 8661602 [details] [diff] [review]
Part 2: Snap translation for 3d transforms, v2

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

::: gfx/layers/Layers.cpp
@@ +625,3 @@
>    }
> +
> +  if(aTransform.IsSingular() ||

if (

	Thinker Li [:sinker] Assignee
	Comment 73 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7043a9842bd5

	Thinker Li [:sinker] Assignee
	Comment 74 • 9 years ago

--
Attachment #8569040 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 75 • 9 years ago

--
Attachment #8661602 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 76 • 9 years ago

--
Attachment #8661668 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 77 • 9 years ago

--
Attachment #8661669 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 78 • 9 years ago

--
Attachment #8661670 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 79 • 9 years ago

--
Attachment #8661677 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 80 • 9 years ago

please checkin part 1 ~ part 6.  Thanks!

	Pulsebot
	Comment 81 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/47d056b3af7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec6364b87d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f03a776042
https://hg.mozilla.org/integration/mozilla-inbound/rev/727ebd9f744a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdaf4cfa6707
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf63ce1ba88

	Carsten Book [:Tomcat]
	Comment 82 • 9 years ago

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14265142&repo=mozilla-inbound

	Pulsebot
	Comment 83 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f436921d97f7

	Thinker Li [:sinker] Assignee
	Comment 84 • 9 years ago

--
Attachment #8662317 [details] [diff] - Attachment is obsolete: true

	Thinker Li [:sinker] Assignee
	Comment 85 • 9 years ago

Robert,
I make bigger tolerance values for this reftest on OSX.  What do you think?

	Thinker Li [:sinker] Assignee
	Comment 86 • 9 years ago

We seems don't run reftest for OSX10.10 for try?!

	Thinker Li [:sinker] Assignee
	Comment 87 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=613a8c5dae00

	Thinker Li [:sinker] Assignee
	Comment 88 • 9 years ago

reftest is fixed.  Please checkin all patches.  Thank you!

	Pulsebot
	Comment 89 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/37cc5cf46da4
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc43e93fb4c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/84031c74b6fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce91e635e21a
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6a976ec849
https://hg.mozilla.org/integration/mozilla-inbound/rev/621ab19e86db

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 90 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/37cc5cf46da4
https://hg.mozilla.org/mozilla-central/rev/dc43e93fb4c2
https://hg.mozilla.org/mozilla-central/rev/84031c74b6fd
https://hg.mozilla.org/mozilla-central/rev/ce91e635e21a
https://hg.mozilla.org/mozilla-central/rev/cb6a976ec849
https://hg.mozilla.org/mozilla-central/rev/621ab19e86db

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 91 • 9 years ago

This is a lot of fairly bad regressions, including several performance issues. It was also a big change to land on the last work day before the  merge to aurora.   

Are there tests we could add to cover the issues we're catching in aurora and nightly? 

Could you look at backing this out from aurora please? I think we haven't finished discovering the fallout from this change. And nightly may be a better place to do that, to have time to fix things. Thanks!

	Matt Woodrow (:mattwoodrow)
	Comment 92 • 9 years ago

Yes, we should backout of aurora.

It doesn't gain us much on its own (code cleanup primarily), so there's no need to rush into shipping it.

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

What Matt said.

	David Baron :dbaron:
	Comment 94 • 9 years ago

Backed out of aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d8e196388b2

	Thinker Li [:sinker] Assignee
	Comment 95 • 9 years ago

Right!  backing out is fair.  Although, I already have patches, it is too rush for now.

	Justin D'Arcangelo [:justindarc]
	Comment 96 • 9 years ago

This has also caused a breakage in the Gaia Music app. See Bug 1210470.

	Alfred Kayser
	Comment 97 • 9 years ago

Could this have caused 1211363?

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 98 • 9 years ago

this is now on aurora- I see it as part of talos numbers- just confirming we want it there?

	Matt Woodrow (:mattwoodrow)
	Comment 99 • 9 years ago

Yeah, I think we do. The majority of the regressions have been fixed, and thinker is working on fixing the remainder.

	Daniel Holbert [:dholbert]
	Comment 100 • 9 years ago

Given that this bug's patch causes a significant regression which can't easily be fixed on Firefox 44 branch [1], could we back this out from Firefox 44, to give ourselves more time to iron out issues here without shipping usability regressions in release builds?

[1] (e.g. regression bug 1208673, whose fix we can't backport because it causes additional regressions, per bug 1208673 comment 41 - 43)

	Thinker Li [:sinker] Assignee
	Comment 101 • 9 years ago

Yes!

	Ryan VanderMeulen [:RyanVM]
	Comment 102 • 9 years ago

Backed out from Aurora44 as well per comments 100 & 101.

https://hg.mozilla.org/releases/mozilla-aurora/rev/1deef50d1397

	Ryan VanderMeulen [:RyanVM]
	Comment 103 • 9 years ago

Merge of the backout to b2g44: https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1deef50d1397

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 104 • 9 years ago

[Tracking Requested - why for this release]: This was confusing enough for 43 and 44 that I'm suggesting tracking to follow up on all its pieces and related bugs for 45+ to make sure we don't miss anything.

	Thinker Li [:sinker] Assignee
	Comment 106 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2344fd2ab6f3

	Matt Woodrow (:mattwoodrow)
	Comment 107 • 9 years ago

Comment on attachment 8709300 [details] [diff] [review]
Backout for aurora 45

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

Hard to tell much from the diff, but looks good!

	Thinker Li [:sinker] Assignee
	Comment 108 • 9 years ago

r=mattwoodrow

	Thinker Li [:sinker] Assignee
	Comment 109 • 9 years ago

Comment on attachment 8709322 [details] [diff] [review]
Backout for aurora 45

Approval Request Comment
[Feature/regressing bug #]: 1097464
[User impact if declined]: YouTube would be broken for adding channel description.
[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2344fd2ab6f3&selectedJob=15600986
[Risks and why]: 
This is a big chunk backout, needs time for tests and user feedbacks.
[String/UUID change made/needed]: