[webkit-reviews] review granted: [Bug 137644] Use is<>() / downcast<>() for Filter / FilterOperation subclasses : [Attachment 239708] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 137644: Use is<>() / downcast<>() for Filter / FilterOperation subclasses
https://bugs.webkit.org/show_bug.cgi?id=137644

Attachment 239708: Patch
https://bugs.webkit.org/attachment.cgi?id=239708&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(FilterEffec
t* 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.