374598 - simplify filter property destruction

Status: RESOLVED DUPLICATE of bug 377149

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Comment 1

[Jonathan Watt [:jwatt]]

Comment on attachment 259089 [details] [diff] [review]
patch

How about also setting NS_STATE_SVG_FILTERED in the ctor so it can be removed from AddEffectProperties, and...

>@@ -996,25 +994,20 @@ nsSVGUtils::StyleEffects(nsIFrame *aFram
> {
>   nsFrameState state = aFrame->GetStateBits();
> 
>   /* clear out all effects */
> 
>   if (state & NS_STATE_SVG_CLIPPED_MASK) {
>     aFrame->DeleteProperty(nsGkAtoms::clipPath);
>   }
> 
>   if (state & NS_STATE_SVG_FILTERED) {
>-    nsSVGFilterProperty *property;
>-    property = NS_STATIC_CAST(nsSVGFilterProperty *,
>-                              aFrame->GetProperty(nsGkAtoms::filter));
>-    if (property)
>-      property->RemoveMutationObserver();
>     aFrame->DeleteProperty(nsGkAtoms::filter);
>   }
> 
>   if (state & NS_STATE_SVG_MASKED) {
>     aFrame->DeleteProperty(nsGkAtoms::mask);
>   }
> 
>   aFrame->RemoveStateBits(NS_STATE_SVG_CLIPPED_MASK |
>                           NS_STATE_SVG_FILTERED |
>                           NS_STATE_SVG_MASKED);

...removing NS_STATE_SVG_FILTERED from here since it will be removed by the DeleteProperty. That way the setting and clearing of NS_STATE_SVG_FILTERED is fully encapsulated within nsSVGFilterProperty.

Comment 2

[Jonathan Watt [:jwatt]]

Oh, and please make the dtor virtual.

Comment 3

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #1)
> (From update of attachment 259089 [details] [diff] [review])
> How about also setting NS_STATE_SVG_FILTERED in the ctor so it can be removed
> from AddEffectProperties, and...

Good idea. I added in the other creation guff from AddEffectProperties too.

> ...removing NS_STATE_SVG_FILTERED from here since it will be removed by the
> DeleteProperty. That way the setting and clearing of NS_STATE_SVG_FILTERED is
> fully encapsulated within nsSVGFilterProperty.
> 

Done, although it doesn't cost us anything. I hope that won't confuse anyone looking at the code later wondering why we only clear 2/3 flags.

Comment 4

[Jonathan Watt [:jwatt]]

Are we sure that aFrame->DeleteProperty will definitely result in the object being deleted immediately? Even if API stuff changes in future? I'm not sure that DeleteProperty necessarily means a property will be freed. Perhaps it's likely to be seen by others (in future) to only mean "remove from the element"? I'd like to look at this more closely, but I have to head out right now.

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #4)
> Are we sure that aFrame->DeleteProperty will definitely result in the object
> being deleted immediately? Even if API stuff changes in future? I'm not sure
> that DeleteProperty necessarily means a property will be freed. Perhaps it's
> likely to be seen by others (in future) to only mean "remove from the element"?
> I'd like to look at this more closely, but I have to head out right now.
> 

We ensure immediate destruction by the way we call SetProperty in the nsSVGFilterProperty constructor. It passes in a pointer to itself as the destructor function to call when DeleteProperty is called. This is documented as part of the nsINode.h API. If someone changes the API to have different semantics to those currently described in the .h file they will have to change the callers too.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 259095 [details] [diff] [review]
address review comments

nsSVGFilterProperty::nsSVGFilterProperty(nsISVGFilterFrame *aFilter,
>                                          nsIFrame *aFrame)
>   : mFilter(aFilter), mFrame(aFrame)
> {
>+  NS_ADDREF(this); // addref to allow QI - FilterPropertyDtor releases

FilterPropertyDtor no longer exists. It should be nsPropertyTable::SupportsDtorFunc. Also I'm not sure I'm too keen on moving the addref into the ctor.


(In reply to comment #5)
> We ensure immediate destruction by the way we call SetProperty in the
> nsSVGFilterProperty constructor. It passes in a pointer to itself as the
> destructor function to call when DeleteProperty is called.

What we pass in to use as the destructor is nsPropertyTable::SupportsDtorFunc. Therefore all we ensure is immediate _release_, which doesn't necessarily mean immediate destruction if something else ends up holding a reference to the property (say due to future code changes?)

Comment 7

[Jonathan Watt [:jwatt]]

I actually can't seen any reason for nsSVGFilterProperty to implement nsISupports. Maybe that should be removed and we should bring back FilterPropertyDtor. I'm really not familiar with the filter code so I don't know why that was killed in the first place. Maybe tor knows.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)
> I actually can't seen any reason for nsSVGFilterProperty to implement
> nsISupports. Maybe that should be removed and we should bring back
> FilterPropertyDtor. I'm really not familiar with the filter code so I don't
> know why that was killed in the first place. Maybe tor knows.
> 

Bug 359516 comment 14

Comment 9

[Robert Longson [:longsonr]]

Attachment: alternative patch

I'd like to iron out some differences between the marker and filter listener code. Either by modifying the marker code not to use a destructor or the filter code to have one (see previous patch). Which way would you like to go?

Comment 10

[tor]

How is this patch simplifying destruction?  It seems that by moving code out of the constructor and destructor you've made things more complicated and fragile for the users.