Bug 23356 – Land new files in WebCore/platform related to accelerated compositing

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Bug to track the landing of new files in platform/ related to accelerate compositing.

[Simon Fraser (smfr)]

Created attachment 26773 [details]
Patch, changelog

[Sam Weinig]

Comment on attachment 26773 [details]
Patch, changelog

In my first pass, I just commented on style issue.

General issues:
- We like using the 2 clause BSD license for new code.
- I am not a fan of the WK prefix, especially in WebCore.  I would prefer using "WebCore", so, WKLayer would become WebCoreLayer.


> +/*
> + * Copyright (C) 2009 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer. 
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution. 
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission. 
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

We prefer to use a 2 clause license these days.

> +namespace WebCore {
> +
> +struct LayerHitTestData* GraphicsLayer::sHitTestData = 0;

The = 0 is not necessary here.

> +void
> +GraphicsLayer::FloatValueList::insert(float key, float value, const TimingFunction* timingFunction)

The void should be on the same line as the rest of the declaration.

> +
> +GraphicsLayer::GraphicsLayer(GraphicsLayerClient* client)
> +: m_client(client)
> +, m_anchorPoint(0.5f, 0.5f)
> +, m_opacity(1.0f)
> +, m_anchorZ(0.0f)
> +#ifndef NDEBUG
> +, m_zPosition(0.0f)
> +#endif
> +, m_backgroundColorSet(false)
> +, m_contentsOpaque(false)
> +, m_preserves3D(false)
> +, m_backfaceVisibility(true)
> +, m_usingTiledLayer(false)
> +, m_masksToBounds(false)
> +, m_drawsContent(false)
> +, m_paintingPhase(GraphicsLayerPaintAllMask)
> +, m_parent(0)
> +#ifndef NDEBUG
> +, m_repaintCount(0)
> +#endif
> +{
> +}

The initialization list should be indented.


> +bool
> +GraphicsLayer::animateFloat(AnimatedPropertyID, const FloatValueList&, const Animation*, double /*beginTime*/)

bool should be on the same line as the declaration.


> +String GraphicsLayer::propertyIdToString(AnimatedPropertyID property)
> +{
> +    switch(property) {

Needs a space between the switch and the (.

> +        case AnimatedPropertyWebkitTransform: return "transform";
> +        case AnimatedPropertyBackgroundColor: return "backgroundColor";
> +        case AnimatedPropertyOpacity: return "opacity";
> +        default: return "";

The return bodies should be on their own lines.


> +    }
> +}
> +
> +int GraphicsLayer::findAnimationEntry(AnimatedPropertyID property, short index) const
> +{
> +    for (size_t i = 0; i < m_animations.size(); ++i)
> +        if (m_animations[i].property == property && m_animations[i].index == index)
> +            return (int) i;

Please use a c++ style cast.

> +    if (i >= 0) {
> +        m_animations[i].isCurrent = true;
> +        m_animations[i].beginTime = beginTime;
> +    }
> +    else {

else should be on the same line as the }

> +        // no animation yet, add it
> +        i = (int) m_animations.size();

C++ style cast.

> +            --size;
> +        }
> +        else

else should be on the same line.

> +            --size;
> +        }
> +        else

else should be on the same line.

> +
> +    int x = (int)roundf(contentPoint.x());
> +    int y = (int)roundf(contentPoint.y());

Please use c++ style casts.

> +
> +    ASSERT(sHitTestData);
> +    return m_client->contentsContainPoint(this, IntPoint(x, y), m_paintingPhase, sHitTestData);
> +}

> +void GraphicsLayer::dumpProperties(TextStream& ts, int indent) const
> +{
> +    writeIndent(ts, indent+1);
> +//    ts << "(CALayer " << (void*)m_layer.get() << ", transformlayer " << (void*)m_transformLayer.get() << ")\n";

Please don't check in commented out code.

The indent+1's should have spaces around + and the casts to void* should use c++ style casts.


> +++ b/WebCore/platform/graphics/GraphicsLayer.h
> @@ -0,0 +1,402 @@
> +/*
> + * Copyright (C) 2009 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer. 
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution. 
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission. 
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

2 clause please.

> +
> +    private:
> +        float mKey;
> +        float mVal;
> +        OwnPtr<TimingFunction> mTF;
> +    };

We use m_. You can also make these names longer, m_value and m_timingFunction. 

> +    class FloatValueList : public Vector<FloatValue> {
> +    public:
> +        void insert(float key, float val, const TimingFunction* tf);
> +    };

We tend to prefer composition over inheritance for basic data structures. 

> +    private:
> +        float mKey;
> +        OwnPtr<TransformOperations> mVal;
> +        OwnPtr<TimingFunction> mTF;

Same m_ issue.

> +    };
> +    
> +    // Used to store a series of transforms in a keyframe list.
> +    class TransformValueList : public Vector<TransformValue> {
> +    public:
> +        typedef Vector<TransformOperation::OperationType> FunctionList;
> +        
> +        void insert(float key, const TransformOperations* value, const TimingFunction* timingFunction);
> +        
> +        // return a list of the required functions. List is empty if keyframes are not valid
> +        // If return value is true, functions contain rotations of >= 180 degrees
> +        void makeFunctionList(FunctionList& list, bool& isValid, bool& hasBigRotation) const;
> +    };

Same composition over inheritance issue.

> +
> +    static GraphicsLayer* createGraphicsLayer(GraphicsLayerClient*);    // factory function

I don't think the comment is necessary.

> +protected:
> +    GraphicsLayer(GraphicsLayerClient*);
> +
> +    void dumpProperties(TextStream& ts, int indent) const;
> +    String dumpName() const { return String("GraphicsLayer"); }
> +
> +    // returns -1 if not found
> +    int findAnimationEntry(AnimatedPropertyID property, short index) const;
> +    bool addAnimationEntry(AnimatedPropertyID property, short index, double beginTime, 
> +                            bool isTransition, const Animation* transition);
> +
> +    virtual void removeAnimation(int /*inAnimIndex*/, bool /*reset*/) {}
> +    void removeAllAnimationsForProperty(AnimatedPropertyID property);
> +
> +protected:

No need to have a second protected: label here.

