FEComponentTransfer cleanup and optimization

[Simon Fraser (smfr)]

FEComponentTransfer cleanup and optimization

[Simon Fraser (smfr)]

Created attachment 327462 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:173:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:174:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:175:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:176:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:177:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:178:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:181:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:182:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:183:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
ERROR: Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:184:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 10 in 3 files


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

[Darin Adler]

Comment on attachment 327462 [details]
Patch

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

Not a huge fan of all the abbreviations here. I would say m_redFunction instead m_redFunc, and redTable instead of rTable, etc.

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:73
> +void FEComponentTransfer::computeIdentityTable(FEComponentTransfer::LookupTable, const ComponentTransferFunction&)

Can just call it LookupTable because this is inside the declaration of a FEComponentTransfer member function.

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:150
> +        unsigned char c = data[pixelOffset];
> +        data[pixelOffset] = rTable[c];

auto or uint8_t or do it in single lines without local variables so we don’t need all the comments:

    data[pixelOffset] = rTable[data[pixelOffset]];
    data[pixelOffset + 1] = gTable[data[pixelOffset + 1]];
    data[pixelOffset + 2] = bTable[data[pixelOffset + 2]];
    data[pixelOffset + 3] = aTable[data[pixelOffset + 3]];

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:171
> +    typedef void (*TransferType)(LookupTable, const ComponentTransferFunction&);

We are using "using" these days:

    using TransferType = void (*)(LookupTable, const ComponentTransferFunction&);

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.cpp:186
> +    (*callEffect[m_redFunc.type])(rValues, m_redFunc);

I don’t think the (*x) is needed. Should just be able to call:

    callEffect[m_redFunc.type](rValues, m_redFunc);

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:76
> +    typedef uint8_t LookupTable[lookupTableSize];

Better to use using. But even better to use std::array so the strangeness of C arrays doesn’t affect us so much. Then when you want to modify the table you have to pass LookupTable& instead. And you can call size() instead of having a separate constant.

    using LookupTable = std::array<uint8_t, 256>;

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:78
> +    static void computeIdentityTable(FEComponentTransfer::LookupTable, const ComponentTransferFunction&);

Same here, no need to write FEComponentTransfer:: prefix.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/225107/webkit

[Radar WebKit Bug Importer]

<rdar://problem/35672813>