Closed Bug 788044 Opened 7 years ago Closed 7 years ago

Make inactive layer tree coordinates relative to the ThebesLayer

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 3 open bugs)

Details

Attachments

(4 files, 7 obsolete files)

7.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
46.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
38.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
This makes handling scrolling with DLBI much easier.

Patch seems to work with my testcases, still need to push to try though.
Attachment #657958 - Flags: review?(roc)
Comment on attachment 657958 [details] [diff] [review]
Make transformed frames the reference frame for their display list tree

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

You should rename nsDisplayListBuilder::ToReferenceFrame to ToRootReferenceFrame.

::: layout/base/nsDisplayList.cpp
@@ +949,5 @@
>    nsRefPtr<LayerManager> layerManager;
>    bool allowRetaining = false;
>    bool doBeginTransaction = true;
>    if (aFlags & PAINT_USE_WIDGET_LAYERS) {
> +    nsIFrame* referenceFrame = aBuilder->GetRootReferenceFrame();

rootReferenceFrame

@@ +3459,5 @@
> +  gfxPoint scaledOffset(
> +      NSAppUnitsToDoublePixels(offset.x, appUnitsPerDevPixel)*aContainerParameters.mXScale,
> +      NSAppUnitsToDoublePixels(offset.y, appUnitsPerDevPixel)*aContainerParameters.mYScale);
> +  newTransformMatrix.Translate(gfxPoint3D(scaledOffset.x, 
> +                                          scaledOffset.y, 0));

I'm not sure what the purpose of this is. Comment?

::: layout/base/nsDisplayList.h
@@ +164,5 @@
>    /**
>     * @return the root of the display list's frame (sub)tree, whose origin
>     * establishes the coordinate system for the display list
>     */
> +  nsIFrame* FindReferenceFrameFor(nsIFrame *aFrame)

Move comment to the right place

@@ +171,5 @@
> +    while (f) {
> +      if (f->IsTransformed()) {
> +        return f;
> +      }
> +      f = nsLayoutUtils::GetCrossDocParentFrame(f);

more elegant as a for loop

@@ +176,5 @@
> +    }
> +    return mReferenceFrame;
> +  }
> +  const nsIFrame* FindReferenceFrameFor(const nsIFrame *aFrame)
> +  {

prefer just one implementation and some const-casting

@@ +186,5 @@
> +      f = nsLayoutUtils::GetCrossDocParentFrame(f);
> +    }
> +    return mReferenceFrame;
> +  }
> +  nsIFrame* GetRootReferenceFrame() 

RootReferenceFrame(), since it can't be null

@@ +643,5 @@
>    }
>    nsDisplayItem(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>                  const nsPoint& aToReferenceFrame)
>      : mFrame(aFrame)
>      , mToReferenceFrame(aToReferenceFrame)

Seems like we need to initialize mReferenceFrame in this constructor. Why didn't this break the world?

We should initialize it using the nsDisplayListBuilder's cache so we don't have to walk the frame hierarchy to the top every single time we construct an item.

@@ +960,5 @@
>    }
>  
>    nsIFrame* mFrame;
>    // Result of ToReferenceFrame(mFrame), if mFrame is non-null
> +  nsIFrame* mReferenceFrame;

Fix comment!
Comment on attachment 657959 [details] [diff] [review]
Add an offset to nsDisplayOpacity layer trees so that they don't move when scrolling

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

Split this patch into a patch that adds the offset capability and another patch that uses it.

Why just nsDisplayOpacity? Shouldn't we do this for all inactive layers?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
 
> Why just nsDisplayOpacity? Shouldn't we do this for all inactive layers?

This change makes the ContainerLayer created by nsDisplayOpacity contain the scroll-position relative translation and not any of the child layers.

None of the other inactive layer types (except transform, which is already handled), create ContainerLayers with children, so there would be change.
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> This change makes the ContainerLayer created by nsDisplayOpacity contain the
> scroll-position relative translation and not any of the child layers.
> 
> None of the other inactive layer types (except transform, which is already
> handled), create ContainerLayers with children, so there would be change.

What about LAYER_SVG_EFFECTS?
Fixed the review comments, and fixed most of the bugs.

The remaining issue is to do without rounding app units to pixels in nsDisplayTransform::GetResultingTransformMatrix. The current code passes all but a few svg tests.
Attachment #657958 - Attachment is obsolete: true
Attachment #657958 - Flags: review?(roc)
Attachment #660689 - Flags: review?(roc)
Attachment #660277 - Attachment is obsolete: true
Attachment #660690 - Flags: review?(roc)
Attachment #657959 - Attachment is obsolete: true
Attachment #657959 - Flags: review?(roc)
Attachment #660691 - Flags: review?(roc)
Attachment #660693 - Attachment is patch: true
Depends on: 790859
Filed bug 790859 for the test_transformed_scrolling_repaints_2.html change.
Comment on attachment 660690 [details] [diff] [review]
Make transformed frames the reference frame for their display list tree v3

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

