Some FilterEffect cleanup and logging
Created attachment 327436 [details] Patch
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
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
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.
(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
<rdar://problem/35683971>