[webkit-reviews] review granted: [Bug 49388] Share code between Mac (CA) and Windows (CACF) GraphicsLayer implementations : [Attachment 75394] Fix merge issue with last patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 49388: Share code between Mac (CA) and Windows (CACF) GraphicsLayer
implementations
https://bugs.webkit.org/show_bug.cgi?id=49388

Attachment 75394: Fix merge issue with last patch
https://bugs.webkit.org/attachment.cgi?id=75394&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> WebCore/platform/graphics/GraphicsLayer.h:51
>  typedef CALayer PlatformLayer;

I think we should do this as:
typedef CALayer* PlatformLayerRef
to avoid confusion about whether it's a pointer or not.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:258
> +PassOwnPtr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerClient* client)

> +{
> +    return new GraphicsLayerCA(client);
> +}
> +

This could be in the header.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:291
> +    // Release the clone layers inside the exception-handling block.
> +    removeCloneLayers();

The comment is no longer applicable.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1459
> +static void copyAnimationProperties(const PlatformCAAnimation* from,
PlatformCAAnimation* to)
> +{
> +    to->setBeginTime(from->beginTime());
> +    to->setDuration(from->duration());
> +    to->setRepeatCount(from->repeatCount());
> +    to->setAutoreverses(from->autoreverses());
> +    to->setFillMode(from->fillMode());
> +    to->setRemovedOnCompletion(from->isRemovedOnCompletion());
> +    to->setAdditive(from->isAdditive());
> +    to->copyTimingFunctionFrom(from);
> +
> +#if HAVE_MODERN_QUARTZCORE

I think we could have a copy constructor on PlatformCAAnimation instead, here.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1524
> +void GraphicsLayerCA::setContentsToCanvas(PlatformLayer* canvasLayer)
> +{
> +    if (m_contentsLayer && canvasLayer == m_contentsLayer->platformLayer())
> +	   return;
> +    
> +    // Create the PlatformCALayer to wrap the incoming layer
> +    m_contentsLayer = canvasLayer ? PlatformCALayer::create(canvasLayer,
this) : 0;
> +    
> +    m_contentsLayerPurpose = canvasLayer ? ContentsLayerForCanvas :
NoContentsLayer;
> +
> +    noteSublayersChanged();
> +    noteLayerPropertyChanged(ContentsCanvasLayerChanged);
> +}

Please move this so it's next to the other "setContentsTo" methods.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1762
> +
> +
> +
> +
> +	   if (isTransformTypeNumber(transformOpType)) {

Extra blank lines here.

> WebCore/platform/graphics/ca/PlatformCAAnimation.h:63
> +    static PassRefPtr<PlatformCAAnimation> create(void* animation);

CAPropertyAnimation rather than void*? Or maybe you have to have a typedef that
maps to CAPropertyAnimation on Mac, and CACFAnimationRef on Windows?

> WebCore/platform/graphics/ca/PlatformCAAnimation.h:69
> +    void* platformAnimation() const;

Why not return a CAPropertyAnimation?

> WebCore/platform/graphics/ca/PlatformCALayer.h:50
> +    // FIXME: This object represents all Layer types used in GraphicsLayer,
and those used by video and canvas.

Why is that a FIXME?

> WebCore/platform/graphics/ca/PlatformCALayer.h:65
> +    // FIXME: turn off implicit animation in ctor

Either do that, or remove the FIXME.
 

r=me with those comments addressed.