| Summary: | Use is<>() / downcast<>() for Filter / FilterOperation subclasses | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | Platform | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, darin, kling, rniwa | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 137056 | ||||||||||
| Bug Blocks: | 137657 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2014-10-11 21:11:33 PDT
Created attachment 239697 [details]
Patch
Created attachment 239708 [details]
Patch
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. 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. 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. Created attachment 239730 [details]
Patch
Comment on attachment 239730 [details] Patch Clearing flags on attachment: 239730 Committed r174654: <http://trac.webkit.org/changeset/174654> All reviewed patches have been landed. Closing bug. |