[webkit-reviews] review denied: [Bug 57464] Clipping in accelerated Canvas2D should be faster : [Attachment 87552] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 57464: Clipping in accelerated Canvas2D should be faster
https://bugs.webkit.org/show_bug.cgi?id=57464

Attachment 87552: Patch
https://bugs.webkit.org/attachment.cgi?id=87552&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87552&action=review

The idea is good but the implementation needs cleanup.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:72
> +    Path m_path;
> +    AffineTransform m_transform;

In WebKit style the m_ prefix isn't needed for members of structs.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:81
> +	   , m_numClippingPaths(0)

Please add a method to this struct like "bool clippingEnabled()" which does the
test of m_numClippingPaths > 0. The tests strewn throughout this patch are
gross and will be fragile in the long run. Even better, make the method call
m_clippingPaths.size() and remove the duplicated m_numClippingPaths member
entirely.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:221
> +    if (m_state->m_ctm.isIdentity() && !m_state->m_numClippingPaths) {

Replace the test of m_numClippingPaths with a method call.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254
> +    applyClipping(m_state->m_numClippingPaths > 0);

Here and below, please replace the test of "m_state->m_numClippingPaths > 0"
with a method call.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:355
> +    m_state->m_clippingPaths.append(PathAndTransform(path, m_state->m_ctm));

> +    m_state->m_numClippingPaths++;

This looks like replicated data. m_numClippingPaths == m_clippingPaths.size().