[Webkit-unassigned] [Bug 79873] [BlackBerry] Upstream LayerCompositingThread.{h, cpp}
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 26 07:55:21 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=79873
--- Comment #6 from Robin Cao <robin.cao at torchmobile.com.cn> 2012-03-26 07:55:21 PST ---
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list