[webkit-reviews] review denied: [Bug 103606] WebGL: Add a class to abstract the status of the Image in texImage2D() and texSubImage2D() : [Attachment 176669] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Jun Jiang <jun.a.jiang at intel.com>'s
request for review:
Bug 103606: WebGL: Add a class to abstract the status of the Image in
texImage2D() and texSubImage2D()
https://bugs.webkit.org/show_bug.cgi?id=103606

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.