Bug 79876 – [BlackBerry] Upstream WebGL related files from platform/graphics

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

[Robin Cao]

WebGL related files from platform/graphics:
    DrawingBufferBlackBerry.cpp
    GraphicsContext3DBlackBerry.cpp
    WebGLLayerWebKitThread.cpp
    WebGLLayerWebKitThread.h

[Robin Cao]

Created attachment 129701 [details]
patch

[Rob Buis]

Comment on attachment 129701 [details]
patch

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.

[Robin Cao]

(In reply to comment #2)
> (From update of attachment 129701 [details])
> 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?
> 

This comment seems irrelevant now, removed.

> > Source/WebCore/platform/graphics/blackberry/DrawingBufferBlackBerry.cpp:113
> > +        return false;
> 
> I'd add an empty line here.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:36
> > +#include "ChromeClientBlackBerry.h"
> 
> Both chrome includes do not seem used? In general check all includes.
> 
All unnecessary includes are removed now.
 
> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:70
> > +    m_context = BlackBerry::Platform::Graphics::createWebGLContext();
> 
> Could move into initializer list above as well.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:154
> > +    return m_texture;
> 
> All above one-liners are candidates for inlining in the header.
> 
All these functions are in cross platform header. We share the code with chromium there. We can consider inlining these functions when upstreaming the cross platform code.

> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:173
> > +            unsigned char *destRow = tempPixels + (imageWidth * 4 * (imageHeight-y-1));
> 
> Watch spacing -> (imageHeight-y-1)
> 
Good catch, fixed.

> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:207
> > +}
> 
> Could be put in header as well.
> 
Also cross platform header, we share the code with other ports.

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

I guess this is what you mean?

> > Source/WebCore/platform/graphics/blackberry/WebGLLayerWebKitThread.cpp:58
> > +    // while we are drawing into it
> 
> Can be made one line, with a period at end.
Done.

[Robin Cao]

Created attachment 130779 [details]
patch v2

[Rob Buis]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 129701 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129701&action=review
> > > Source/WebCore/platform/graphics/blackberry/WebGLLayerWebKitThread.cpp:32
> > > +    , m_needsDisplay(0)
> > 
> > Huh?
> > 
> m_needsDisplay(0) -> m_needsDisplay(false)
> 
> I guess this is what you mean?

Exactly :) I'll review again.

[Rob Buis]

Comment on attachment 130779 [details]
patch v2

LGTM.

[WebKit Review Bot]

Comment on attachment 130779 [details]
patch v2

Rejecting attachment 130779 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 124103 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 124103.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11869018

[Rob Buis]

Comment on attachment 130779 [details]
patch v2

LGTM.

[WebKit Review Bot]

Comment on attachment 130779 [details]
patch v2

Clearing flags on attachment: 130779

Committed r110171: <http://trac.webkit.org/changeset/110171>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.