TextureMapper: Simplify transform manipulations.
Created attachment 119613 [details] Patch
Comment on attachment 119613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119613&action=review > Source/WebCore/ChangeLog:9 > + - Make sure that the replica node has a complete transform and > + use it directly instead of keeping a copy in the source. I can't see how this code would work; Since the replica's parent layer is always null, and you don't check for replicatedLayer. Someone needs to combine the "regular" layer's transform and the replica's local transform. That code was pretty tricky and is tested by many layout tests (compositing/reflection). I see no mention in this changelog that those were tested before+after the changes, which makes me think that these changes would break those reflection tests :) > Source/WebCore/ChangeLog:18 > + - Fix the fillsForward transform adjustment in syncAnimations and > + make the intention clearer by using setTransform and setOpacity. Different patch.
Created attachment 122255 [details] Patch
Comment on attachment 122255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122255&action=review Good direction, needs some improvements. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69 > +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix& parentTransform) I don't see why we need to functions. We should just have one function, and use (parent || effectTarget). > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76 > + m_transforms.target = parentTransform; > + m_transforms.target > .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() ) > - .multiply(m_transforms.base) > - .translate3d(-originX, -originY, -m_state.anchorPoint.z()); > + .multiply(m_transforms.base); You can do m_transforms.target = TransformationMatrix(parentTransform).multiply(m_transforms.base); <nitpick/> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:78 > + // In case a parent had preserves3D and this layer has not, flatten our children. We should early-return if we don't have children, after we've translated the target transform back. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:95 > + // The replica layer isn't parented and needs its transform to be stacked atop ours. > + if (m_state.replicaLayer) > + m_state.replicaLayer->computeTransformsSelf(m_transforms.target); Instead, you can call effectTarget from within the replica layer if there's no parent.
(In reply to comment #4) > (From update of attachment 122255 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122255&action=review > > Good direction, needs some improvements. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69 > > +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix& parentTransform) > > I don't see why we need to functions. We should just have one function, and use (parent || effectTarget). > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76 > > + m_transforms.target = parentTransform; > > + m_transforms.target > > .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() ) > > - .multiply(m_transforms.base) > > - .translate3d(-originX, -originY, -m_state.anchorPoint.z()); > > + .multiply(m_transforms.base); > > You can do > m_transforms.target = TransformationMatrix(parentTransform).multiply(m_transforms.base); > <nitpick/> I mean m_transforms.target = TransformationMatrix(parentTransform) .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(), m_state.anchorPoint.z() ) .multiply(m_transforms.base);
Comment on attachment 122255 [details] Patch After discussing this on IRC, I believe that my comments actually don't make this patch any better.
Created attachment 122421 [details] Patch Here, it should be on the right bug. Tried to use webkit-patch upload -g SHA^..SHA, but I guess I'll stick with -g HEAD..
Committed r104931: <http://trac.webkit.org/changeset/104931>