Bug 179932 – Some FilterEffect cleanup and logging

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Some FilterEffect cleanup and logging

[Simon Fraser (smfr)]

Created attachment 327436 [details]
Patch

[EWS Watchlist]

Comment on attachment 327436 [details]
Patch

Attachment 327436 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5325070

New failing tests:
svg/animations/additive-type-by-animation.html

[EWS Watchlist]

Created attachment 327439 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 327436 [details]
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.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #4)
> Comment on attachment 327436 [details]
> 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.

Actually it returns a RefPtr<>:
    static RefPtr<GenericTypedArrayView> createUninitialized(unsigned length);

> > 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.

I'll clean this up in a followup; I want to add constness to all the source buffers.

https://trac.webkit.org/changeset/225137/webkit

[Radar WebKit Bug Importer]

<rdar://problem/35683971>