Bug 79871

Created attachment 129422 [details] patch

Comment on attachment 129422 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129422&action=review Looks good, still some things to improve. > Source/WebCore/platform/graphics/blackberry/LayerData.h:156 > + bool includeVisibility() Could be made const. > Source/WebCore/platform/graphics/blackberry/LayerData.h:192 > + LayerProgramShader m_layerProgramShader; Could move this one so all the bools can become a bitfield, saving some mem. Not sure how often LayerData is created though. > Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.cpp:426 > + return m_superlayer; Why not inline this one? > Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.h:47 > +#include <wtf/text/StringHash.h> Are all of these included needed? > Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.h:156 > + bool isDrawable() { return m_isDrawable; } const > Source/WebCore/platform/graphics/blackberry/PluginLayerWebKitThread.cpp:79 > + m_pluginView->updateBuffer(IntRect(IntPoint(0, 0), m_pluginView->size())); IntPoint::zero > Source/WebCore/platform/graphics/blackberry/VideoLayerWebKitThread.cpp:58 > + // clear hole punch rect Not a sentence. > Source/WebCore/platform/graphics/blackberry/VideoLayerWebKitThread.cpp:90 > + setHolePunchRect(IntRect(0, 0, m_bounds.width(), m_bounds.height())); Probably more efficient to do IntRect(IntPoint::zero(), m_bounds); > Source/WebCore/platform/graphics/blackberry/VideoLayerWebKitThread.cpp:94 > + if (frameView) Can be combined.

(In reply to comment #2) > (From update of attachment 129422 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129422&action=review > > Looks good, still some things to improve. > > > Source/WebCore/platform/graphics/blackberry/LayerData.h:156 > > + bool includeVisibility() > > Could be made const. > > > Source/WebCore/platform/graphics/blackberry/LayerData.h:192 > > + LayerProgramShader m_layerProgramShader; > > Could move this one so all the bools can become a bitfield, saving some mem. Not sure how often LayerData is created though. > > > Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.cpp:426 > > + return m_superlayer; > > Why not inline this one? > > > Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.h:47 > > +#include <wtf/text/StringHash.h> > > Are all of these included needed? > > > Source/WebCore/platform/graphics/blackberry/LayerWebKitThread.h:156 > > + bool isDrawable() { return m_isDrawable; } > > const > > > Source/WebCore/platform/graphics/blackberry/PluginLayerWebKitThread.cpp:79 > > + m_pluginView->updateBuffer(IntRect(IntPoint(0, 0), m_pluginView->size())); > > IntPoint::zero > > > Source/WebCore/platform/graphics/blackberry/VideoLayerWebKitThread.cpp:58 > > + // clear hole punch rect > > Not a sentence. > > > Source/WebCore/platform/graphics/blackberry/VideoLayerWebKitThread.cpp:90 > > + setHolePunchRect(IntRect(0, 0, m_bounds.width(), m_bounds.height())); > > Probably more efficient to do IntRect(IntPoint::zero(), m_bounds); > > > Source/WebCore/platform/graphics/blackberry/VideoLayerWebKitThread.cpp:94 > > + if (frameView) > > Can be combined. Thanks for the review, I'll address all your comments above in a revised patch.

Created attachment 129638 [details] patch v2

Comment on attachment 129638 [details] patch v2 Clearing flags on attachment: 129638 Committed r109668: <http://trac.webkit.org/changeset/109668>

All reviewed patches have been landed. Closing bug.