Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState
Created attachment 102952 [details] Patch
Comment on attachment 102952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102952&action=review I wish we could test this; I see a number of issues (not taking perspective into account, possible scrolling issues). > Source/WebCore/platform/graphics/GraphicsLayer.h:368 > + virtual void syncCompositingState(const FloatRect&) { } I think it's worth naming the parameter in a C-style comment. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:836 > + state.translate(-anchorPoint().x(), -anchorPoint().y(), -anchorPoint().z(), accumulation); > + state.applyTransform(transform(), accumulation); > + state.translate(anchorPoint().x(), anchorPoint().y(), anchorPoint().z(), accumulation); Rather than having to add TransformState::translate() for this, can't you just compute the transform taking the anchor point into account, then apply it? Also, you're ignoring childTransform here. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:842 > + if (masksToBounds()) { > + // Replace the quad in the TransformState with one that is clipped to this layer's bounds I think you might want to assert here that accumulation == FlattenTransform, otherwise state.lastPlanarQuad might be in some ancestor's coordinate system. > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:96 > +@interface CATiledLayer(GraphicsLayerCAPrivate) > +- (void)displayInRect:(CGRect)r levelOfDetail:(int)lod options:(NSDictionary *)dict; > +- (BOOL)canDrawConcurrently; > +- (void)setCanDrawConcurrently:(BOOL)flag; > +@end You should #ifdef this for not Leopard and not SnowLeopard. > Source/WebCore/platform/graphics/transforms/TransformState.cpp:62 > +void TransformState::translate(float x, float y, float z, TransformAccumulation accumulate) I don't think this is necessary. > Source/WebCore/rendering/RenderLayerCompositor.cpp:210 > + if (GraphicsLayer* rootLayer = rootGraphicsLayer()) { > + FrameView* frameView = m_renderView ? m_renderView->frameView() : 0; > + if (frameView) > + rootLayer->syncCompositingState(frameView->frameRect()); > + } Is frameRect() the right thing? How is scrolling taken into account?
(In reply to comment #2) > (From update of attachment 102952 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102952&action=review > > I wish we could test this; I see a number of issues (not taking perspective into account, possible scrolling issues). > > > Source/WebCore/platform/graphics/GraphicsLayer.h:368 > > + virtual void syncCompositingState(const FloatRect&) { } > > I think it's worth naming the parameter in a C-style comment. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:836 > > + state.translate(-anchorPoint().x(), -anchorPoint().y(), -anchorPoint().z(), accumulation); > > + state.applyTransform(transform(), accumulation); > > + state.translate(anchorPoint().x(), anchorPoint().y(), anchorPoint().z(), accumulation); > > Rather than having to add TransformState::translate() for this, can't you just compute the transform taking the anchor point into account, then apply it? I will do that... > > Also, you're ignoring childTransform here. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:842 > > + if (masksToBounds()) { > > + // Replace the quad in the TransformState with one that is clipped to this layer's bounds > > I think you might want to assert here that accumulation == FlattenTransform, otherwise state.lastPlanarQuad might be in some ancestor's coordinate system. Will do > > > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:96 > > +@interface CATiledLayer(GraphicsLayerCAPrivate) > > +- (void)displayInRect:(CGRect)r levelOfDetail:(int)lod options:(NSDictionary *)dict; > > +- (BOOL)canDrawConcurrently; > > +- (void)setCanDrawConcurrently:(BOOL)flag; > > +@end > > You should #ifdef this for not Leopard and not SnowLeopard. Ok > > > Source/WebCore/platform/graphics/transforms/TransformState.cpp:62 > > +void TransformState::translate(float x, float y, float z, TransformAccumulation accumulate) > > I don't think this is necessary. I'll make the change above > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:210 > > + if (GraphicsLayer* rootLayer = rootGraphicsLayer()) { > > + FrameView* frameView = m_renderView ? m_renderView->frameView() : 0; > > + if (frameView) > > + rootLayer->syncCompositingState(frameView->frameRect()); > > + } > > Is frameRect() the right thing? How is scrolling taken into account? For WK2, scrolling in a GraphicsLayer below this, so it should handle it correctly. This is the area I have to change for iOS, since it is like WK1, which does scrolling differently.
Created attachment 102994 [details] Patch
Comment on attachment 102994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102994&action=review > Source/WebCore/ChangeLog:6 > + This is in support of <rdar://problem/9706222> and <rdar://problem/9434765> No need to mention radar numbers. > Source/WebCore/page/FrameView.cpp:709 > + fullScreenLayer->syncCompositingState(frameRect()); Please add a comment that this only works on platforms where RLC uses a scroll layer. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:831 > +void GraphicsLayerCA::recursiveCommitChanges(TransformState& state, float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool affectedByPageScale) > { > + TransformState::TransformAccumulation accumulation = preserves3D() ? TransformState::AccumulateTransform : TransformState::FlattenTransform; > + state.move(m_position.x(), m_position.y(), accumulation); This code needs to maintain stack of TransformStates, otherwise your state is wrong when you come back out of children.
Created attachment 103294 [details] Patch
Comment on attachment 103294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103294&action=review r=me but please do the localState thing. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:831 > -void GraphicsLayerCA::recursiveCommitChanges(float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool affectedByPageScale) > +void GraphicsLayerCA::recursiveCommitChanges(TransformState& state, float pageScaleFactor, const FloatPoint& positionRelativeToBase, bool affectedByPageScale) > { > + // Save the state before sending down to kids and restore it after > + TransformState savedTransformState = state; The more natural way to do this would be to have TransformState localState = state; and pass localState to kids. Then your TransformState& param can be const.
Committed r92651: <http://trac.webkit.org/changeset/92651>