316764 - Implement <svg:mask>

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: <mask> implementation

Comment 2

[Robert Longson [:longsonr]]

Should nsSVGFeaturesList.h be modified to have http://www.w3.org/TR/SVG11/feature#Mask as a supported feature as part of this patch?

Comment 3

[tor]

Attachment: modify feature list, too

Comment 4

[tor]

Attachment: try the right attachment...

Comment 5

[Scooter Morris]

FYI: The tree has drifted underneath this patch (nsSVGUtils.h and nsSVGUtils.cpp to name two examples).  Pretty simple to workaround, but I thought I'd let you know.

Comment 6

[tor]

Attachment: update to tip

Comment 7

[tor]

Attachment: also fix clipPaths for multiple overlapping children

Comment 8

[Scooter Morris]

Comment on attachment 208766 [details] [diff] [review]
also fix clipPaths for multiple overlapping children

>Index: layout/svg/base/src/nsSVGClipPathFrame.cpp

> NS_IMETHODIMP
> nsSVGClipPathFrame::ClipPaint(nsISVGRendererCanvas* canvas,
>+                              nsISVGRendererSurface* aClipSurface,
>                               nsISVGChildFrame* aParent,
>                               nsCOMPtr<nsIDOMSVGMatrix> aMatrix)
> {
>   nsRect dirty;
>   nsresult rv;
> 
>   mClipParent = aParent,
>   mClipParentMatrix = aMatrix;
> 
>   NotifyCanvasTMChanged(PR_TRUE);
> 
>-  rv = canvas->SetRenderMode(nsISVGRendererCanvas::SVG_RENDER_MODE_CLIP);

Could you provide a comment here about what IsTrivial means?  Something like:
// See if we have more than one SVG child.  If not do a simple clip.

>+  PRBool isTrivial;
>+  IsTrivial(&isTrivial);
>+
>+  if (isTrivial)
>+    rv = canvas->SetRenderMode(nsISVGRendererCanvas::SVG_RENDER_MODE_CLIP);
>+  else {
>+    rv = canvas->SetRenderMode(nsISVGRendererCanvas::SVG_RENDER_MODE_CLIP_MASK);
>+
>+    canvas->PushSurface(aClipSurface);
>+  }
>+
>   
> 

>Index: layout/svg/base/src/nsSVGGFrame.cpp
>===================================================================

>@@ -112,66 +110,106 @@ nsSVGGFrame::PaintSVG(nsISVGRendererCanv
>     if (mFilter) {
>       if (!mFilterRegion)
>         mFilter->GetInvalidationRegion(this, getter_AddRefs(mFilterRegion));
>       mFilter->FilterPaint(canvas, this);
>       return NS_OK;
>     }
>   }
> 
>+  nsISVGOuterSVGFrame* outerSVGFrame = GetOuterSVGFrame();
>+
>+  /* check for a clip path */
>+
>+  PRBool trivialClip = PR_TRUE;
>   nsSVGClipPathFrame *clip = NULL;
>+  nsCOMPtr<nsISVGRendererSurface> clipMaskSurface;
>+
>   aURI = GetStyleSVGReset()->mClipPath;
>   if (aURI) {
>     NS_GetSVGClipPathFrame(&clip, aURI, mContent);
> 
>     if (clip) {
>-      nsCOMPtr<nsIDOMSVGMatrix> matrix = GetCanvasTM();
>-      canvas->PushClip();
>-      clip->ClipPaint(canvas, this, matrix);

Again, a comment would be helpful...

>+      clip->IsTrivial(&trivialClip);
>+
>+      if (trivialClip) {
>+        canvas->PushClip();
>+      } else {
>+        nsSVGUtils::GetSurface(outerSVGFrame, getter_AddRefs(clipMaskSurface));
>+        if (!clipMaskSurface)
>+          clip = nsnull;
>+      }
>+
>+      if (clip) {
>+        nsCOMPtr<nsIDOMSVGMatrix> matrix = GetCanvasTM();
>+        clip->ClipPaint(canvas, clipMaskSurface, this, matrix);
>+      }
>     }
>   }


>+  /* check for mask */
>+
>+  nsSVGMaskFrame *mask = nsnull;
>+  nsCOMPtr<nsISVGRendererSurface> maskSurface, maskedSurface;
>+
>+  aURI = GetStyleSVGReset()->mMask;
>+  if (aURI) {
>+    NS_GetSVGMaskFrame(&mask, aURI, mContent);
>+
>+    if (mask) {
>+      nsSVGUtils::GetSurface(outerSVGFrame, getter_AddRefs(maskSurface));
>+
>+      if (maskSurface) {
>+        nsCOMPtr<nsIDOMSVGMatrix> matrix = GetCanvasTM();
>+        if (NS_FAILED(mask->MaskPaint(canvas, maskSurface, this, matrix,
>+                                      display->mOpacity)))
>+          maskSurface = nsnull;
>       }
>     }
>   }
> 
>+  if (maskSurface || clipMaskSurface || display->mOpacity != 1.0) {
>+    nsSVGUtils::GetSurface(outerSVGFrame, getter_AddRefs(maskedSurface));
>+    if (maskedSurface) {
>+      canvas->PushSurface(maskedSurface);
>+    } else
>+      maskSurface = nsnull;



Why is this needed?  A comment is probably in order.

>+NS_IMETHODIMP
>+nsSVGMaskFrame::QueryInterface(REFNSIID aIID, void** aInstancePtr)
>+{
>+  if (nsnull == aInstancePtr) {
>+    return NS_ERROR_NULL_POINTER;
>+  }
>+  if (aIID.Equals(nsSVGMaskFrame::GetCID())) {
>+    *aInstancePtr = (void*)(nsSVGMaskFrame*)this;

Addref'ing a frame is probably not a good idea...
>+    NS_ADDREF_THIS();
>+    return NS_OK;
>+  }
>+  return (nsSVGDefsFrame::QueryInterface(aIID, aInstancePtr));
>+}


>+NS_IMETHODIMP
>+nsSVGMaskFrame::InitSVG()
>+{
>+  nsresult rv = nsSVGDefsFrame::InitSVG();
>+  if (NS_FAILED(rv))
>+    return rv;
>+

Shouldn't this be mMaskParentMatrix = nsnull; ?
>+  mMaskParentMatrix = NULL;
>+

Comment 9

[tor]

Attachment: adjust per review comments

Comment 10

[tor]

Attachment: right version...

Comment 11

[Scooter Morris]

Comment on attachment 209009 [details] [diff] [review]
right version...

Looks fine to me

Comment 12

[tor]

Checked in.

Comment 13

[Boris Zbarsky [:bzbarsky]]

This broke the AIX build:

/home/tbox/sb/tinderbox/AIX_5.1_Clobber/mozilla/layout/svg/base/src/nsSVGClipPathFrame.cpp
"/usr/include/math.h", line 236.9: 1540-0848 (S) The macro name "M_PI" is already defined with a different definition.
"/home/tbox/sb/tinderbox/AIX_5.1_Clobber/mozilla/layout/svg/base/src/nsSVGUtils.h", line 60.9: 1540-0425 (I) "M_PI" is defined on line 60 of "/home/tbox/sb/tinderbox/AIX_5.1_Clobber/mozilla/layout/svg/base/src/nsSVGClipPathFrame.cpp".