Bug 79867

Created attachment 129411 [details] patch

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.

(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!

Created attachment 129652 [details] patch v2

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

(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.

Created attachment 130582 [details] patch v3

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.

(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.

Created attachment 130783 [details] patch-for-landing

Comment on attachment 130783 [details] patch-for-landing Clearing flags on attachment: 130783 Committed r110156: <http://trac.webkit.org/changeset/110156>

All reviewed patches have been landed. Closing bug.