[webkit-reviews] review denied: [Bug 79867] [BlackBerry] Upstream GraphicsLayerBlackBerry.{h, cpp} : [Attachment 129411] patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Robin Cao
<robin.cao at torchmobile.com.cn>'s request for review:
Bug 79867: [BlackBerry] Upstream GraphicsLayerBlackBerry.{h, cpp}
https://bugs.webkit.org/show_bug.cgi?id=79867

Attachment 129411: patch
https://bugs.webkit.org/attachment.cgi?id=129411&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129411&action=review


Looks good but can be cleaned up a bit more.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:53
> +#include "NotImplemented.h"

Not used?

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:58
> +#include <wtf/text/CString.h>

Check for all includes if they are needed please.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:71
> +    layer.setBorderColor(static_cast<RGBA32>(0));

Better use transparant from Color.h

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:81
> +    layer.setBackgroundColor(static_cast<RGBA32>(0));

Ditto.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:278
> +void GraphicsLayerBlackBerry::setHasFixedContainer(bool b)

b is not a good param name here.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:287
> +void GraphicsLayerBlackBerry::setHasFixedAncestorInDOMTree(bool b)

Ditto.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:400
> +    // This is what GraphicsLayerCA checks for

Lacks period.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:404
> +    // We only support these two kinds of properties ATM

Ditto.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:406
> +	&& values.property() != AnimatedPropertyOpacity)

Can be put on one line.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:433
> +    while (animation = removeAnimationByName(animationName,
m_runningAnimations)) {

Can be combined.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:37
> +#include "GraphicsContext.h"

Needed?

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:39
> +#include "LayerAnimation.h"

Needed?

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:98
> +    virtual void removeAnimation(const String& /*animationName*/);

Why comment the param names?

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:172
> +    ContentsLayerPurpose m_contentsLayerPurpose;

You should be able to compress this a bit by giving it 2 bits :
ContentsLayerPurpose m_contentsLayerPurpose : 2;
But make sure our compiler really decreases the object size, just try with
sizeof before and after.