[webkit-reviews] review denied: [Bug 79873] [BlackBerry] Upstream LayerCompositingThread.{h, cpp} : [Attachment 129659] patch v2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 5 07:16:35 PST 2012
Rob Buis <rwlbuis at gmail.com> has denied Robin Cao
<robin.cao at torchmobile.com.cn>'s request for review:
Bug 79873: [BlackBerry] Upstream LayerCompositingThread.{h, cpp}
https://bugs.webkit.org/show_bug.cgi?id=79873
Attachment 129659: patch v2
https://bugs.webkit.org/attachment.cgi?id=129659&action=review
------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
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(); }
More information about the webkit-reviews
mailing list