[webkit-reviews] review denied: [Bug 23356] Land new files in WebCore/platform related to accelerated compositing : [Attachment 26773] Patch, changelog
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 23 15:55:48 PST 2009
Darin Adler <darin at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 23356: Land new files in WebCore/platform related to accelerated
compositing
https://bugs.webkit.org/show_bug.cgi?id=23356
Attachment 26773: Patch, changelog
https://bugs.webkit.org/attachment.cgi?id=26773&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> +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__
> + at class WKLayer;
> + at 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.
> + at class WKLayer;
> + at 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.
> + at interface CALayer(WKLayerAdditions)
> +
> +- (void)setLayerOwner:(WebCore::GraphicsLayer*)aLayer;
> +- (WebCore::GraphicsLayer*)layerOwner;
> +
> + at end
Does this need to be in the header?
> +#ifndef NDEBUG
> +// for debug logging
> + at interface CALayer(ExtendedDescription)
> +
> +- (NSString*)extendedDescription;
> +
> + at 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.
More information about the webkit-reviews
mailing list