Bug 47027 – refactor the nested large switch statements in GraphicsContext3DCG.cpp:getImageData()

• Component: WebGL
• Status: RESOLVED FIXED

[Zhenyao Mo]

Currently it's getting out of hand.  kbr suggested to use table instead.

[Zhenyao Mo]

Created attachment 71449 [details]
patch

[Zhenyao Mo]

Created attachment 71451 [details]
revised patch: remove an extra whitespace

[WebKit Review Bot]

Attachment 71451 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:65:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:66:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:68:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhenyao Mo]

I think I am not going to fix the style complaints because the current arrangement is much better for readability.

[Kenneth Russell]

Comment on attachment 71451 [details]
revised patch: remove an extra whitespace

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

[Zhenyao Mo]

Created attachment 72058 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 72058 [details]
revised patch: responding to kbr's review

View in context: https://bugs.webkit.org/attachment.cgi?id=72058&action=review

Looks good. One comment.

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

Please add a comment indicating that this returns kSourceFormatNumFormats if the combination of input parameters is unsupported.

[Zhenyao Mo]

Committed r70706: <http://trac.webkit.org/changeset/70706>