[Webkit-unassigned] [Bug 103606] WebGL: Add a class to abstract the status of the Image in texImage2D() and texSubImage2D()

[bugzilla-daemon at webkit.org]

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


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #176669|review?                     |review-
               Flag|                            |




--- Comment #2 from Kenneth Russell <kbr at google.com>  2012-11-29 13:04:05 PST ---
(From update of attachment 176669)
View in context: https://bugs.webkit.org/attachment.cgi?id=176669&action=review

Thanks for doing this restructuring. It looks pretty good overall but a few changes are needed. Please upload a revised patch fixing these issues and I'll gladly r+ it. Please mark it cq? if you want it submitted to the commit queue (I strongly recommend you go through the commit queue since it touches nearly all the ports). Also, please don't forget to regenerate the ChangeLog if you rename the methods and classes as I suggested.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3629
> +    void* imagePixelData = const_cast<void*>(imageLocker.imagePixelData());

Please fix the signature of texImage2DBase to take const void* instead of casting away const here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3639
> +    }

It's WebKit policy to keep patches focused and not introduce unrelated changes like this optimization. I would prefer you do it in a second patch, but since it's pretty trivial I won't argue if you want to do it here. Same for texSubImage2DImpl below.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3879
> +    void* imagePixelData = const_cast<void*>(imageLocker.imagePixelData());

Please fix the signature of texSubImage2DBase to take const void* instead of casting away const here.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:879
> +    // flags. Returns true upon success.

This comment is now out of date; please update it. In particular, ignoreGammaAndColorProfile is only given to the ImageLocker, and AlphaOp is now passed here directly. I think this method should also be renamed to "packImageData". Suggested text:

Packs the contents of the given image, passed in |pixels|, into the passed Vector, according to the given format and type, and obeying the flipY and AlphaOp flags. Returns true upon success.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:882
> +    class ImageLocker {

Upon seeing how this class is used, I think it should be named ImageExtractor. See further comments below.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:883
> +    public:

Indent by two spaces here.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:885
> +        bool getImageData(bool premultiplyAlpha, bool ignoreGammaAndColorProfile);

getImageData shouldn't be public. Having it be public means it could be called multiple times and I don't think the implementations are prepared to handle that. Please change the constructor to take "Image* image, bool premultiplyAlpha, bool ignoreGammaAndColorProfile" and call getImageData (or whatever it ends up being called) inside the constructor. Set a flag called m_succeeded (e.g., "m_succeeded = getImageData(...)"), add a "bool succeeded()" method returning that flag, and in WebGLRenderingContext, call succeeded() instead of getImageData. Also, please document explicitly that each platform needs to implement the destructor and getImageData. The constructor should be the first public method in the class; please move it up. Also, I think getImageData should now be renamed to something else, like extractImage -- it doesn't behave like a getter any more.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:887
> +        ImageLocker(Image* image) {m_image = image;};

Please move the body of the constructor into GraphicsContext3D.cpp, since it will become non-trivial.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:888
> +        const void* imagePixelData() {return m_imagePixelData;};

Please add spaces inside the braces and get rid of the ';'s at the end of the lines here and below.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:894
> +    private:

Indent by two spaces here.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:902
> +        CGImageRef cgImage;

These three members need to use the m_ naming convention.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:917
> +    private:

Indentation is wrong here. Back up "private" by two spaces.

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