Bug 79873

Created attachment 129415 [details] patch

Comment on attachment 129415 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129415&action=review Not much wrong, but still some cleanup to do. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:297 > + FloatQuad surfaceQuad = getTransformedRect(m_bounds, IntRect(IntPoint(), m_bounds), drawTransform); Better use IntPoint::zero > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:487 > + if (layer) Can combine these two lines. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:679 > + ASSERT_NOT_REACHED(); Can combine with AnimatedPropertyBackgroundColor case, they do the same. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:47 > +class SkBitmap; Not used? Please clean it up and the includes above.

Thanks for the review, all the comments will be addressed in a revised patch.

Created attachment 129659 [details] patch v2

Comment on attachment 129659 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=129659&action=review Looks good, can be improved some more. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:39 > +#include "HTMLCanvasElement.h" Is this one actually used? Please check all includes in general. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:152 > + result.setP2(drawTransform.mapPoint(FloatPoint(x, y+h))); Watch spacing "y + h" > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:153 > + result.setP3(drawTransform.mapPoint(FloatPoint(x+w, y+h))); Ditto. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:162 > +static bool enableVideoClipping = false; Can be moved into the function below, only used there. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:240 > + m_mediaPlayer->paint(0, IntRect((int)(x+0.5), (int)(y+0.5), m_bounds.width(), m_bounds.height())); Take care of the spacing please, x+0.5 -> x + 0.5 etc. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:285 > + // FIXME: is this going to happen? Might require an ASSERT if we are quite sure it will not happen. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:563 > +// FIXME: Copy pasted from WebCore::KeyframeAnimation!!! I am seeing this kind of FIXME a lot. Maybe we need a PR to track this? > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:57 > +class LayerMessage; Not used? > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:155 > + } Can be put on one line like above : size_t numSublayers() const { return m_sublayers.size(); }

Thanks for the review, comments inlined. (In reply to comment #5) > (From update of attachment 129659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129659&action=review > > Looks good, can be improved some more. > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:39 > > +#include "HTMLCanvasElement.h" > > Is this one actually used? Please check all includes in general. > All unnecessary includes are removed. > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:152 > > + result.setP2(drawTransform.mapPoint(FloatPoint(x, y+h))); > > Watch spacing "y + h" > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:153 > > + result.setP3(drawTransform.mapPoint(FloatPoint(x+w, y+h))); > > Ditto. > Fixed. > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:162 > > +static bool enableVideoClipping = false; > > Can be moved into the function below, only used there. > Fixed. > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:240 > > + m_mediaPlayer->paint(0, IntRect((int)(x+0.5), (int)(y+0.5), m_bounds.width(), m_bounds.height())); > > Take care of the spacing please, x+0.5 -> x + 0.5 etc. > Fixed. > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:285 > > + // FIXME: is this going to happen? > > Might require an ASSERT if we are quite sure it will not happen. > Replaced with ASSERT_NOT_REACHED(); > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:563 > > +// FIXME: Copy pasted from WebCore::KeyframeAnimation!!! > > I am seeing this kind of FIXME a lot. Maybe we need a PR to track this? > All layer animation related code have been moved to LayerAnimation.cpp. > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:57 > > +class LayerMessage; > > Not used? > Removed. > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:155 > > + } > > Can be put on one line like above : size_t numSublayers() const { return m_sublayers.size(); } Done.

Created attachment 133813 [details] patch-v3

Comment on attachment 133813 [details] patch-v3 Looks good.

Comment on attachment 133813 [details] patch-v3 Clearing flags on attachment: 133813 Committed r112110: <http://trac.webkit.org/changeset/112110>

All reviewed patches have been landed. Closing bug.