[Webkit-unassigned] [Bug 52200] Small filter primitive renderer improvements

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 17 00:12:33 PST 2011


https://bugs.webkit.org/show_bug.cgi?id=52200


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78804|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #11 from Dirk Schulze <krit at webkit.org>  2011-01-17 00:12:32 PST ---
(From update of attachment 78804)
View in context: https://bugs.webkit.org/attachment.cgi?id=78804&action=review

I'd like to see Niko to look at this patch as well. Maybe he has some more proposals.

PS: Why do you add cq flag? You have commit rights and in my opinion it is better to land a patch yourself, you have to be available on landing anyway to fix problems.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:114
> +        builder->appendEffectToEffectReferences(effect, effectElement->renderer());

effect needs to be release if you change RefPtr to PassRefPtr in appendEffectToEffectReferences(...). Because with PassRefPtr you give over the ownership. This is maybe not what you want.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:48
> +    // They depend on the RenderObject argument of RenderSVGResourceFilter::applyResource.

They is unclear, do you mean FilterEffects or the subregion of a filter effect?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:71
> +void SVGFilterBuilder::appendEffectToEffectReferences(PassRefPtr<FilterEffect> passEffectReference, RenderObject* object)

I wouldn't change RefPtr to PassRefPtr, as long as we don't know who should take the ownership.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:57
> +    inline PassRefPtr<FilterEffect> getEffectByRenderer(RenderObject* object) { return m_effectRenderer.get(object); }

getters don't begin with 'get' for WebCore, just effectByRenderer. You're not calling this function anywhere, as far as I can see. Why did you add this here? Can you change the name of getEffectReferences() as well?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:76
>      HashMap<RefPtr<FilterEffect>, FilterEffectSet> m_effectReferences;
> +    HashMap<RenderObject*, RefPtr<FilterEffect> > m_effectRenderer;

Do both hashmaps need to take RefPtr<FilterEffect>? Can we garantee somehow, that if A has the element B has it as well? Would be better to save a pointer in at least one map.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list