[webkit-reviews] review granted: [Bug 179952] FEComponentTransfer cleanup and optimization : [Attachment 327462] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 179952: FEComponentTransfer cleanup and optimization
https://bugs.webkit.org/show_bug.cgi?id=179952

Attachment 327462: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 327462
  --> https://bugs.webkit.org/attachment.cgi?id=327462
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.