[webkit-reviews] review denied: [Bug 79876] [BlackBerry] Upstream WebGL related files from platform/graphics : [Attachment 129701] patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Robin Cao
<robin.cao at torchmobile.com.cn>'s request for review:
Bug 79876: [BlackBerry] Upstream WebGL related files from platform/graphics
https://bugs.webkit.org/show_bug.cgi?id=79876

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129701&action=review


Looks good, some more cleanup can be done.

> Source/WebCore/platform/graphics/blackberry/DrawingBufferBlackBerry.cpp:66
> +// FIXME: The last two params are ignored for the BlackBerry platform. Is
this still true?

FIXME: fix comment. Last two params?

> Source/WebCore/platform/graphics/blackberry/DrawingBufferBlackBerry.cpp:113
> +	   return false;

I'd add an empty line here.

>
Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:36
> +#include "ChromeClientBlackBerry.h"

Both chrome includes do not seem used? In general check all includes.

>
Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:70
> +    m_context = BlackBerry::Platform::Graphics::createWebGLContext();

Could move into initializer list above as well.

>
Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:154

> +    return m_texture;

All above one-liners are candidates for inlining in the header.

>
Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:173

> +	       unsigned char *destRow = tempPixels + (imageWidth * 4 *
(imageHeight-y-1));

Watch spacing -> (imageHeight-y-1)

>
Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:207

> +}

Could be put in header as well.

> Source/WebCore/platform/graphics/blackberry/WebGLLayerWebKitThread.cpp:32
> +    , m_needsDisplay(0)

Huh?

> Source/WebCore/platform/graphics/blackberry/WebGLLayerWebKitThread.cpp:58
> +    // while we are drawing into it

Can be made one line, with a period at end.