Bug 79867 – [BlackBerry] Upstream GraphicsLayerBlackBerry.{h, cpp}

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Robin Cao]

This is the BlackBerry implementation of GraphicsLayer.

[Robin Cao]

Created attachment 129411 [details]
patch

[Rob Buis]

Comment on attachment 129411 [details]
patch

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.

[Robin Cao]

(In reply to comment #2)
> (From update of attachment 129411 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129411&action=review
> 
> 
> > 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.

I tried the bit-field, but sizeof(GraphicsLayerBlackBerry) gives the same result. I prefer not to change it.

All the other comments will be addressed in a revised patch. Thank you very much!

[Robin Cao]

Created attachment 129652 [details]
patch v2

[Rob Buis]

Comment on attachment 129652 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=129652&action=review

Looks good, can be cleaned up some more.

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

Is PlatformString.h include really needed?

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:56
> +using namespace std;

Might not be needed if you use clampTo.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:332
> +    float clampedOpacity = max(min(opacity, 1.0f), 0.0f);

better use clampTo from MathExtras.h to make clear what you are doing here.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:356
> +        m_layer->setNeedsDisplayInRect(rect);

You could look at putting some the one/two liners in the header, but only if you do not need to add another include....

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:567
> +}

Above two functions look very similar, can we share code?

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:836
> +    m_contentsLayer->setPosition(FloatPoint(m_contentsRect.x(), m_contentsRect.y()));

better use m_contentsLayer->setPosition(m_contentsRect.location());

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:853
> +        m_contentsLayer->setAnchorPoint(FloatPoint(0, 0));

FloatPoint::zero

[Robin Cao]

(In reply to comment #5)
> (From update of attachment 129652 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129652&action=review
> 
> Looks good, can be cleaned up some more.
> 
> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:54
> > +#include "PlatformString.h"
> 
> Is PlatformString.h include really needed?
> 

Good catch, this is already indirectly included in GraphicsLayer.h, so can be removed.

> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:56
> > +using namespace std;
> 
> Might not be needed if you use clampTo.
> 
> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:332
> > +    float clampedOpacity = max(min(opacity, 1.0f), 0.0f);
> 
> better use clampTo from MathExtras.h to make clear what you are doing here.
> 

Fixed, clampTo is much more readable here.

> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:356
> > +        m_layer->setNeedsDisplayInRect(rect);
> 
> You could look at putting some the one/two liners in the header, but only if you do not need to add another include....
> 

Moving these two liner to header file will require one more include in the header. So I prefer not to move them.

> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:567
> > +}
> 
> Above two functions look very similar, can we share code?
> 

The two functions look very similar, but they have some subtle differences, which make it hard to share code here. We can refactor the code later.

> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:836
> > +    m_contentsLayer->setPosition(FloatPoint(m_contentsRect.x(), m_contentsRect.y()));
> 
> better use m_contentsLayer->setPosition(m_contentsRect.location());
> 

I made the following changes:
-    m_contentsLayer->setPosition(FloatPoint(m_contentsRect.x(), m_contentsRect.y()));
-    m_contentsLayer->setBounds(IntSize(m_contentsRect.width(), m_contentsRect.height()));
+    m_contentsLayer->setPosition(m_contentsRect.location());
+    m_contentsLayer->setBounds(m_contentsRect.size());


> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:853
> > +        m_contentsLayer->setAnchorPoint(FloatPoint(0, 0));
> 
> FloatPoint::zero

Fixed.

[Robin Cao]

Created attachment 130582 [details]
patch v3

[Rob Buis]

Comment on attachment 130582 [details]
patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=130582&action=review

Almost there, have a look especially at the release issue. Maybe you can get Leo to cq+ for you once fixed.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:146
> +    if (GraphicsLayer::replaceChild(oldChild, newChild)) {

I think early return may look better here.

> Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:384
> +            return layerAnimation.release();

I dont think you want to release here? Note that removeAnimationByIdAndProperty doesnt do it either.

[Robin Cao]

(In reply to comment #8)
> (From update of attachment 130582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130582&action=review
> 
> Almost there, have a look especially at the release issue. Maybe you can get Leo to cq+ for you once fixed.
> 
> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:146
> > +    if (GraphicsLayer::replaceChild(oldChild, newChild)) {
> 
> I think early return may look better here.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp:384
> > +            return layerAnimation.release();
> 
> I dont think you want to release here? Note that removeAnimationByIdAndProperty doesn't do it either.

"return layerAnimation.release();" is equivalent to "return layerAnimation;" in functionality. But for readability and consistency it makes sense to use "return layerAnimation;" in both functions.

[Robin Cao]

Created attachment 130783 [details]
patch-for-landing

[WebKit Review Bot]

Comment on attachment 130783 [details]
patch-for-landing

Clearing flags on attachment: 130783

Committed r110156: <http://trac.webkit.org/changeset/110156>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.