[webkit-reviews] review denied: [Bug 47027] refactor the nested large switch statements in GraphicsContext3DCG.cpp:getImageData() : [Attachment 71451] revised patch: remove an extra whitespace

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Zhenyao Mo <zmo at google.com>'s
request for review:
Bug 47027: refactor the nested large switch statements in
GraphicsContext3DCG.cpp:getImageData()
https://bugs.webkit.org/show_bug.cgi?id=47027

Attachment 71451: revised patch: remove an extra whitespace
https://bugs.webkit.org/attachment.cgi?id=71451&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=71451&action=review

This looks really nice overall (thanks for doing this cleanup) but there are a
few issues that need to be addressed.

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:52
> +    kSourceFormatBaseUndefined

It turns out WebKit constant style is not to use the "k" prefix. So,
"SourceFormatBaseR", etc. Please add a SourceFormatBaseNumFormats or similar
for below.

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:58
> +    kAlphaFormatLast,

Naming convention. Please also add an AlphaFormatNumFormats (collision with
AlphaFormatLast is unfortunate).

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:61
> +static GraphicsContext3D::SourceDataFormat getSourceDataFormat(unsigned int
componentsPerPixel, AlphaFormat alphaFormat, bool bit16, bool bigEndian)

bit16 -> is16BitFormat

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:63
> +    const static SourceDataFormatBase tableFormatBase[4][3] = { //
componentsPerPixel x AlphaFormat

I think this would read better as "formatTableBase". Please also use
AlphaFormatNumFormats as the second array dimension.

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:65
> +	   { kSourceFormatBaseR,	 kSourceFormatBaseA,	    
kSourceFormatBaseA },	      // 1 componentsPerPixel

Please fix the style errors. You can put the extra spaces to the left of the
"}".

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:70
> +    const static GraphicsContext3D::SourceDataFormat tableFormat[7][3] = {
// SourceDataFormatBase x bitsPerComponentAndEndian

Instead of [7] use SourceFormatBaseNumFormats. Also, I think this would read
better as "formatTable".