Bug 79871 – [BlackBerry] Upstream LayerWebKitThread and its derived classes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Robin Cao]

LayerWebKitThread is the BlackBerry implementation of PlatformLayer.

[Robin Cao]

Created attachment 129422 [details]
patch

[Rob Buis]

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.

[Robin Cao]

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

[Robin Cao]

Created attachment 129638 [details]
patch v2

[WebKit Review Bot]

Comment on attachment 129638 [details]
patch v2

Clearing flags on attachment: 129638

Committed r109668: <http://trac.webkit.org/changeset/109668>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.