Bug 137644 – Use is<>() / downcast<>() for Filter / FilterOperation subclasses

• Component: Platform
• Status: RESOLVED FIXED

[Chris Dumez]

Use is<>() / downcast<>() for Filter / FilterOperation subclasses

[Chris Dumez]

Created attachment 239697 [details]
Patch

[Chris Dumez]

Created attachment 239708 [details]
Patch

[Mihnea Ovidenie]

Comment on attachment 239708 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:488
> +        float amount = filterOperation ? downcast<BasicComponentTransferFilterOperation>(filterOperation)->amount() : 0;

I guess here you could also use downcast<BasicComponentTransferFilterOperation>(*filterOperation).amount() like above. Same below.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:82
>  {

I think it is worth to change o -> operation here too.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:166
>  {

Same here,i would change o -> operation.

[Darin Adler]

Comment on attachment 239708 [details]
Patch

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

Pleas consider all my comment optional things. Not sure they are all good ideas

One thing all these switch statements point to is the C++ anti-pattern where we don’t make sufficient use of polymorphism. Maybe this is more efficient?

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:82
> +            const DropShadowFilterOperation& dropShadowOperation = downcast<DropShadowFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:96
> +            const BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:161
> +            const BlurFilterOperation& blurOperation = downcast<BlurFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:170
> +            const BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:180
> +            const BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:201
> +            const BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:210
> +            const BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:220
> +            const BasicComponentTransferFilterOperation& componentTransferOperation = downcast<BasicComponentTransferFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:251
> +            const BasicComponentTransferFilterOperation& componentTransferOperation = downcast<BasicComponentTransferFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:274
> +            const BlurFilterOperation& blurOperation = downcast<BlurFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

>> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:488
>> +        float amount = filterOperation ? downcast<BasicComponentTransferFilterOperation>(filterOperation)->amount() : 0;
> 
> I guess here you could also use downcast<BasicComponentTransferFilterOperation>(*filterOperation).amount() like above. Same below.

Is that right? What guarantees filterOperation is non-null?

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:76
> +    const BasicColorMatrixFilterOperation* fromOperation = downcast<BasicColorMatrixFilterOperation>(from);

I suggest auto* or const auto* here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:85
> +    const BasicColorMatrixFilterOperation& other = downcast<BasicColorMatrixFilterOperation>(o);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:112
> +    const BasicComponentTransferFilterOperation* fromOperation = downcast<BasicComponentTransferFilterOperation>(from);

I suggest auto* or const auto* here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:121
> +    const BasicComponentTransferFilterOperation& other = downcast<BasicComponentTransferFilterOperation>(operation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:160
> +    const BlurFilterOperation* fromOperation = downcast<BlurFilterOperation>(from);

I suggest auto* or const auto* here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:169
> +    const DropShadowFilterOperation& other = downcast<DropShadowFilterOperation>(o);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:184
> +    const DropShadowFilterOperation* fromOperation = downcast<DropShadowFilterOperation>(from);

I suggest auto* or const auto* here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperations.cpp:108
>      for (size_t i = 0; i < m_operations.size(); ++i) {

Can this just use a modern for loop?

> Source/WebCore/platform/graphics/filters/FilterOperations.cpp:112
> +            const BlurFilterOperation& blurOperation = downcast<BlurFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/platform/graphics/filters/FilterOperations.cpp:120
> +            const DropShadowFilterOperation& dropShadowOperation = downcast<DropShadowFilterOperation>(filterOperation);

I suggest auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:144
>      for (size_t i = 0; i < operations.operations().size(); ++i) {

Can this just use a modern for loop?

> Source/WebCore/rendering/FilterEffectRenderer.cpp:149
> +            ReferenceFilterOperation& referenceOperation = downcast<ReferenceFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:183
> +            BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:211
> +            BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:218
> +            BasicColorMatrixFilterOperation& colorMatrixOperation = downcast<BasicColorMatrixFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:225
> +            BasicComponentTransferFilterOperation& componentTransferOperation = downcast<BasicComponentTransferFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:238
> +            BasicComponentTransferFilterOperation& componentTransferOperation = downcast<BasicComponentTransferFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:251
> +            BasicComponentTransferFilterOperation& componentTransferOperation = downcast<BasicComponentTransferFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:262
> +            BasicComponentTransferFilterOperation& componentTransferOperation = downcast<BasicComponentTransferFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:274
> +            BlurFilterOperation& blurOperation = downcast<BlurFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:280
> +            DropShadowFilterOperation& dropShadowOperation = downcast<DropShadowFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:101
>      for (size_t i = 0, size = operations.size(); i < size; ++i) {

Could we just use a modern for loop here?

> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:105
> +        ReferenceFilterOperation& referenceFilterOperation = downcast<ReferenceFilterOperation>(filterOperation);

I suggest auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebCore/rendering/RenderLayerFilterInfo.cpp:106
> +        CachedSVGDocumentReference* documentReference = referenceFilterOperation.cachedSVGDocumentReference();

I suggest auto* here since the type here isn’t all that interesting; we just use this immediately on the next line.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:73
>  FloatRect RenderSVGResourceFilterPrimitive::determineFilterPrimitiveSubregion(FilterEffect* effect)

Should take a FilterEffect&.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:75
> +    SVGFilter& filter = downcast<SVGFilter>(effect->filter());

Could consider auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:66
>      for (size_t i = 0; i < filters.size(); ++i) {

Maybe a modern for loop?

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:87
> +            const DropShadowFilterOperation& shadow = downcast<DropShadowFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:697
> +        const BasicColorMatrixFilterOperation& colorMatrixFilter = downcast<BasicColorMatrixFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:702
> +        const BasicColorMatrixFilterOperation& colorMatrixFilter = downcast<BasicColorMatrixFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:712
> +        const BasicColorMatrixFilterOperation& colorMatrixFilter = downcast<BasicColorMatrixFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:717
> +        const BasicComponentTransferFilterOperation& componentTransferFilter = downcast<BasicComponentTransferFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:722
> +        const BasicComponentTransferFilterOperation& componentTransferFilter = downcast<BasicComponentTransferFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:727
> +        const BasicComponentTransferFilterOperation& componentTransferFilter = downcast<BasicComponentTransferFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:732
> +        const BasicComponentTransferFilterOperation& componentTransferFilter = downcast<BasicComponentTransferFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:737
> +        const BlurFilterOperation& blurFilter = downcast<BlurFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:742
> +        const DropShadowFilterOperation& dropShadowFilter = downcast<DropShadowFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.mm:751
> +        const DefaultFilterOperation& defaultFilter = downcast<DefaultFilterOperation>(filter);

Could consider auto& or const auto& here since we don’t need to repeat the class name twice on the same line of code.

[Chris Dumez]

Comment on attachment 239708 [details]
Patch

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

>>> Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm:488
>>> +        float amount = filterOperation ? downcast<BasicComponentTransferFilterOperation>(filterOperation)->amount() : 0;
>> 
>> I guess here you could also use downcast<BasicComponentTransferFilterOperation>(*filterOperation).amount() like above. Same below.
> 
> Is that right? What guarantees filterOperation is non-null?

There is a null check on the same line :)

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:66
>>      for (size_t i = 0; i < filters.size(); ++i) {
> 
> Maybe a modern for loop?

I actually thought about that but FilterOperations does not have begin() / end() defined. We could add those methods in order to user modern loops but this seemed a bit out of scope for this patch.

[Chris Dumez]

Created attachment 239730 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 239730 [details]
Patch

Clearing flags on attachment: 239730

Committed r174654: <http://trac.webkit.org/changeset/174654>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.