Closed
Bug 1097464
Opened 10 years ago
Closed 9 years ago
Move preserve-3d handling into the compositor
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
People
(Reporter: roc, Assigned: sinker)
References
(Depends on 3 open bugs, Blocks 2 open bugs)
Details
(Whiteboard: [webvr][vrm2])
Attachments
(7 files, 30 obsolete files)
878 bytes,
patch
|
Details | Diff | Splinter Review | |
4.98 KB,
patch
|
Details | Diff | Splinter Review | |
3.13 KB,
patch
|
Details | Diff | Splinter Review | |
29.59 KB,
patch
|
Details | Diff | Splinter Review | |
116.35 KB,
patch
|
Details | Diff | Splinter Review | |
13.71 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
256.44 KB,
patch
|
sinker
:
review+
|
Details | Diff | Splinter Review |
Comment hidden (obsolete) |
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.
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
Attachment #8537473 -
Attachment is obsolete: true
Attachment #8548121 -
Flags: feedback?(roc)
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.
Attachment #8548121 -
Flags: feedback?(roc) → feedback-
Reporter | ||
Comment 4•10 years ago
|
||
Also, have you tested this with async-animations of preserve-3d transforms?
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.
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.
Flags: needinfo?(tlee)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(tlee)
Attachment #8562091 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8562092 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8562093 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8562094 -
Flags: review?(roc)
Assignee | ||
Comment 11•10 years ago
|
||
David, could you give me the bug of desktop APZ?
(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.
Reporter | ||
Comment 13•10 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"?
Attachment #8562091 -
Flags: review?(roc)
Attachment #8562091 -
Flags: review?(matt.woodrow)
Attachment #8562091 -
Flags: review+
Reporter | ||
Comment 14•10 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"
Attachment #8562092 -
Flags: review?(roc) → review+
Comment 15•10 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.
Reporter | ||
Comment 16•10 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.
Attachment #8562094 -
Flags: review?(roc)
Comment 17•10 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
Assignee | ||
Updated•10 years ago
|
Attachment #8562094 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8562094 -
Flags: review?(roc)
Updated•10 years ago
|
Attachment #8562091 -
Flags: review?(matt.woodrow) → review-
Assignee | ||
Comment 18•10 years ago
|
||
--
Attachment #8562091 [details] [diff] - Attachment is obsolete: true
Attachment #8569040 -
Flags: review?(roc)
Attachment #8569040 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 19•10 years ago
|
||
--
Attachment #8562093 [details] [diff] - Attachment is obsolete: true
Attachment #8569041 -
Flags: review?(roc)
Assignee | ||
Comment 20•10 years ago
|
||
--
Attachment #8562094 [details] [diff] - Attachment is obsolete: true
Attachment #8569042 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8562091 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8562094 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8569044 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8562093 -
Attachment is obsolete: true
Attachment #8562093 -
Flags: review?(roc)
Reporter | ||
Updated•10 years ago
|
Attachment #8569040 -
Flags: review?(roc) → review+
Updated•10 years ago
|
Attachment #8569040 -
Flags: review?(matt.woodrow) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8569041 -
Flags: review?(roc) → review+
Reporter | ||
Updated•10 years ago
|
Attachment #8569044 -
Flags: review?(roc) → review+
Reporter | ||
Comment 22•10 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."
Reporter | ||
Comment 23•10 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 :-)
Attachment #8569042 -
Flags: review?(roc) → review+
Reporter | ||
Comment 24•10 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
Attachment #8569042 -
Flags: review+
Assignee | ||
Comment 25•10 years ago
|
||
--
Attachment #8569041 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
--
Attachment #8569042 [details] [diff] - Attachment is obsolete: true
Attachment #8573757 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8569041 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8569042 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8573758 -
Flags: review?(roc)
Assignee | ||
Comment 28•10 years ago
|
||
--
Attachment #8573758 [details] [diff] - Attachment is obsolete: true
Attachment #8573761 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8573758 -
Attachment is obsolete: true
Attachment #8573758 -
Flags: review?(roc)
Assignee | ||
Comment 29•10 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.
Reporter | ||
Comment 30•10 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?
Attachment #8573757 -
Flags: review?(roc)
Reporter | ||
Comment 31•10 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.
Assignee | ||
Comment 32•10 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.
Assignee | ||
Comment 33•10 years ago
|
||
Assignee | ||
Comment 34•10 years ago
|
||
--
Attachment #8573757 [details] [diff] - Attachment is obsolete: true
Attachment #8575236 -
Flags: review?(roc)
Assignee | ||
Comment 35•10 years ago
|
||
--
Attachment #8573761 [details] [diff] - Attachment is obsolete: true
Attachment #8575237 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8573757 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8573761 -
Attachment is obsolete: true
Attachment #8573761 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8575234 -
Flags: review?(roc)
Reporter | ||
Comment 36•10 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"?
Attachment #8575234 -
Flags: review?(roc)
Reporter | ||
Comment 37•10 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().
Attachment #8575236 -
Flags: review?(roc)
Assignee | ||
Comment 38•10 years ago
|
||
--
Attachment #8575236 [details] [diff] - Attachment is obsolete: true
Attachment #8606966 -
Flags: review?(roc)
Assignee | ||
Updated•10 years ago
|
Attachment #8575236 -
Attachment is obsolete: true
Assignee | ||
Comment 39•10 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.
Reporter | ||
Comment 40•10 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
Attachment #8606966 -
Flags: review?(roc) → review-
Reporter | ||
Comment 41•10 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.
Attachment #8575237 -
Flags: review?(roc)
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.
Summary: Remove BasicLayers flattening → Move preserve-3d handling into the compositor
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.
Assignee | ||
Comment 44•9 years ago
|
||
--
Attachment #8606966 [details] [diff] - Attachment is obsolete: true
Attachment #8624631 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8606966 -
Attachment is obsolete: true
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
Attachment #8624631 -
Flags: review?(roc) → review-
Updated•9 years ago
|
Whiteboard: [webvr]
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.
Flags: needinfo?(tlee)
Updated•9 years ago
|
Whiteboard: [webvr] → [webvr][vrm2]
Updated•9 years ago
|
Assignee: nobody → kgilbert
Updated•9 years ago
|
Blocks: fuzzy-text
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.
Flags: needinfo?(tlee)
Comment 48•9 years ago
|
||
Thanks, Thinker! It is looking great so far. Please ask any time if i can help with anything.
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.
Blocks: 1168263
Assignee | ||
Comment 50•9 years ago
|
||
Hi Matt, I would upload another patch in this week.
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()
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.
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
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.
Assignee | ||
Comment 55•9 years ago
|
||
--
Attachment #8624631 [details] [diff] - Attachment is obsolete: true
Attachment #8656998 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8624631 -
Attachment is obsolete: true
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.
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.
Attachment #8656998 -
Flags: review?(roc) → review+
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?
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
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.
Assignee | ||
Comment 61•9 years ago
|
||
--
Attachment #8562092 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8562092 -
Attachment is obsolete: true
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.
Assignee | ||
Comment 63•9 years ago
|
||
--
Attachment #8656998 [details] [diff] - Attachment is obsolete: true
Attachment #8661604 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8656998 -
Attachment is obsolete: true
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?
Attachment #8661604 -
Flags: review?(roc) → review+
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.
Attachment #8661604 -
Flags: review+ → review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8661604 -
Flags: review?(roc) → review+
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8661602 -
Flags: review?(roc)
Assignee | ||
Comment 67•9 years ago
|
||
--
Attachment #8573756 [details] [diff] - Attachment is obsolete: true
Attachment #8661665 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8573756 -
Attachment is obsolete: true
Assignee | ||
Comment 68•9 years ago
|
||
--
Attachment #8569044 [details] [diff] - Attachment is obsolete: true
Attachment #8661668 -
Flags: review?(roc)
Assignee | ||
Comment 69•9 years ago
|
||
--
Attachment #8575234 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 70•9 years ago
|
||
--
Attachment #8661604 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8569044 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8575234 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8661604 -
Attachment is obsolete: true
Assignee | ||
Comment 71•9 years ago
|
||
--
Attachment #8661665 [details] [diff] - Attachment is obsolete: true
Attachment #8661677 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8661665 -
Attachment is obsolete: true
Attachment #8661665 -
Flags: review?(roc)
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 (
Attachment #8661602 -
Flags: review?(roc) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8661668 -
Flags: review?(roc) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8661677 -
Flags: review?(roc) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8575237 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8548121 -
Attachment is obsolete: true
Assignee | ||
Comment 73•9 years ago
|
||
Assignee | ||
Comment 74•9 years ago
|
||
--
Attachment #8569040 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 75•9 years ago
|
||
--
Attachment #8661602 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 76•9 years ago
|
||
--
Attachment #8661668 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 77•9 years ago
|
||
--
Attachment #8661669 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 78•9 years ago
|
||
--
Attachment #8661670 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Comment 79•9 years ago
|
||
--
Attachment #8661677 [details] [diff] - Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8569040 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8661602 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8661668 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8661669 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8661670 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8661677 -
Attachment is obsolete: true
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
Keywords: checkin-needed
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
Flags: needinfo?(kgilbert)
Comment 83•9 years ago
|
||
Assignee: kgilbert → tlee
Flags: needinfo?(kgilbert) → needinfo?(tlee)
Assignee | ||
Comment 84•9 years ago
|
||
--
Attachment #8662317 [details] [diff] - Attachment is obsolete: true
Attachment #8662430 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8662317 -
Attachment is obsolete: true
Assignee | ||
Comment 85•9 years ago
|
||
Robert,
I make bigger tolerance values for this reftest on OSX. What do you think?
Flags: needinfo?(tlee)
Assignee | ||
Comment 86•9 years ago
|
||
We seems don't run reftest for OSX10.10 for try?!
Assignee | ||
Comment 87•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8662430 -
Flags: review?(roc) → review+
Assignee | ||
Comment 88•9 years ago
|
||
reftest is fixed. Please checkin all patches. Thank you!
Keywords: checkin-needed
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
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1207143
Updated•9 years ago
|
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!
Flags: needinfo?(tlee)
Flags: needinfo?(roc)
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.
Backed out of aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d8e196388b2
status-firefox44:
--- → fixed
Target Milestone: mozilla43 → mozilla44
Assignee | ||
Comment 95•9 years ago
|
||
Right! backing out is fair. Although, I already have patches, it is too rush for now.
Flags: needinfo?(tlee)
Comment 96•9 years ago
|
||
This has also caused a breakage in the Gaia Music app. See Bug 1210470.
Blocks: 1210470
Updated•9 years ago
|
Updated•9 years ago
|
Comment 97•9 years ago
|
||
Could this have caused 1211363?
Comment 98•9 years ago
|
||
this is now on aurora- I see it as part of talos numbers- just confirming we want it there?
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
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)
Flags: needinfo?(tlee)
Comment 102•9 years ago
|
||
Backed out from Aurora44 as well per comments 100 & 101.
https://hg.mozilla.org/releases/mozilla-aurora/rev/1deef50d1397
status-firefox45:
--- → fixed
Target Milestone: mozilla44 → mozilla43
Comment 103•9 years ago
|
||
Merge of the backout to b2g44: https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1deef50d1397
status-b2g-v2.5:
--- → disabled
status-b2g-master:
--- → fixed
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.
Updated•9 years ago
|
Updated•9 years ago
|
Assignee | ||
Comment 105•9 years ago
|
||
Assignee | ||
Comment 106•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8709300 -
Flags: review?(matt.woodrow)
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!
Attachment #8709300 -
Flags: review?(matt.woodrow) → review+
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]:
Attachment #8709322 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8709322 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 110•9 years ago
|
||
https://treeherder.mozilla.org/logviewer.html#?job_id=15601267&repo=try
Matt, how do you think about these test fails? I think they seems the problems of layout, out of the scope of our changes?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 111•9 years ago
|
||
Fix reftest.list
Attachment #8709300 -
Attachment is obsolete: true
Attachment #8709322 -
Attachment is obsolete: true
Attachment #8709345 -
Flags: review+
Comment 112•9 years ago
|
||
(In reply to Thinker Li [:sinker] from comment #110)
> https://treeherder.mozilla.org/logviewer.html#?job_id=15601267&repo=try
>
> Matt, how do you think about these test fails? I think they seems the
> problems of layout, out of the scope of our changes?
Yeah, I can't see how our changes would cause those failures.
Flags: needinfo?(matt.woodrow)
So, bug 1168263 landed in 45 and my understanding is that it depended on this bug to work properly. I could be wrong, but if that's the case, is backing this out of 45 going to break stuff?
Flags: needinfo?(tlee)
Flags: needinfo?(matt.woodrow)
RyanVM informed me that the backout already landed on aurora, at https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d, as part of bug 1240783. Except the cset makes no mention of bug 1240783 anywhere.
In the future if you're doing a backout as a separate bug please list the bug number of the new bug in the cset, and please comment on ALL of the relevant bugs that it backs out. The only thing worse than landing a pile of crap in the tree is leaving incorrect state in all the bugs that it affects, so nobody can figure out what is going on. To be clear: all of the bugs that were backed out should have their status-firefox45 flag set to "disabled", and a link to the aurora cset should be posted as a comment.
Note also that bug 1168263 is an APZ blocker, so if any of these bugs gets further backed out of 46, APZ will not ship in 46, and that will make me and a lot of people very unhappy. Considering e10s is also hitched to APZ, this is directly on the critical path for e10s, so please sort it out ASAP.
Updated•9 years ago
|
Updated•9 years ago
|
Flags: needinfo?(matt.woodrow)
You need to log in
before you can comment on or make changes to this bug.