This makes handling scrolling with DLBI much easier. Patch seems to work with my testcases, still need to push to try though.

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.

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee3959754f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee831a85d12 Landed the first two parts.

(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.

https://hg.mozilla.org/mozilla-central/rev/0ee3959754f8 https://hg.mozilla.org/mozilla-central/rev/6ee831a85d12 https://hg.mozilla.org/mozilla-central/rev/029e527719c0

This should 'just work' for all inactive container layers.

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.

https://hg.mozilla.org/mozilla-central/rev/fa9d4c87e6de Should this have a test?

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.