Bug 79876

Created attachment 129701 [details] patch

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.

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

Created attachment 130779 [details] patch v2

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

Comment on attachment 130779 [details] patch v2 LGTM.

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

Comment on attachment 130779 [details] patch v2 LGTM.

Comment on attachment 130779 [details] patch v2 Clearing flags on attachment: 130779 Committed r110171: <http://trac.webkit.org/changeset/110171>

All reviewed patches have been landed. Closing bug.