::: layout/base/nsDisplayList.cpp
@@ +3365,5 @@
> +                     0);
> +
> +  if (!sRecursing) {
> +    result.Translate(rounded);
> +  }

This sRecursing stuff desperately needs a comment.

Can't we avoid it by passing an extra parameter to GetResultingTransformMatrix?

::: layout/base/nsDisplayList.h
@@ +162,5 @@
>    nsISelection* GetBoundingSelection() { return mBoundingSelection; }
> +
> +  /**
> +   * @return the root of given frame's (sub)tree, whose origin
> +   * establishes the coordinate system for the child display items.

Can you modify the big comment in nsDisplayList.h to explain about multiple reference frames?

::: layout/ipc/RenderFrameParent.cpp
@@ +875,5 @@
>        new (aBuilder) nsDisplayRemote(aBuilder, aFrame, this));
>    }
>  
>    // Clip the shadow layers to subdoc bounds
> +  nsPoint offset = aFrame->GetOffsetToCrossDoc(aBuilder->FindReferenceFrameFor(aFrame));

Why can't this be aBuilder->ToReferenceFrame(aFrame)?
Comment on attachment 660691 [details] [diff] [review]
Add infrastructure to offset layer trees

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

Why isn't nsDisplayList.h modified in this patch?

::: gfx/layers/Layers.h
@@ +1070,5 @@
>     */
>    void SetAllowResidualTranslation(bool aAllow) { mAllowResidualTranslation = aAllow; }
>  
> +  void SetOffset(const gfxPoint& aOffset) { mOffset = aOffset; }
> +  const gfxPoint& GetOffset() { return mOffset; }

This desperately needs documentation

::: layout/base/FrameLayerBuilder.h
@@ +152,5 @@
>        mInActiveTransformedSubtree(aParent.mInActiveTransformedSubtree),
>        mDisableSubpixelAntialiasingInDescendants(aParent.mDisableSubpixelAntialiasingInDescendants)
>      {}
>      float mXScale, mYScale;
> +    gfxPoint mOffset;

So does this
Comment on attachment 660692 [details] [diff] [review]
Add an offset to nsDisplayOpacity layer trees so that they don't move when scrolling

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

::: layout/base/nsDisplayList.cpp
@@ +2415,3 @@
>    nsRefPtr<Layer> container = aManager->GetLayerBuilder()->
>      BuildContainerLayerFor(aBuilder, aManager, mFrame, this, mList,
> +                           containerParameters, &newTransformMatrix);

I feel BuildContainerLayerFor should be doing this automatically based on the containerParameters, without its callers having to be changed. Do you think that's possible?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 660690 [details] [diff] [review]
> Make transformed frames the reference frame for their display list tree v3
> 
> Review of attachment 660690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +3365,5 @@
> > +                     0);
> > +
> > +  if (!sRecursing) {
> > +    result.Translate(rounded);
> > +  }
> 
> This sRecursing stuff desperately needs a comment.
> 
> Can't we avoid it by passing an extra parameter to
> GetResultingTransformMatrix?

That's what I had originally, but it adds an extra parameter to the public API (which it desperately doesn't need) that should never be used by other users and would be confusing.

We could have a GetResultingTransformInternal but I didn't think that was an improvement over just using a static var.

> 
> ::: layout/base/nsDisplayList.h
> @@ +162,5 @@
> >    nsISelection* GetBoundingSelection() { return mBoundingSelection; }
> > +
> > +  /**
> > +   * @return the root of given frame's (sub)tree, whose origin
> > +   * establishes the coordinate system for the child display items.
> 
> Can you modify the big comment in nsDisplayList.h to explain about multiple
> reference frames?

Sure

> 
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +875,5 @@
> >        new (aBuilder) nsDisplayRemote(aBuilder, aFrame, this));
> >    }
> >  
> >    // Clip the shadow layers to subdoc bounds
> > +  nsPoint offset = aFrame->GetOffsetToCrossDoc(aBuilder->FindReferenceFrameFor(aFrame));
> 
> Why can't this be aBuilder->ToReferenceFrame(aFrame)?

Will fix.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
>Why isn't nsDisplayList.h modified in this patch?

What should it need to change there? We only need to adjust BuildLayer implementations that *don't* call BuildContainerLayerFor.

> 
> ::: gfx/layers/Layers.h
> @@ +1070,5 @@
> >     */
> >    void SetAllowResidualTranslation(bool aAllow) { mAllowResidualTranslation = aAllow; }
> >  
> > +  void SetOffset(const gfxPoint& aOffset) { mOffset = aOffset; }
> > +  const gfxPoint& GetOffset() { return mOffset; }
> 
> This desperately needs documentation
> 
> ::: layout/base/FrameLayerBuilder.h
> @@ +152,5 @@
> >        mInActiveTransformedSubtree(aParent.mInActiveTransformedSubtree),
> >        mDisableSubpixelAntialiasingInDescendants(aParent.mDisableSubpixelAntialiasingInDescendants)
> >      {}
> >      float mXScale, mYScale;
> > +    gfxPoint mOffset;
> 
> So does this

