[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