377149 - stop storing mask and clip-path frames

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Storing frames is bad as we get into problems tracking their destruction.

Possibilities:

a) store the element in the property. Would this work properly with reference counting?

b) create another class to store the properties as per marker and filter frames. And store an nsrefptr to the element in that. Downside is that you store the pointer to this class which has a pointer to the element so you use twice the memory. Having said that you only use it if you have one so perhaps this is the way to go.

Comment 1

[Robert Longson [:longsonr]]

Attachment: mask crash

Comment 2

[Robert Longson [:longsonr]]

You may need to force a refresh e.g. by covering the circle with another application and uncovering it again after you have clicked on click me in order to bang.

Comment 3

[Robert Longson [:longsonr]]

having thought about it some more, without a mutation listener you don't know when the elements will disappear so neither a) nor b) are any good so we could do:

c) don't use properties at all and just look up the frames every time you want to use them.

d) put in mutation listener classes for mask and clip-path as per filters and markers

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

also makes mask and clip paths somewhat live.

Comment 5

[Robert Longson [:longsonr]]

Attachment: testcase

shape should disappear when clicked on.

Comment 6

[tor]

This seems to have considerable overlap with bug 374598 (and to a lesser extent bug 377015).

Comment 7

[Robert Longson [:longsonr]]

Attachment: address review comment

Update to tip. 

bug 377015 has landed and I've gone for the destructor approach throughout so that filter, clipPath and mask mutation observers are all now like nsSVGMarkerProperty.

Comment 8

[Robert Longson [:longsonr]]

Attachment: diff -w

Comment 9

[Robert Longson [:longsonr]]

If this is OK I'll make bug 374598 invalid.

Comment 10

[tor]

In nsSVGMaskFrame::ComputeMaskAlpha and nsSVGMaskFrame::GetCanvasTM, you can look at the mask element's fields directly instead of calling mask->Get...Units().

This changes our behavior of a display:none filter - before we would ignore the filter, but now it looks like we won't draw anything.  While neither is per spec, the former behavior seems closer to what we should be doing.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #10)
> In nsSVGMaskFrame::ComputeMaskAlpha and nsSVGMaskFrame::GetCanvasTM, you can
> look at the mask element's fields directly instead of calling
> mask->Get...Units().

Done.

> 
> This changes our behavior of a display:none filter - before we would ignore the
> filter, but now it looks like we won't draw anything.  While neither is per
> spec, the former behavior seems closer to what we should be doing.
> 

Doesn't the code already do that via bug 35775 :-) Anyway I think this new patch addresses that issue.

Comment 12

[tor]

(In reply to comment #11)
> > This changes our behavior of a display:none filter - before we would ignore the
> > filter, but now it looks like we won't draw anything.  While neither is per
> > spec, the former behavior seems closer to what we should be doing.
> 
> Doesn't the code already do that via bug 35775 :-) Anyway I think this new
> patch addresses that issue.

I'm not sure what bug you intended to point to, but the existing code won't mark the frame as filtered because it won't be able to find a filter frame.  Your patch would mark it filtered because it only looks for the element.

Comment 13

[Robert Longson [:longsonr]]

(In reply to comment #12)
> I'm not sure what bug you intended to point to, but the existing code won't
> mark the frame as filtered because it won't be able to find a filter frame. 
> Your patch would mark it filtered because it only looks for the element.
> 

I'm failing to see something then because this looks to me like you get the frame marked both with/without this patch. There may be a problem with clipping though as you need a frame to determine whether to clip trivial or not.

   if (style->mFilter && !(aFrame->GetStateBits() & NS_STATE_SVG_FILTERED)) {
     nsIContent *filter = NS_GetSVGFilterElement(style->mFilter,
                                                 aFrame->GetContent());
-    if (filter) {
-      nsSVGFilterProperty *property = new nsSVGFilterProperty(filter, aFrame);
-      if (!property) {
-        NS_ERROR("Could not create filter property");
-        return;
-      }
-      NS_ADDREF(property); // addref to allow QI - FilterPropertyDtor releases
-      aFrame->SetProperty(nsGkAtoms::filter,
-                          NS_STATIC_CAST(nsISupports*, property),
-                          nsPropertyTable::SupportsDtorFunc);
-      aFrame->AddStateBits(NS_STATE_SVG_FILTERED);
+    if (filter && !new nsSVGFilterProperty(filter, aFrame)) {
+      NS_ERROR("Could not create filter property");
+      return;
     }
   }
 
The current patch does fix this though I think, the frame is marked filtered whenever there is an element and then PaintChildWithEffect draws it unfiltered when it can't find a frame.

Comment 14

[tor]

A concern about this patch is that you only add the clip bits if the frame is around, so StyleEffects might end up leaving a property around for a lot longer than is necessary.  Two possibilities for fixing this are to unconditionally try removing the property, or switch to a single clip state bit and adjust nsSVGUtils to query the complexity when needed.

Comment 15

[Robert Longson [:longsonr]]

Attachment: address review comment

Went for a single clip bit.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 262489 [details] [diff] [review]
address review comment

+  nsWeakPtr mObserved;

Say mObservedContent, to make it clear what's being observed?

-  if (state & NS_STATE_SVG_CLIPPED_MASK) {
+  if (state & NS_STATE_SVG_CLIPPED)
     aFrame->DeleteProperty(nsGkAtoms::clipPath);
-  }

Don't trim off these braces.

Comment 17

[Robert Longson [:longsonr]]

Attachment: version for check in with sr comments addressed

Comment 18

[Robert Longson [:longsonr]]

checked in.