Will do!
(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> That's what I had originally, but it adds an extra parameter to the public
> API (which it desperately doesn't need) that should never be used by other
> users and would be confusing.
> 
> We could have a GetResultingTransformInternal but I didn't think that was an
> improvement over just using a static var.

I think it would be actually :-).

If we have the Internal method then we can use some overloads to prevent callers having to call GetResultingTransform with a bunch of meaningless null parameters, too.

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> >Why isn't nsDisplayList.h modified in this patch?
> 
> What should it need to change there?

Never mind, I was confused.
As you wish!

I have a patch to cleanup the parameter crazyness with GetResultingTransformMatrix. Will try get that rebased and into it's own bug asap.
Attachment #660690 - Attachment is obsolete: true
Attachment #660690 - Flags: review?(roc)
Attachment #661109 - Flags: review?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> Created attachment 661109 [details] [diff] [review]

> I have a patch to cleanup the parameter crazyness with
> GetResultingTransformMatrix. Will try get that rebased and into it's own bug
> asap.

Thank you :)
Followup (that I deserved to have to do, because I've been forgetting to unhide 10.8 jobs on Try after I green them up) in https://hg.mozilla.org/integration/mozilla-inbound/rev/029e527719c0 to mark 586683-1.html as quite fuzzy on 10.8.
This should 'just work' for all inactive container layers.
Attachment #660691 - Attachment is obsolete: true
Attachment #660692 - Attachment is obsolete: true
Attachment #660693 - Attachment is obsolete: true
Attachment #660691 - Flags: review?(roc)
Attachment #660692 - Flags: review?(roc)
Attachment #660693 - Flags: review?(roc)
Attachment #661658 - Flags: review?(roc)
Comment on attachment 661658 [details] [diff] [review]
Add infrastructure to offset layer trees v2

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

::: layout/base/FrameLayerBuilder.cpp
@@ +517,5 @@
>     */
>    float mXScale, mYScale;
>  
>    /**
> +   * The offset from 0,0 of the ThebesLayer to the reference frame.

slightly easier to read as "The offset from the ThebesLayer's 0,0 to the reference frame."

@@ +519,5 @@
>  
>    /**
> +   * The offset from 0,0 of the ThebesLayer to the reference frame.
> +   */
> +  nsIntPoint mTranslation;

You should say something about how the ThebesLayer's transform is set up, and why it's different from mTranslation.

::: layout/base/FrameLayerBuilder.h
@@ +153,5 @@
>        mInActiveTransformedSubtree(aParent.mInActiveTransformedSubtree),
>        mDisableSubpixelAntialiasingInDescendants(aParent.mDisableSubpixelAntialiasingInDescendants)
>      {}
>      float mXScale, mYScale;
> +    nsIntPoint mOffset;

This needs to be documented.
Attachment #661658 - Flags: review?(roc) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Depends on: 792032
Depends on: 792051
No longer depends on: 792032
Depends on: 792907
Depends on: 793132
Assignee: nobody → matt.woodrow
Depends on: 793629
https://hg.mozilla.org/mozilla-central/rev/fa9d4c87e6de

Should this have a test?
Flags: in-testsuite?
Depends on: 794103
Depends on: 794396
Depends on: 794686
Target Milestone: --- → mozilla18
Just to note that simply setting a transform is enough to trigger this bug under many circumstances, there's no need for the nested divs, position:fixed, etc. This is a regression caused by bug 788044.

I've spoken to Matt Woodrow about it - if DLBI sticks, we should back this out of Aurora, otherwise we'll devise and check in a fix.
Ignore comment #31, wrong bug.
Depends on: 795837
Depends on: 795859
No longer depends on: 795859
Depends on: 800041
Depends on: 800483
Depends on: 800135
Depends on: 800480
Depends on: 808486
Depends on: 827577
Depends on: 829623
Depends on: 829921
Depends on: 849996
Depends on: 1028369
Depends on: 1259787
You need to log in before you can comment on or make changes to this bug.