[webkit-reviews] review denied: [Bug 65708] Logic to compute visible display rect in GraphicsLayerCA::syncCompositingState : [Attachment 102952] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 4 12:49:41 PDT 2011
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 65708: Logic to compute visible display rect in
GraphicsLayerCA::syncCompositingState
https://bugs.webkit.org/show_bug.cgi?id=65708
Attachment 102952: Patch
https://bugs.webkit.org/attachment.cgi?id=102952&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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
> + at interface CATiledLayer(GraphicsLayerCAPrivate)
> +- (void)displayInRect:(CGRect)r levelOfDetail:(int)lod options:(NSDictionary
*)dict;
> +- (BOOL)canDrawConcurrently;
> +- (void)setCanDrawConcurrently:(BOOL)flag;
> + at 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?
More information about the webkit-reviews
mailing list