> +enum {
> +    GraphicsLayerPaintBackgroundMask     = (1 << 0),
> +    GraphicsLayerPaintForegroundMask     = (1 << 1),
> +    GraphicsLayerPaintAllMask            = (unsigned int)-1

Please use a c++ style cast.


> +namespace WebCore {
> +
> +class GraphicsLayerCA : public GraphicsLayer
> +{

{ should be on the line with the class declaration.

> +    GraphicsLayerCA(GraphicsLayerClient* client);

no need for the parameter name.

> +
> +    virtual void setTransform(const TransformationMatrix& t);
> +
> +    virtual void setChildrenTransform(const TransformationMatrix& t);

No need for parameter name.

> +static NSValue* createTransformFunctionValue(const GraphicsLayer::TransformValue& transformValue, size_t index,
> +                                                     const IntSize& size, TransformOperation::OperationType transformType)
> +{
> +    TransformOperation* op = (index >= transformValue.value()->operations().size()) ? 0 : transformValue.value()->operations()[index].get();
> +    
> +    switch (transformType) {
> +        case TransformOperation::ROTATE:
> +            return [NSNumber numberWithFloat:op ? (float) (static_cast<RotateTransformOperation*>(op)->angle() * M_PI / 180) : 0];
> +
> +        case TransformOperation::SCALE_X:
> +            return [NSNumber numberWithFloat:op ? (float) static_cast<ScaleTransformOperation*>(op)->x() : 0];
> +        case TransformOperation::SCALE_Y:
> +            return [NSNumber numberWithFloat:op ? (float) static_cast<ScaleTransformOperation*>(op)->y() : 0];
> +
> +        case TransformOperation::TRANSLATE_X:
> +            return [NSNumber numberWithFloat:op ? (float) static_cast<TranslateTransformOperation*>(op)->x(size) : 0];
> +        case TransformOperation::TRANSLATE_Y:
> +            return [NSNumber numberWithFloat:op ? (float) static_cast<TranslateTransformOperation*>(op)->x(size) : 0];
> +        
> +        case TransformOperation::SCALE:
> +        case TransformOperation::TRANSLATE:
> +        case TransformOperation::SKEW_X:
> +        case TransformOperation::SKEW_Y:
> +        case TransformOperation::SKEW:
> +        case TransformOperation::MATRIX:
> +        case TransformOperation::IDENTITY:
> +        case TransformOperation::NONE:
> +        {
> +            TransformationMatrix t;
> +            if (op)
> +                op->apply(t, size);
> +            CATransform3D cat;
> +            copyTransform(cat, t);
> +            return [NSValue valueWithCATransform3D:cat];
> +        }
> +    }
> +    
> +    return 0;
> +}
> +
> +static String getAnimationValueFunction(TransformOperation::OperationType transformType)
> +{
> +    switch (transformType) {
> +        case TransformOperation::ROTATE: return String("rotateZ");
> +        case TransformOperation::SCALE_X: return String("scaleX");
> +        case TransformOperation::SCALE_Y: return String("scaleY");
> +        case TransformOperation::TRANSLATE_X: return String("translateX");
> +        case TransformOperation::TRANSLATE_Y: return String("translateY");
> +        default: return String("");
> +    }
> +}
> +

> +    GraphicsLayerCA* childLayerCA = static_cast<GraphicsLayerCA*>(childLayer);
> +    GraphicsLayerCA* siblingLayerCA = static_cast<GraphicsLayerCA*>(sibling);
> +    if (found) {
> +        [hostLayerForSublayers() insertSublayer:childLayerCA->layerForSuperlayer() below:siblingLayerCA->layerForSuperlayer()];
> +    } else {

The braces around the if are not necessary.

> +    if (found) {
> +        [hostLayerForSublayers() insertSublayer:childLayerCA->layerForSuperlayer() above:siblingLayerCA->layerForSuperlayer()];
> +    } else {

Same here.

> +bool
> +GraphicsLayerCA::animateFloat(AnimatedPropertyID property, const FloatValueList& values, const Animation* animation, double beginTime)

bool should be on the same line.

> +void
> +GraphicsLayerCA::setBasicAnimation(AnimatedPropertyID property, const String& valueFunction, short index, void* fromVal, void* toVal, 
> +                            bool isTransition, const Animation* transition, double beginTime)

void on the same line.

> +void
> +GraphicsLayerCA::setKeyframeAnimation(AnimatedPropertyID property, const String& valueFunction, short index, void* keys, void* values, void* timingFunctions, 
> +                                    bool isTransition, const Animation* anim, double beginTime)

Same issue.

> +        id val = [layer valueForKeyPath:keyPath];
> +        switch(property) {

Space needed after switch. 

> +#import <QuartzCore/QuartzCore.h>
> +
> +namespace WebCore {
> +class GraphicsLayer;
> +}

class should be indented.

> +@interface WKLayer : CALayer 
> +{
> +    WebCore::GraphicsLayer* _layerOwner;
> +    float _contentsScale;
> +}
> +
> +// implements WKLayerAdditions
> +// implements ExtendedDescription

I don't think these comments are necessary.

> +        // smaller than the layer bounds (e.g. tiled layers)
> +        CGRect clipBounds = CGContextGetClipBoundingBox(ctx);
> +        IntRect clip(enclosingIntRect(clipBounds));
> +        layerContents->paintGraphicsLayerContents(context, clip);
> +
> +        context.setUseFontSmoothing(wasSmoothing);
> +
> +        [NSGraphicsContext restoreGraphicsState];
> +    }
> +    else {

Either the else should be on the same line as the }, the #ifndef should surround the entire else block.

> +#ifndef NDEBUG
> +        ASSERT_NOT_REACHED();
> +
> +        // FIXME: ideally we'd avoid calling -setNeedsDisplay on a layer that is a plain color,
> +        // so CA never makes backing store for it (which is what -setNeedsDisplay will do above).
> +        CGContextSetRGBFillColor(ctx, 0.0f, 1.0f, 0.0f, 1.0f);
> +        CGRect aBounds = [layer bounds];
> +        CGContextFillRect(ctx, aBounds);
> +#endif
> +    }
> +

> +@implementation CALayer(ExtendedDescription)
> +
> +- (NSString*)_descriptionWithPrefix:(NSString*)inPrefix
> +{
> +    CGRect aBounds  = [self bounds];
> +    CGPoint aPos    = [self position];
> +    CATransform3D t = [self transform];

We ususally don't line up the ='s.

> +    CALayer* curLayer;
> +    while ((curLayer = [sublayersEnum nextObject]))
> +    {
> +        [curDesc appendString:[curLayer _descriptionWithPrefix:sublayerPrefix]];
> +        //[curDesc appendString:@"\n"];

{ should be on the same line as the while, and the commented out code should be removed.

[Simon Fraser (smfr)]

I'll fix all these issues.

[Darin Adler]

Comment on attachment 26773 [details]
Patch, changelog

> +struct LayerHitTestData* GraphicsLayer::sHitTestData = 0;

The use of a global here seems like a really ugly thing. Is there any way around it?

> +void
> +GraphicsLayer::FloatValueList::insert(float key, float value, const TimingFunction* timingFunction)

Having the void on a separate line is not our usual formatting.

> +GraphicsLayer::GraphicsLayer(GraphicsLayerClient* client)
> +: m_client(client)

We normally indent one tab stop of these initialization lists.

> +, m_anchorPoint(0.5f, 0.5f)
> +, m_opacity(1.0f)
> +, m_anchorZ(0.0f)
> +#ifndef NDEBUG
> +, m_zPosition(0.0f)
> +#endif

Mitz and I and a few others talked about cases like this, and our consensus is that in cases like this where the "f" doesn't matter and a conversion would be done correctly and quietly, we don't use the "f". So we'd use "0.5", "1", and "0" rather than "0.5f", "1.0f", and "0.0f".

> +void GraphicsLayer::setPosition(const FloatPoint& point)
> +{
> +    m_position = point;
> +}
> +
> +void GraphicsLayer::setAnchorPoint(const FloatPoint& point, float anchorZ)
> +{
> +    m_anchorPoint = point;
> +    m_anchorZ = anchorZ;
> +}
> +
> +void GraphicsLayer::setSize(const FloatSize& size)
> +{
> +    m_size = size;
> +}
> +
> +void GraphicsLayer::setTransform(const TransformationMatrix& t)
> +{
> +    m_transform = t;
> +}
> +
> +void GraphicsLayer::setChildrenTransform(const TransformationMatrix& t)
> +{
> +    m_childrenTransform = t;
> +}
> +
> +void GraphicsLayer::setPreserves3D(bool preserves3D)
> +{
> +    m_preserves3D = preserves3D;
> +}
> +
> +void GraphicsLayer::setMasksToBounds(bool masksToBounds)
> +{
> +    m_masksToBounds = masksToBounds;
> +}
> +
> +void GraphicsLayer::setDrawsContent(bool drawsContent)
> +{
> +    m_drawsContent = drawsContent;
> +}
> +
> +void GraphicsLayer::setBackgroundColor(const Color& inColor, const Animation*, double /*beginTime*/)
> +{
> +    m_backgroundColor = inColor;
> +    m_backgroundColorSet = true;
> +}
> +
> +void GraphicsLayer::clearBackgroundColor()
> +{
> +    m_backgroundColor = Color();
> +    m_backgroundColorSet = false;
> +}
> +
> +void GraphicsLayer::setContentsOpaque(bool opaque)
> +{
> +    m_contentsOpaque = opaque;
> +}
> +
> +void GraphicsLayer::setBackfaceVisibility(bool visible)
> +{
> +    m_backfaceVisibility = visible;
> +}
> +
> +bool GraphicsLayer::setOpacity(float opacity, const Animation*, double)
> +{
> +    m_opacity = opacity;
> +    return false;       // not animating
> +}
> +
> +void GraphicsLayer::setDrawingPhase(GraphicsLayerPaintingPhase phase)
> +{
> +    m_paintingPhase = phase;
> +}
> +
> +void GraphicsLayer::setNeedsDisplay()
> +{
> +}
> +
> +void GraphicsLayer::setNeedsDisplayInRect(const FloatRect&)
> +{
> +}
> +
> +bool GraphicsLayer::animateTransform(const TransformValueList&, const IntSize&, const Animation*, double /*beginTime*/, bool /*isTransition*/)
> +{
> +    return false;
> +}
> +
> +bool
> +GraphicsLayer::animateFloat(AnimatedPropertyID, const FloatValueList&, const Animation*, double /*beginTime*/)
> +{
> +    return false;
> +}
> +
> +void GraphicsLayer::paintGraphicsLayerContents(GraphicsContext& context, const IntRect& clip)
> +{
> +    m_client->paintContents(this, context, m_paintingPhase, clip);
> +}
> +
> +GraphicsLayer* GraphicsLayer::hitTest(const FloatPoint&, struct LayerHitTestData*) const
> +{
> +    return 0;
> +}
> +
> +bool GraphicsLayer::layerContainsPoint(const FloatPoint&) const
> +{
> +    return false;
> +}

Unless these are all virtual, they seem like good candidates for inlines. If they are all virtual, are any of them good candidates for to be pure virtual?

> +String GraphicsLayer::propertyIdToString(AnimatedPropertyID property)
> +{
> +    switch(property) {
> +        case AnimatedPropertyWebkitTransform: return "transform";
> +        case AnimatedPropertyBackgroundColor: return "backgroundColor";
> +        case AnimatedPropertyOpacity: return "opacity";
> +        default: return "";
> +    }

The switch should have a space after it before the parenthesis.

Maybe this could use conventional formatting instead of the "every case on one line" formatting.

We normally omit "default" so we can take advantage of the gcc warning about missing enum values in switch statements, which is disabled for switch statements that have a default. For a case like this we'd put the return for the default case after the function. But also, shouldn't there be an ASSERT_NOT_REACHED, or is it legal to call this with a property ID that's not one of the known ones?

> +    for (size_t i = 0; i < m_animations.size(); ++i)
> +        if (m_animations[i].property == property && m_animations[i].index == index)
> +            return (int) i;

We use braces around the body of multi-line for statements. We use C++ style casts, not C-style.

> +    }
> +    else {

Closing brace goes on same line as else.

> +        i = (int) m_animations.size();

We use C++ style casts, not C-style.

> +        m_animations.append(AnimationEntry());
> +        m_animations[i].beginTime = beginTime;
> +        m_animations[i].property = property;
> +        m_animations[i].index = index;

This seems to cry out for a constructor that has an argument or two.

> +}
> +
> +
> +void GraphicsLayer::removeAllAnimationsForProperty(AnimatedPropertyID property)

Extra blank line here.

> +        }
> +        else
> +            ++i;

We put the brace on the same line as the else.

> +            --size;
> +        }
> +        else

Ditto.

> +    int x = (int)roundf(contentPoint.x());
> +    int y = (int)roundf(contentPoint.y());

Normally it's better to use lroundf in a case like this instead of calling roundf and casting. Also, C++ style casts are the norm.

> +    ASSERT(sHitTestData);
> +    return m_client->contentsContainPoint(this, IntPoint(x, y), m_paintingPhase, sHitTestData);

Is that assertion really helpful?

> +        if (drawsContent())
> +            if (m_usingTiledLayer)
> +                setDebugBorder(Color(0, 255, 0, 204), 2.0f);    // tiled layer: green
> +            else
> +                setDebugBorder(Color(255, 0, 0, 204), 2.0f);    // normal layer: red
> +        else if (masksToBounds()) {
> +            setDebugBorder(Color(128, 255, 255, 178), 2.0f);    // masking layer: pale blue
> +            if (GraphicsLayer::showDebugBorders())
> +                setDebugBackgroundColor(Color(128, 255, 255, 52));
> +        } else
> +            setDebugBorder(Color(255, 255, 0, 204), 2.0f);      // container: yellow

The if should have braces around its multiple-line body.

> +#include "Animation.h"
> +#include "Color.h"
> +#include "FloatPoint.h"
> +#include "FloatRect.h"
> +#include "FloatQuad.h"
> +#include "GraphicsLayerClient.h"
> +#include "IntRect.h"
> +#include "PlatformString.h"
> +#include "TextStream.h"
> +#include "TimingFunction.h"
> +#include "TransformationMatrix.h"
> +#include "TransformOperations.h"
> +
> +#include <wtf/OwnPtr.h>

Is there anything we can do to reduce the number of includes here? Is every one of these needed?

> +
> +#if PLATFORM(MAC)
> +#ifdef __OBJC__
> +@class WKLayer;
> +@class CALayer;
> +typedef WKLayer PlatformLayer;
> +typedef CALayer* NativeLayer;
> +#else
> +typedef void* PlatformLayer;
> +typedef void* NativeLayer;
> +#endif
> +#else
> +typedef void* PlatformLayer;
> +typedef void* NativeLayer;
> +#endif
> +
> +namespace WebCore {
> +
> +class GraphicsContext;
> +class Image;
> +class TimingFunction;
> +
> +// GraphicsLayer is an abstraction for a rendering surface with backing store,
> +// which may have associated transformation and animations.
> +
> +// FIXME: inherit from TreeShared?

I don't think that FIXME is clear enough. Why would we want to inherit from TreeShared?

> +        FloatValue(float key = nanf(0), float value = 0, const TimingFunction* timingFunction = 0)

I've never seen that nanf syntax before. Is that portable?

> +    class FloatValueList : public Vector<FloatValue> {
> +    public:
> +        void insert(float key, float val, const TimingFunction* tf);
> +    };

We usually consider deriving from a Vector to be an anti-pattern. Could this just be a helper function rather than a class? Also, we'd omit that "tf" identifier.

> +        float mKey;

We use m_key.

> +        OwnPtr<TransformOperations> mVal;

And m_value;

> +        OwnPtr<TimingFunction> mTF;

And m_timingFunction.

> +    String name() const { return m_name; }

This should return a const String& for better efficiency.

> +    FloatPoint position() const { return m_position; }

This could return a const FloatPoint& for better efficiency.

> +    FloatPoint anchorPoint() const { return m_anchorPoint; }

Ditto.

> +    FloatSize size() const { return m_size; }

Ditto.

> +    TransformationMatrix transform() const { return m_transform; }

Ditto.

> +    TransformationMatrix childrenTransform() const { return m_childrenTransform; }

Ditto.

> +    Color backgroundColor() const { return m_backgroundColor; }

Ditto.

> +    virtual void setBackgroundColor(const Color&, const Animation* anim = 0, double beginTime = 0);

We'd normally omit the name "anim" here.

> +    virtual bool setOpacity(float o, const Animation* anim = 0, double beginTime = 0);

We'd normally use the word "opacity" not "o" and omit the name anim here.

> +    void setDrawingPhase(GraphicsLayerPaintingPhase phase);

We'd normally omit the name "phase" here. I may have missed other cases like this in setters above.

> +    virtual void setNeedsDisplayInRect(const FloatRect& inRect);

We'd normally omit the name "inRect" here. I may have missed other cases like this in setters above.

> +    virtual bool animateTransform(const TransformValueList& values, const IntSize& size,
> +                                  const Animation* anim, double beginTime, bool isTransition);

We'd omit "values", "size", and "anim" here.

> +    virtual bool animateFloat(AnimatedPropertyID property, const FloatValueList& values,
> +                                  const Animation* transition, double beginTime);

We'd omit "property", "values", and "transition" here.

> +    virtual void setContentsToImage(Image*) {}
> +    virtual void setContentsToVideo(PlatformLayer*) {}
> +    virtual void setContentsBackgroundColor(const Color&) {}
> +    virtual void clearContents() {}
> +    
> +    virtual void updateContentsRect() {}

Should any of these be "= 0" instead of "{}"? Also, we normally use "{ }".

> +    void paintGraphicsLayerContents(GraphicsContext& context, const IntRect& clip);

We'd normally omit "context" here.

> +    virtual GraphicsLayer* hitTest(const FloatPoint& point, struct LayerHitTestData* inData) const;

We'd omit "point", "struct", and "inData" here.

> +    virtual bool layerContainsPoint(const FloatPoint& point) const;

We'd normally omit "point" here.

> +    virtual FloatPoint convertPointToLayer(const FloatPoint& point, GraphicsLayer* targetLayer) const;
> +    virtual FloatPoint convertPointFromLayer(const FloatPoint& point, GraphicsLayer* sourceLayer) const;

And here.

> +    virtual void convertQuadToLayer(FloatQuad& quad, const GraphicsLayer* targetLayer) const;

And omit "quad" here.

> +    void dumpLayer(TextStream& ts, int indent = 0) const;

And omit "ts" here.

> +    static bool graphicsContextsFlipped();

We worked hard to keep the "flipped" concept entirely out of WebCore until now. Does this really need to be here? And if it does, are we using the Cocoa sense, where "flipped" means what everyone else seems to call "normal"? Can we use our own term for this, perhaps, that's less ambiguous?

> +    static String propertyIdToString(AnimatedPropertyID property);

We'd omit "property" here.

> +    void dumpProperties(TextStream& ts, int indent) const;

And "ts" here.

> +    String dumpName() const { return String("GraphicsLayer"); }

If this is a non-virtual function, that's quite strange. If it's virtual, we normally repeat the word virtual. You should not need the explicit String() here.

> +    int findAnimationEntry(AnimatedPropertyID property, short index) const;

We'd omit the word "property" here.

> +    bool addAnimationEntry(AnimatedPropertyID property, short index, double beginTime, 
> +                            bool isTransition, const Animation* transition);

We'd omit "property" and "transition" here.

> +    void removeAllAnimationsForProperty(AnimatedPropertyID property);

We'd omit the word "property" here.

> +protected:
> +    GraphicsLayerClient* m_client;

Can any of the stuff below be private instead of protected?

> +    FloatPoint m_position;
> +    FloatPoint m_anchorPoint;
> +    FloatSize  m_size;

We normally wouldn't try to line things up here. So please leave out that extra space.

> +    class AnimationEntry {
> +        public:
> +        AnimationEntry()
> +        : beginTime(-1)
> +        , pauseTime(-2)
> +        , endTime(-1)
> +        , property(AnimatedPropertyInvalid)
> +        , isCurrent(true)
> +        , isTransition(true) { }

Indentation and formatting here is not the normal we use in WebKit. The public should not be indented. The constructor should be indented from where the public is. The initialization list should be indented from the constructor. The braces should be on separate lines and below since this is not the special case of an entire definition one one line.

> +        RefPtr<Animation> animLayer;

The abbreviation here seems unneeded. It should either be named layer, animation, or animationLayer, not animLayer.

> +    static struct LayerHitTestData* sHitTestData;      // temporarily assigned while hit testing

There's no need for the "struct". That's never needed in C++; it's a C-ism. In the rare cases we are forced to use globals we try to use internal linkage rather than static data members if possible.

> +enum {
> +    GraphicsLayerPaintBackgroundMask     = (1 << 0),
> +    GraphicsLayerPaintForegroundMask     = (1 << 1),
> +    GraphicsLayerPaintAllMask            = (unsigned int)-1

Seems unnecessary to use the negative number here; I think that UINT_MAX or 0xFFFFFFFFU would both be better.

> +typedef unsigned int GraphicsLayerPaintingPhase;

We don't use enum/typedef pairs in WebKit. This should just be the name of the enum. Or the constants could just be static constants and not an enum. In any case, we use "unsigned", not "unsigned int".

> +class GraphicsContext;
> +class GraphicsLayer;
> +class IntPoint;
> +class IntRect;

Forward declarations of classes from outside the file should come first, before any definitions in the file.

> +    virtual void notifyTransitionStarted(const GraphicsLayer* layer, AnimatedPropertyID property, double time) = 0;

We'd normally omit the names "layer" and "property" here.

> +    virtual void notifyAnimationStarted(const GraphicsLayer* layer, double time) = 0;

And "layer" here.

> +    virtual void paintContents(const GraphicsLayer* layer, GraphicsContext& context, GraphicsLayerPaintingPhase compositePhase, const IntRect& inClip) = 0;

And "layer", "context", and "compositePhase" here. And call it "clipRect" or "clip" instead of "inClip".

> +    virtual bool contentsContainPoint(const GraphicsLayer* layer, const IntPoint& point, GraphicsLayerPaintingPhase compositePhase, struct LayerHitTestData* hitTestData) = 0;

We'd normally omit the names "layer", "point", "compositePhase" and "hitTestData" here. Also "struct".

> +    virtual FloatPoint convertToContentsCoordinates(const GraphicsLayer* layer, const FloatPoint& point) = 0;

We'd normally omit the names "layer" and "point" here.

> +    virtual IntRect contentsBox(const GraphicsLayer* layer) = 0;

And "layer" here.

> +#ifndef GraphicsLayerCA_h_
> +#define GraphicsLayerCA_h_

No trailing underscore.

> +#import "config.h"

Header files must not include "config.h".

> +#import "GraphicsLayer.h"
> +
> +#include <wtf/RetainPtr.h>

No spaces between the "" and the <> includes; just a sorted list.

> +@class WKLayer;
> +@class AnimationDidStartCB;

Normally we sort alphabetically. AnimationDidStartCB is an Objective-C class name, and they are in a global namespace, so it needs a prefix, typically "Web". Our prefix is not "WK", it's "Web". So it should be "WebLayer", not "WKLayer".

> +class GraphicsLayerCA : public GraphicsLayer
> +{

Brace goes on the same line as class name.

> +public:
> +
> +    GraphicsLayerCA(GraphicsLayerClient* client);

Extra blank line here. We'd omit the word "client" here.

> +    virtual void setName(const String& name);

We'd omit the name "name" here.

> +    virtual void addChildAbove(GraphicsLayer* layer, GraphicsLayer* sibling);
> +    virtual void addChildBelow(GraphicsLayer* layer, GraphicsLayer* sibling);

We'd normally omit the name "layer". I suggest using the name "parent" instead of using no name at all.

> +    virtual void setAnchorPoint(const FloatPoint&, float anchorZ);

No 3D point class?

> +    virtual void setTransform(const TransformationMatrix& t);
> +
> +    virtual void setChildrenTransform(const TransformationMatrix& t);

We'd omit the name "t" here.

> +    virtual void setBackgroundColor(const Color&, const Animation* anim = 0, double beginTime = 0);

We'd omit the name "anim" here.

> +    virtual bool setOpacity(float, const Animation* anim = 0, double beginTime = 0);

We'd omit the name "anim" here.

> +    // mark the given rect (in layer coords) as needing dispay
> +    virtual void setNeedsDisplayInRect(const FloatRect&);

Comment doesn't seem helpful here. We're just overriding it. We shouldn't cut and paste all the comments.

> +    virtual bool animateTransform(const TransformValueList& values, const IntSize& size,
> +                                  const Animation* anim, double beginTime, bool isTransition);

We'd omit "values", "size", and "anim" here.

> +    virtual bool animateFloat(AnimatedPropertyID property, const FloatValueList& values,
> +                                  const Animation* transition, double beginTime);

We'd omit "property", "values", and "transition" here. This also shows how it's not good to try to line up parentheses, because they get unaligned when you rename things. That's one reason I prefer indenting one tab stop, but all the text editors seems to be trying to line things up (yuck).

> +    virtual GraphicsLayer* hitTest(const FloatPoint& point, struct LayerHitTestData* data) const;
> +    virtual bool layerContainsPoint(const FloatPoint& point) const;

We'd omit "point", and "data" here.

> +    virtual FloatPoint convertPointToLayer(const FloatPoint& point, GraphicsLayer* targetLayer) const;
> +    virtual FloatPoint convertPointFromLayer(const FloatPoint& point, GraphicsLayer* sourceLayer) const;

We'd omit "point" here.

> +    virtual void convertQuadToLayer(FloatQuad& ioQuad, const GraphicsLayer* targetLayer) const;

I think it's confusing that the quad one modifies the quad in place, and the point ones don't. Can't we pick one style or the other?

We'd omit "ioQuad" here, and we don't use "in" "out" and "io" prefixes, although here I see it could be helpful.

> +    void setBasicAnimation(AnimatedPropertyID property, const String& component, short index, void* fromVal, void* toVal, 
> +                                bool isTransition, const Animation* transition, double time);

The use of void* here seems like a poor way to do the polymorphism, but maybe that's already baked in to how we do these things elsewhere in the animation machinery.

We'd normally omit the words "property" and "transition" here.

> +    void setKeyframeAnimation(AnimatedPropertyID property, const String& component, short index, void* keys, void* values, void* timingFunctions, 
> +                                bool isTransition, const Animation* transition, double time);

We'd normally omit the words "property" and "transition" here.

The use of void* here seems poor; we should find a better way to do this.

> +    void setAnimation(AnimatedPropertyID property);

We'd normally omit the word "property" here.

> +    void animDone(AnimatedPropertyID property, short index);

We'd normally omit the words "property" here. Why the abbreviation anim here, when all the other functions call it animation.

> +    virtual void removeAnimation(int inAnimIndex, bool reset);

We'd use "index" or "animationIndex", rather than "inAnimIndex". The use of bool for arguments like this is normally discouraged. One alternative is an enum.

> +protected:

Should be private, not protected. Also, should only say it once, and should have no blank line.

> +    RetainPtr<WKLayer> m_layer;
> +    RetainPtr<WKLayer> m_transformLayer;
> +    RetainPtr<WKLayer> m_contentsLayer;
> +
> +    RetainPtr<AnimationDidStartCB> m_animationDidStartCB;

I'm not fond of the use of "CB" here to mean "callback". Is that our terminology or something from CoreAnimation? We have a lot of other objects like this in our Objective-C++ code and we never use the CB prefix for any of them.

> +#import <QuartzCore/QuartzCore.h>
> +
> +#import <QuartzCore/CAAnimationPrivate.h>
> +#import <QuartzCore/CAValueFunction.h>
> +
> +#import "Animation.h"
> +#import "BlockExceptions.h"
> +#import "CString.h"
> +#import "Image.h"
> +#import "PlatformString.h"
> +#import "RotateTransformOperation.h"
> +#import "ScaleTransformOperation.h"
> +#import "SystemTime.h"
> +#import "TranslateTransformOperation.h"
> +#import "WKLayer.h"
> +#import "WKTiledLayer.h"
> +#import <wtf/CurrentTime.h>

Includes should be in a single paragraph, and sorted as with the sort tool.

We can't include a header like <QuartzCore/CAAnimationPrivate.h>; people outside Apple won't have that on their computers. So we need an alternate strategy for a file like that.

> +static NSString* const kAnimationCSSPropertyKey = @"GraphicsLayerCA_property";

We don't use "k" for this sort of thing, normally. Why do we need this? It has no WebKit prefix, so I assume it's some part of CA's interface?

> +static const int kMaxPixelDimension = 2000;
> +static const int kTiledLayerTileSize = 512; // This is arbitrary and not empirical

I'd like to see some more extensive comments explaining these.

> +static double unixTimeToCATime(double t)
> +{
> +    return CACurrentMediaTime() - WTF::currentTime() + t;
> +}

Can't we determine the conversion factor at compile time instead? It seems bad to calculate the current time twice every single time we convert a time from one to the other! Also, the WTF time isn't necessarily "Unix" time. Maybe CA time is just the same as CF time?

> +- (void)animationDidStart:(CAAnimation *)anim

Not a fan of "anim".

> +static NSValue* createTransformFunctionValue(const GraphicsLayer::TransformValue& transformValue, size_t index,
> +                                                     const IntSize& size, TransformOperation::OperationType transformType)

Another case of the "pointless to try to line things up", in my opinion.

> +            return [NSNumber numberWithFloat:op ? (float) (static_cast<RotateTransformOperation*>(op)->angle() * M_PI / 180) : 0];

We use C++ casts, not C casts. But is the cast to float actually helpful?

> +        case TransformOperation::ROTATE: return String("rotateZ");
> +        case TransformOperation::SCALE_X: return String("scaleX");
> +        case TransformOperation::SCALE_Y: return String("scaleY");
> +        case TransformOperation::TRANSLATE_X: return String("translateX");
> +        case TransformOperation::TRANSLATE_Y: return String("translateY");
> +        default: return String("");

Should not need the explicit String() on these. Should not have a default (see my comment above).

> +    CGColorRef borderColor = cgColor(color);
> +    [layer setBorderColor:borderColor];
> +    CGColorRelease(borderColor);

You didn't do this, but it's *terrible* that cgColor returns a newly-created CGColor! It doesn't have the word "Create" or "Copy" in its name, nor does it return a RetainPtr. We need to fix that!!

> +    static int showDebugBorders = -1;
> +
> +    if (showDebugBorders < 0) {
> +        BEGIN_BLOCK_OBJC_EXCEPTIONS
> +        showDebugBorders = [[NSUserDefaults standardUserDefaults] boolForKey:@"WebCoreLayerBorders"];
> +        END_BLOCK_OBJC_EXCEPTIONS
> +    }

There is a more-elegant way to do this in C++ without the special magic value. Also, I don't think we need to block exceptions. There's no reason to expect an exception here and it's debug-only code. We can write it like this:

    static bool showDebugBorders = [[NSUserDefaults standardUserDefaults] boolForKey:@"WebCoreLayerBorders"];
    return showDebugBorders;

> +    if (showRepaintCounter < 0) {
> +        BEGIN_BLOCK_OBJC_EXCEPTIONS
> +        showRepaintCounter = [[NSUserDefaults standardUserDefaults] boolForKey:@"WebCoreLayerRepaintCounter"];
> +        END_BLOCK_OBJC_EXCEPTIONS
> +    }

Ditto.

> +void GraphicsLayerCA::setAnchorPoint(const FloatPoint& point, float inZ)

"inZ" is really awkward. How about just "z" or "anchorZ"?

> +        setBasicAnimation(AnimatedPropertyBackgroundColor, "", 0, (id)fromBackgroundColor, (id)bgColor, true, transition, beginTime);

We use C++ casts, not C casts.

Notice how mysterious that "true" is? I hate boolean arguments!

> +    float clampedOpacity = std::max(std::min(opacity, 1.0f), 0.0f);

I like to write the clamping like this:

    float clampedOpacity = max(0.0f, min(opacity, 1.0f));

So you can see that it's going to be "between" 0 and 1. Also, we normally put using namespace std at the top of .cpp files and leave out the "std::" within the file, unless there's a problem with ambiguity.
    
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS
> +    TransformationMatrix t;
> +    CATransform3D toT3D;
> +    copyTransform(toT3D, t);
> +    [primaryLayer() setTransform:toT3D];
> +    END_BLOCK_OBJC_EXCEPTIONS

We really don't want multiple sections of BEGIN_BLOCK_OBJC_EXCEPTIONS in the same function. Just one wrapping more of the function would be better.

> +bool
> +GraphicsLayerCA::animateFloat(AnimatedPropertyID property, const FloatValueList& values, const Animation* animation, double beginTime)

Return type on same line.

> +        [animatedLayer(property) setValue: 0 forKeyPath:propertyIdToString(property)];

No space after colon.

> +        setBasicAnimation(property, "", 0, 
> +                          isnan(fromVal) ? nil : [NSNumber numberWithFloat:fromVal], 
> +                          isnan(toVal) ?   nil : [NSNumber numberWithFloat:toVal],
> +                          false, animation, beginTime);

Don't believe in lining things up.

> +    NSMutableArray* timesArray  = [[NSMutableArray alloc] init];
> +    NSMutableArray* valArray    = [[NSMutableArray alloc] init];
> +    NSMutableArray* tfArray     = [[NSMutableArray alloc] init];

Ditto.

> +    for (FloatValueList::const_iterator it = values.begin(); it != values.end(); ++it) {

We normally suggest iterating vectors with indices. Iterators are provided to help use them with generic algorithms, but are a bit more awkward, and also more fragile since they fail if vector is enlarged while iterating.

> +        const FloatValue& curValue = (*it);

No parentheses needed.

> +        [timesArray addObject: [NSNumber numberWithFloat:curValue.key()]];
> +        [valArray addObject: [NSNumber numberWithFloat:curValue.value()]];

No spaces after colons.

> +            [tfArray addObject: [CAMediaTimingFunction functionWithControlPoints:0.25f:0.1f:0.25f:1.0f]];

Add spaces before those colons?

> +             [tfArray addObject:[CAMediaTimingFunction functionWithControlPoints:
> +                                            (float)tf->x1():(float)tf->y1():(float)tf->x2():(float)tf->y2()]];

C++ style is preferred for casts. No casts at all is even better. Very strange to have the first colon on a different line. Spaces before colons?

> +        // FIXME: is image flipping really a property of the graphics context?
> +        bool needToFlip = GraphicsLayer::graphicsContextsFlipped();
> +        CGPoint anchorPoint = needToFlip ? CGPointMake(0.0f, 1.0f) : CGPointMake(0.0f, 0.0f);

This seems mixed up. Also, CGPointZero is available.

> +        IntRect  contentRect = m_client->contentsBox(this);

Extra space here, not needed.

> +void
> +GraphicsLayerCA::setBasicAnimation(AnimatedPropertyID property, const String& valueFunction, short index, void* fromVal, void* toVal, 
> +                            bool isTransition, const Animation* transition, double beginTime)

The void should be on the same line as the function name. Again, the vain effort to line things up.

> +    if (!fromVal && !toVal)
> +        return;

Are 0 values legal and to be silently ignored? Or should there be two assertions in addition to or instead of that check?

> +    String animName = keyPath + "_" + String::number(index);

Strings do this very inefficiently. This will cause multiple memory allocations every time. More efficient would be to call a helper function that uses Vector<UChar>. Probably OK for now.

> +        // If we send a duration of 0 to CA, then it will use the default duration
> +        // of 250ms. So send a very small value instead.
> +        double duration = transition->duration();
> +        if (duration <= 0)
> +            duration = 1e-3;

The 1e-3 should be a named constant. The constant could have this comment instead.

> +        float repeatCount = transition->iterationCount();
> +        if (repeatCount < 0)
> +            repeatCount = FLT_MAX;
> +        else if (transition->direction())
> +            repeatCount /= 2;

Need a comment explaining why direction() being true makes the repeat count divide by two. I think I know why, but the person reading the code won't necessarily see it. Might be clearer if it was a local variable named autoreverses that was being looked at.

> +        // TODO: currently we don't have any component animations for more than one
> +        // function. So for now we will always use additive NO.

We use FIXME for this, never TODO.

> +        [anim setAdditive:(property == AnimatedPropertyWebkitTransform) ? YES : NO];

A bool expression can be used directly. No need for "? YES : NO".

> +        [anim setValue:[NSNumber numberWithInt: prop] forKey:kAnimationCSSPropertyKey];

Extra space after colon here.

> +        if (fromVal != nil)
> +            [anim setFromValue:(id)fromVal];
> +        if (toVal != nil)
> +            [anim setToValue:(id)toVal];

Why are those casts to (id) needed? We normally use C++ casts.

> +        CALayer* presLayer = [layer presentationLayer];
> +        if (presLayer)
> +            [layer setValue: [presLayer valueForKeyPath:keyPath] forKeyPath:keyPath];

Space after colon.

> +        // If we send a duration of 0 to CA, then it will use the default duration
> +        // of 250ms. So send a very small value instead.
> +        double duration = anim->duration();
> +        if (duration <= 0)
> +            duration = 1e-3;

Lots of repeated code. Can we refactor to reduce the cut and paste-y ness of this?

> +    String animName = keyPath + "_" + String::number(m_animations[animIndex].index);

I see this over and over again. Should call a helper function for it.

> +        switch(property) {

Missing space here.

> +            case AnimatedPropertyWebkitTransform:
> +                // We don't save the current transform, so we will have to rely on it being passed in.
> +                break;

I don't understand this comment. What does "will have to rely" mean. Why is the transform different from other properties in this respect? Does this comment reflect a problem, or is it just a half-hearted explanation of a good design?

> +            case AnimatedPropertyBackgroundColor:
> +                m_backgroundColor = Color(val);
> +                break;
> +            case AnimatedPropertyOpacity:
> +                m_opacity = [val floatValue];
> +                break;
> +            default:
> +                break;

See comments above about "default" in switch statements.

> +static inline bool handlesHitTest(CALayer* layer)
> +{
> +    return layer && ([layer isKindOfClass:[WKLayer self]] || [layer isKindOfClass:[WKTiledLayer self]]) &&
> +                    [layer layerOwner] &&
> +                    [layer layerOwner]->client();
> +}

Looks like this is lined up off by one character. I think earlier returns would make this easier to read and then you wouldn't have the lining up problem.

> +    if (color.isValid())
> +        setLayerBackgroundColor(m_layer.get(), color);
> +    else
> +        clearLayerBackgroundColor(m_layer.get());

I don't think an invalid color should be a special case. In my opinion any background color with alpha of 0 should have the same behavior. It's almost never helpful to handle an invalid color as a special case unless you have some need for a lack of color separate from transparent color.

> +    return (size.width() > kMaxPixelDimension || size.height() > kMaxPixelDimension);

Extra unneeded parentheses here. Do we really want a layer for a 1-pixel-tall thing that's really wide? Or a 0-pixel-tall thing?

> +    CALayer* oldLayer = m_layer.get();
> +    [oldLayer retain];      // keep it around after the RetainPtr is assigned the new layer. balanced by -release below.

Instead make oldLayer be a RetainPtr and then you don't need these comments to explain the retain and release.

> +    CGPoint anchorPoint = CGPointMake(0.0f, 0.0f);

CGZeroPoint

> +namespace WebCore {
> +class GraphicsLayer;
> +}

We indent these.

> +@interface CALayer(WKLayerAdditions)
> +
> +- (void)setLayerOwner:(WebCore::GraphicsLayer*)aLayer;
> +- (WebCore::GraphicsLayer*)layerOwner;
> +
> +@end

Does this need to be in the header?

> +#ifndef NDEBUG
> +// for debug logging
> +@interface CALayer(ExtendedDescription)
> +
> +- (NSString*)extendedDescription;
> +
> +@end
> +#endif

Does this need to be in the header?

> +#import <QuartzCore/QuartzCore.h>
> +
> +#import "WKLayer.h"
> +
> +#import "GraphicsContext.h"
> +#import "GraphicsLayer.h"
> +#import "WebCoreTextRenderer.h"
> +#import <wtf/UnusedParam.h>

WKLayer.h needs to be included first, before QuartzCore.h. Other includes should be sorted.

> ++ (void)drawContents:(WebCore::GraphicsLayer*)layerContents ofLayer:(CALayer*)layer intoContext:(CGContextRef)ctx

Should be "context", not "ctx".

> +    if (layerContents && layerContents->client()) {
> +
> +        [NSGraphicsContext saveGraphicsState];

Extra blank line here. Should leave it out.

> +        PlatformGraphicsContext* platformContext = static_cast<PlatformGraphicsContext*>(ctx);

There should be no need for a type cast here. Please don't cast it. In fact, you should be able to just pass the context to the GraphicsContext constructor.

> +        // Turn off text smoothing
> +        bool wasSmoothing = context.usingFontSmoothing();
> +        context.setUseFontSmoothing(false);

The comment says the same thing as the code. This is a perfect opportunity to explain *why* we're doing this instead of restating what we're doing. Also, capitalized comments should also have periods.

> +    }
> +    else {

Brace goes on same line.

> +- (void)drawInContext:(CGContextRef)ctx

"context", not "ctx"

> +- (BOOL)containsPoint:(CGPoint)inPoint

"point", not "inPoint"

> +        IntPoint    thePoint(inPoint.x, inPoint.y);
> +        BOOL layerOwnerContainsPoint =  _layerOwner->contentsContainPoint(thePoint);
> +        return layerOwnerContainsPoint;

Should just say _layerOwner->contentsContainPoint(IntPoint(point)) here -- no point in having a two named local variables.

> +        IntPoint thePoint(inPoint.x, inPoint.y);
> +        return _layerOwner->contentsContainPoint(thePoint);

Same here. More copy and paste code.

Enough comments that I'll say review- for now. Seems in good shape, though.

[Simon Fraser (smfr)]

Created attachment 27000 [details]
Revised patch

[Darin Adler]

Comment on attachment 27000 [details]
Revised patch


> +String GraphicsLayer::propertyIdToString(AnimatedPropertyID property)
> +{
> +    switch (property) {
> +        case AnimatedPropertyWebkitTransform:
> +            return "transform";
> +        case AnimatedPropertyOpacity:
> +            return "opacity";
> +        case AnimatedPropertyBackgroundColor:
> +            return "backgroundColor";
> +        case AnimatedPropertyInvalid:
> +            ASSERT_NOT_REACHED();
> +    }
> +    return "";
> +}

You need an ASSERT_NOT_REACHED() outside the switch statement too, in case the property is a value not in the enum at all.

> +        FloatValue(float key = NAN, float value = 0, const TimingFunction* timingFunction = 0)

I believe that elsewhere we fund that the portable value was:

    std::numeric_limits<float>::quiet_nan()

And that NAN was not consistently available on platforms other than Mac OS X. Sorry I wasn't more specific in my last round of review.

> +enum GraphicsLayerPaintingPhase {
> +    GraphicsLayerPaintBackgroundMask = (1 << 0),
> +    GraphicsLayerPaintForegroundMask = (1 << 1),
> +    GraphicsLayerPaintAllMask = UINT_MAX
> +};

To use UINT_MAX you need to include the appropriate header. I believe it's <limits.h>.

This generally looks good but I don't have time to read it all right now. It's particularly challenging to collate the new patch with my old questions. I'll get to it as soon as I can but it's challenging!

[Simon Fraser (smfr)]

(In reply to comment #6)
> (From update of attachment 27000 [details] [review])

> > +        FloatValue(float key = NAN, float value = 0, const TimingFunction* timingFunction = 0)
> 
> I believe that elsewhere we fund that the portable value was:
> 
>     std::numeric_limits<float>::quiet_nan()

JSCore uses NaN:
http://trac.webkit.org/browser/trunk/JavaScriptCore/runtime/JSCell.cpp#L35

[Simon Fraser (smfr)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 27000 [details] [review] [review])
> 
> > > +        FloatValue(float key = NAN, float value = 0, const TimingFunction* timingFunction = 0)
> > 
> > I believe that elsewhere we fund that the portable value was:
> > 
> >     std::numeric_limits<float>::quiet_nan()
> 
> JSCore uses NaN:
> http://trac.webkit.org/browser/trunk/JavaScriptCore/runtime/JSCell.cpp#L35

Oh, never mind, I see that's inside an #if defined(NaN)

[Simon Fraser (smfr)]

Filed bug 23624 to expose a NaN we can use.

[Simon Fraser (smfr)]

Created attachment 27159 [details]
Final patch.

There was no need for the NaN, so no problem there. Other minor cleanup in response to comments.

[Simon Fraser (smfr)]

Created attachment 27161 [details]
Fixed, final

[Dave Hyatt]

Comment on attachment 27161 [details]
Fixed, final

r=me

[Simon Fraser (smfr)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	A	WebCore/platform/graphics/GraphicsLayer.cpp
	A	WebCore/platform/graphics/GraphicsLayer.h
	A	WebCore/platform/graphics/GraphicsLayerClient.h
	A	WebCore/platform/graphics/mac/GraphicsLayerCA.h
	A	WebCore/platform/graphics/mac/GraphicsLayerCA.mm
	A	WebCore/platform/graphics/mac/WebLayer.h
	A	WebCore/platform/graphics/mac/WebLayer.mm
	A	WebCore/platform/graphics/mac/WebTiledLayer.h
	A	WebCore/platform/graphics/mac/WebTiledLayer.mm
Committed r40433