330498 - One set of SVG effects (mask/filter/clipPath) logic

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: move svg effects logic to nsSVGUtils

Comment 2

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

I don't think you need to call StyleEffects in destructors. Frame destruction should automatically destroy all properties.

Rename PaintEffects to PaintChild, or possibly PaintChildWithEffects.

+  nsCOMPtr<nsISVGRendererSurface> closure;

Doesn't seem to be used.

+      property = (nsSVGFilterProperty *)aFrame->GetProperty(nsGkAtoms::filter);

NS_STATIC_CAST

We should deCOM FindFilterInvalidation, but it doesn't have to be done now.

   nsCOMPtr<nsISVGRendererRegion> region, tmp;

region can be a regular pointer, tmp is unused.

+  /* Style change effects (filter/clip/mask/opacity) */
+  /* Modification effects (filter/clip/mask/opacity) */

These could say a little bit more. E.g., "Notify that the frame's style may have changed (or the frame is being deleted)."

+    retval = matrix.get();
+    NS_IF_ADDREF(retval);

I think you can use "matrix.swap(retval);" here.

+  const nsStyleSVGReset *style = NS_STATIC_CAST(const nsStyleSVGReset *,
+                                                aFrame->GetStyleData(eStyleStruct_SVGReset));

Why can't you use aFrame->GetStyleSVGReset()?

+  float opacity = aFrame->GetStyleContext()->GetStyleDisplay()->mOpacity;

aFrame->GetStyleDisplay()

You might want to split off the "UpdateEffectsProperties" part of PaintEffects into its own static helper function, for readability.

+  if (opacity != 1.0 ||
+      state & (NS_STATE_SVG_CLIPPED_COMPLEX | NS_STATE_SVG_MASKED) ||
+      ((state & NS_STATE_SVG_FILTERED) && (state & NS_STATE_SVG_CLIPPED_TRIVIAL))) {

Why do you not need a 

+    property = (nsSVGFilterProperty *)aFrame->GetProperty(nsGkAtoms::filter);
+    clip = (nsISVGClipPathFrame *)aFrame->GetProperty(nsGkAtoms::clipPath);
+    clip = (nsISVGClipPathFrame *)aFrame->GetProperty(nsGkAtoms::clipPath);
+    mask = (nsISVGMaskFrame *)aFrame->GetProperty(nsGkAtoms::mask);

NS_STATIC_CAST

+    nsISVGClipPathFrame *clip;
+    
+    clip = (nsISVGClipPathFrame *)aFrame->GetProperty(nsGkAtoms::clipPath);

Here and elsewhere you can write
    nsISVGClipPathFrame *clip =
      NS_STATIC_CAST(nsISVGClipPathFrame *, aFrame->GetProperty(nsGkAtoms::clipPath));

+    if (state & NS_STATE_SVG_CLIPPED_TRIVIAL) {
+      aCanvas->PushClip();
+    } else {

Where does that get Popped?

I have a really hard time following the logic in PaintEffects. Perhaps it would be easier if instead of matching if (XYZ) Push... ...... if (XYZ) Pop..., then enclosed code could be moved into a helper function and we just do "if (XYZ) { ...; Helper(); ... } else { Helper(); }".

+void
+nsSVGUtils::AddObserver(nsISupports *aObserver, nsISupports *aTarget)
+void
+nsSVGUtils::RemoveObserver(nsISupports *aObserver, nsISupports *aTarget)

Make these static functions (not members). They'll get optimized better.

+void
+nsSVGUtils::FilterPropertyDtor(void *aObject, nsIAtom *aPropertyName,
+                               void *aPropertyValue, void *aData)

ditto

+  nsSVGFilterProperty *property = (nsSVGFilterProperty *)aPropertyValue;

NS_STATIC_CAST

+    nsSVGFilterProperty *property;
+    property = (nsSVGFilterProperty *)aFrame->GetProperty(nsGkAtoms::filter);
+    // XXX - invalidate top filter region

If you aren't going to invalidate (yet), don't bother getting the property.

+    nsISVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(aFrame);
+    if (outerSVGFrame) {
+      nsCOMPtr<nsISVGRendererRegion> region;
+      nsSVGUtils::FindFilterInvalidation(aFrame, getter_AddRefs(region));
+      if (region)
+        outerSVGFrame->InvalidateRegion(region, PR_TRUE);
+    }

Move this to a static helper and share it.

+        // return NS_OK; can't return. we need reverse order but only
+        // have a singly linked list...

This means we tranverse the entire SVG frame tree for every GetFrameForPoint. Yow. One thing you could do here is copy the child list into an nsAutoVoidArray, then traverse it from front to back, which is asymptotically no slower and gives you the chance to early-exit. You might also want to look at bounding boxes. But it probably just doesn't matter at the moment since this will only execute on event handling.

Comment 3

[tor]

Attachment: update per comments

Partial redraw of nested group opacity (ex: <g opacity="0.5"><rect opacity="0.5" .../></g>) doesn't display properly.  I need to think more about how to fix that.

Comment 4

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

+already_AddRefed<nsISVGRendererSurface>
+GetComplexClipSurface(nsISVGRendererCanvas *aCanvas, nsIFrame *aFrame)

+already_AddRefed<nsISVGRendererSurface>
+GetMaskSurface(nsISVGRendererCanvas *aCanvas, nsIFrame *aFrame, float opacity)

These should be static.

Are you going to revise this patch to fix that bug you mentioned? I presume it's a subsurface issue. Is the right thing to do here to use vlad's push/pop_group API?

Comment 5

[tor]

Attachment: add statics

Nested group opacity is broken in the trunk right now (bug 331130), so this wouldn't be a regression.  I was thinking it could be handled after this went in.

Yes, this is a subsurface problem.  Using the push/pop group may help with some aspects of the problem, but I was holding off on using that API until it was merged upstream.

Comment 6

[tor]

Attachment: merge to tip - version checked in

Comment 7

[tor]

Checked in.

Comment 8

[tor]

Comment on attachment 215328 [details] [diff] [review]
update per comments

Clear old review request.

Comment 9

[tor]

This broke nsSVGForeignObjectFrame::PaintSVG due to the API change.  Not sure of the clean way to fix it, so #if'd out the problem code for now (should still work, just do more work than necessary).