[Webkit-unassigned] [Bug 79876] [BlackBerry] Upstream WebGL related files from platform/graphics

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 7 20:31:27 PST 2012


https://bugs.webkit.org/show_bug.cgi?id=79876





--- Comment #3 from Robin Cao <robin.cao at torchmobile.com.cn>  2012-03-07 20:31:27 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list