[webkit-reviews] review granted: [Bug 179932] Some FilterEffect cleanup and logging : [Attachment 327436] 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 179932: Some FilterEffect cleanup and logging
https://bugs.webkit.org/show_bug.cgi?id=179932

Attachment 327436: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 327436
  --> https://bugs.webkit.org/attachment.cgi?id=327436
Patch

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

And of the test failures seem like they could be related to this patch? Maybe
the svg/animations/additive-type-by-animation.html one?

> Source/WebCore/platform/graphics/filters/FilterEffect.h:61
> +    RefPtr<Uint8ClampedArray> unmultipliedResult(const IntRect&);
> +    RefPtr<Uint8ClampedArray> premultipliedResult(const IntRect&);

These functions, and others like them, seem like they should be returning Ref,
not RefPtr. Their implementations seem to return a value that in turn comes
from a function that returns a Ref.

> Source/WebCore/platform/graphics/filters/FilterEffect.h:63
> +    void copyUnmultipliedResult(Uint8ClampedArray* destination, const
IntRect&);
> +    void copyPremultipliedResult(Uint8ClampedArray* destination, const
IntRect&);

These functions, and others like them, seem like they should take a reference
rather than a pointer to the destination. This patch already is touching every
call site since the name is changing, which seems like an opportunity to change
this.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:234
> +	   m_rendererFilterDataMap.remove(&renderer);

This is double tasing. More efficient to use find above and then remove using
the iterator rather than having to hash again. find/remove is faster than
get/remove.