Closed Bug 450340 Opened 16 years ago Closed 16 years ago

Support SVG mask/clip-path/filter effects in HTML

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(3 files, 5 obsolete files)

Attached patch fix (obsolete) — Splinter Review
Here's the big patch.

A lot of this patch is just generalizing SVG code to handle arbitrary frames, especially the filter instance code.

There's some integration with nsCSSRendering and other places to account for the expansion in drawable area that SVG filters can introduce. To make this work well I had to stop outline rendering from depending on the frame overflow area.

There's display list integration so that we can actually draw the effects on non-SVG frames.

There's a hook in nsFrame::Invalidate so that the right areas are invalidated when something changes inside a filter.

There's a new method GetMatrixPropagation which I had to add to make nsSVGUtils::GetBBox clean.

nsSVGIntegrationUtils contains most of the really new code. This avoids having to add too much SVG-specific code to the core frame classes.
Attachment #333489 - Flags: superreview?(mats.palmgren)
Attachment #333489 - Flags: review?(longsonr)
Comment on attachment 333489 [details] [diff] [review]
fix


General points:

Great stuff :-)

a)
I think nsSVGFilterFrame::RestoreTargetState should migrate to a class where the constructor sets the target state (i.e. the bit in CreateInstance) and the destructor contains tcode currently within RestoreTargetState. I think that would make the code less fragile as you could put in early returns in some of the functions that currently have to run on to call RestoreTargetState. You could use the class in nsSVGUtils::GetBBox too.

b)
You've put an actual implementation of Get/SetMatrixPropagation in nsSVGDisplayContainerFrame. 

I assume that nsSVGDisplayContainerFrame didn't have an implementation because:
i) it isn't designed to be a concrete class
ii) a PRPackedBool there can't pack with anything else so all the classes derived from this will now be 4 bytes bigger.

I'd rather you just return PR_FALSE from nsSVGDisplayContainerFrame::GetMatrixPropagation and let the derived classes override this as they do now.

If you're going to keep the nsSVGDisplayContainerFrame implementation then all the derived classes of nsSVGDisplayContainerFrame; nsSVGTSpanFrame, nsSVGTextFrame etc. should use that and their code and variables store matrix propagation should be removed.


>+float
>+nsSVGLength2::GetAxisLength(nsIFrame *aNonSVGFrame) const
>+{
>+  gfxRect rect = nsSVGIntegrationUtils::GetSVGRectForNonSVGFrame(aNonSVGFrame);
>+  switch (mCtxType) {
>+  case nsSVGUtils::X: return rect.Width();
>+  case nsSVGUtils::Y: return rect.Height();
>+  case nsSVGUtils::XY:
>+    return nsSVGUtils::ComputeNormalizedHypotenuse(rect.Width(), rect.Height());
>+  default:
>+    NS_NOTREACHED("Unknown axis type");
>+    return 0;
>+  }
> }

Are you sure rect.Width() and rect.Height() can never be 0. We divide by GetAxisLength elsewhere in this file so I suspect that we need to deal with that like we do in the original GetAxisLength method. 

Also I think its safer if unknown axis type returns 1.

> 
> float
> nsSVGLength2::GetUnitScaleFactor(nsSVGElement *aSVGElement) const
> {
...
>+  default:
>+    NS_NOTREACHED("Unknown unit type");
>+    return 0;

Why are you not calling GetUnitScaleFactor(aSVGElement->GetCtx()); I don't think this change is correct.

>+  }
>+}
>+
>+float
>+nsSVGLength2::GetUnitScaleFactor(nsIFrame *aFrame) const
>+{
>+  if (aFrame->IsFrameOfType(nsIFrame::eSVG))
>+    return GetUnitScaleFactor(static_cast<nsSVGElement*>(aFrame->GetContent()));

How could SVG frames get here? If XBL content with an SVG generic frame gets here it will crash. I think it would be safest to remove the above lines unless there's something I'm missing here.

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>--- a/layout/base/nsCSSRendering.cpp
>+++ b/layout/base/nsCSSRendering.cpp

Gulp, I'm not in Kansas any more Toto.

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsFrame.cpp
>@@ -1172,21 +1176,29 @@ nsIFrame::BuildDisplayListForStackingCon
> 
>   nsRect absPosClip;
>   const nsStyleDisplay* disp = GetStyleDisplay();
>   PRBool applyAbsPosClipping =
>       ApplyAbsPosClipping(aBuilder, disp, this, &absPosClip);
>   nsRect dirtyRect = aDirtyRect;
>+  nsPoint frameOffset = aBuilder->ToReferenceFrame(this);
>   if (applyAbsPosClipping) {
>-    dirtyRect.IntersectRect(dirtyRect,
>-                            absPosClip - aBuilder->ToReferenceFrame(this));
>-  }

Why this change. You don't seem to use the new variable subsequently.

> 
> class nsDisplaySummary : public nsDisplayItem
> {
> 
>-  PRBool isComposited = disp->mOpacity != 1.0f;
>+  PRBool isComposited = disp->mOpacity != 1.0f ||
>+#ifdef MOZ_SVG
>+    nsSVGIntegrationUtils::UseEffectsForFrame(aChild)
>+#endif
>+    ;

I'd be surprised if this compiled without MOZ_SVG defined. I think the || needs to be inside MOZ_SVG too. 

>diff --git a/layout/svg/base/src/nsSVGFilterFrame.cpp b/layout/svg/base/src/nsSVGFilterFrame.cpp
>--- a/layout/svg/base/src/nsSVGFilterFrame.cpp
>+++ b/layout/svg/base/src/nsSVGFilterFrame.cpp
>@@ -42,16 +42,17 @@
> #include "nsGkAtoms.h"
> #include "nsSVGUtils.h"
> #include "nsSVGFilterElement.h"
> #include "nsSVGFilterInstance.h"
> #include "nsSVGFilters.h"
> #include "gfxASurface.h"
> #include "gfxContext.h"
> #include "gfxImageSurface.h"
>+#include "nsSVGRect.h"

I don't see why the above line is necessary.

>+nsRect
>+nsSVGFilterFrame::GetFilterBBox(nsIFrame *aTarget, const nsRect *aSourceBBox)
>+{
>+  nsRect result;
>+
>+  nsAutoPtr<nsSVGFilterInstance> instance;
>+  nsresult rv = CreateInstance(aTarget, nsnull,
>+    nsnull, nsnull, aSourceBBox, getter_Transfers(instance));
>+  if (NS_SUCCEEDED(rv)) {
>+    if (!instance) {
>+      // The filter draws nothing, so the bbox is empty.
>+      result = nsRect();
>+    } else {
>+      // We've passed in the source's dirty area so the instance knows about it.
>+      // Now we can ask the instance to compute the area of the filter output
>+      // that's dirty.

The comment is wrong.

>diff --git a/layout/svg/base/src/nsSVGFilterFrame.h b/layout/svg/base/src/nsSVGFilterFrame.h
>--- a/layout/svg/base/src/nsSVGFilterFrame.h
>+++ b/layout/svg/base/src/nsSVGFilterFrame.h
>@@ -33,48 +33,86 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef __NS_SVGFILTERFRAME_H__
> #define __NS_SVGFILTERFRAME_H__
> 
> #include "nsRect.h"
>+#include "nsSVGUtils.h"

I don't know why you need this. Won't some forward class/struct declares do?

>diff --git a/layout/svg/base/src/nsSVGFilterInstance.cpp b/layout/svg/base/src/nsSVGFilterInstance.cpp
>--- a/layout/svg/base/src/nsSVGFilterInstance.cpp
>+++ b/layout/svg/base/src/nsSVGFilterInstance.cpp
>@@ -33,30 +33,33 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsSVGFilterInstance.h"
> #include "nsSVGUtils.h"
> #include "nsIDOMSVGUnitTypes.h"
> #include "nsSVGMatrix.h"
>+#include "nsSVGFilterFrame.h"

The frame should require the instance not vice versa. I hope this is redundant and can be removed.

> NS_IMETHODIMP
> nsSVGGFrame::SetOverrideCTM(nsIDOMSVGMatrix *aCTM)
> {
>   mOverrideCTM = aCTM;
>   return NS_OK;
> }
> 
> already_AddRefed<nsIDOMSVGMatrix>
>diff --git a/layout/svg/base/src/nsSVGGFrame.h b/layout/svg/base/src/nsSVGGFrame.h
>--- a/layout/svg/base/src/nsSVGGFrame.h
>+++ b/layout/svg/base/src/nsSVGGFrame.h
>@@ -69,16 +69,17 @@ public:
>   // nsIFrame interface:
>   NS_IMETHOD AttributeChanged(PRInt32         aNameSpaceID,
>                               nsIAtom*        aAttribute,
>                               PRInt32         aModType);
> 
>   // nsISVGChildFrame interface:
>   virtual void NotifySVGChanged(PRUint32 aFlags);
>   NS_IMETHOD SetMatrixPropagation(PRBool aPropagate);
>+  virtual PRBool GetMatrixPropagation();

See b) in general points.

>   NS_IMETHOD SetOverrideCTM(nsIDOMSVGMatrix *aCTM);
>   virtual already_AddRefed<nsIDOMSVGMatrix> GetOverrideCTM();
> 
>   // nsSVGContainerFrame methods:
>   virtual already_AddRefed<nsIDOMSVGMatrix> GetCanvasTM();
> 
>   nsCOMPtr<nsIDOMSVGMatrix> mCanvasTM;
>   nsCOMPtr<nsIDOMSVGMatrix> mOverrideCTM;
>diff --git a/layout/svg/base/src/nsSVGGlyphFrame.cpp b/layout/svg/base/src/nsSVGGlyphFrame.cpp
>--- a/layout/svg/base/src/nsSVGGlyphFrame.cpp
>+++ b/layout/svg/base/src/nsSVGGlyphFrame.cpp
>@@ -1281,16 +1281,29 @@ nsSVGGlyphFrame::EnsureTextRun(float *aD
>       return PR_FALSE;
>   }
> 
>   *aDrawScale = float(size/textRunSize);
>   *aMetricsScale = (*aDrawScale)/GetTextRunUnitsFactor();
>   return PR_TRUE;
> }
> 
>+NS_IMETHODIMP
>+nsSVGGlyphFrame::SetMatrixPropagation(PRBool aPropagate)
>+{
>+  mPropagateTransform = aPropagate;
>+  return NS_OK;
>+}
>+
>+PRBool
>+nsSVGGlyphFrame::GetMatrixPropagation()
>+{
>+  return mPropagateTransform;
>+}
>+
> //----------------------------------------------------------------------
> // helper class
> 
> CharacterIterator::CharacterIterator(nsSVGGlyphFrame *aSource,
>         PRBool aForceGlobalTransform)
>   : mSource(aSource), mCurrentAdvance(0), mCurrentChar(-1),
>     mInError(PR_FALSE)
> {
>diff --git a/layout/svg/base/src/nsSVGGlyphFrame.h b/layout/svg/base/src/nsSVGGlyphFrame.h
>--- a/layout/svg/base/src/nsSVGGlyphFrame.h
>+++ b/layout/svg/base/src/nsSVGGlyphFrame.h
>@@ -119,17 +119,18 @@ public:
>   NS_IMETHOD UpdateCoveredRegion();
>   NS_IMETHOD GetBBox(nsIDOMSVGRect **_retval);
> 
>   NS_IMETHOD_(nsRect) GetCoveredRegion();
>   NS_IMETHOD InitialUpdate();
>   virtual void NotifySVGChanged(PRUint32 aFlags);
>   NS_IMETHOD NotifyRedrawSuspended();
>   NS_IMETHOD NotifyRedrawUnsuspended();
>-  NS_IMETHOD SetMatrixPropagation(PRBool aPropagate) { return NS_OK; }
>+  NS_IMETHOD SetMatrixPropagation(PRBool aPropagate);
>+  virtual PRBool GetMatrixPropagation();

I don't understand why this doesn't just return PR_FALSE always. I doubt this will ever get called will it as it's text elements that have effects, not the text itself.

>+  PRPackedBool mPropagateTransform;

Unnecesssary?

>diff --git a/layout/svg/base/src/nsSVGIntegrationUtils.cpp b/layout/svg/base/src/nsSVGIntegrationUtils.cpp
>new file mode 100644
>--- /dev/null
>+++ b/layout/svg/base/src/nsSVGIntegrationUtils.cpp
>@@ -0,0 +1,410 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is the Mozilla SVG project.
>+ *
>+ * The Initial Developer of the Original Code is IBM Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>+ * the Initial Developer. All Rights Reserved.

This file seems to contain shiny new code rather than stuff cadged from elsewhere in the codebase. If that is so then the text above is incorrect.

>+    if (aTransform) {
>+      // Transform by aTransform first
>+      m = nsSVGUtils::ConvertSVGMatrixToThebes(aTransform);
>+      gfxCtx->Multiply(m);
>+    }
>+    
>+    // We're expected to paint with 1 unit equal to 1 CSS pixel. But
>+    // mInnerList->Paint will expected 1 unit to equal 1 device pixel. So
>+    // adjust.

s/will expected/will expect/ or alternatively /expects/

>diff --git a/layout/svg/base/src/nsSVGIntegrationUtils.h b/layout/svg/base/src/nsSVGIntegrationUtils.h
>new file mode 100644
>--- /dev/null
>+++ b/layout/svg/base/src/nsSVGIntegrationUtils.h
>@@ -0,0 +1,117 @@

>+ * The Initial Developer of the Original Code is IBM Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>+ * the Initial Developer. All Rights Reserved.

Is it?

> float
> nsSVGUtils::UserSpace(nsSVGElement *aSVGElement, nsSVGLength2 *aLength)

Please change to const nsSVGLength2 *aLength while you're here.

> {
>   return aLength->GetAnimValue(aSVGElement);
>+}
>+
>+float
>+nsSVGUtils::UserSpace(nsIFrame *aNonSVGContext, nsSVGLength2 *aLength)
>+{

const nsSVGLength2 *aLength

>+  return aLength->GetAnimValue(aNonSVGContext);
> }


>diff --git a/layout/svg/base/src/nsSVGUtils.h b/layout/svg/base/src/nsSVGUtils.h
>--- a/layout/svg/base/src/nsSVGUtils.h
>+++ b/layout/svg/base/src/nsSVGUtils.h

>+  /**
>+   * Get bounding-box for aFrame.
>+   * @param aDisableMatrixPropagation if true, we disable ancestor
>+   * transformations when computing the bbox. if false, we don't touch the
>+   * ancestor tranformation state (if they're disabled, we don't enable them).
>+   */
>+  static already_AddRefed<nsIDOMSVGRect>
>+  GetBBox(nsIFrame *aFrame);

I see no aDisableMatrixPropagation parameter here. I suspect the comment leftover from an earlier iteration of your code and that it should just be removed.
Attachment #333489 - Flags: review?(longsonr) → review-
(In reply to comment #2)
> I think nsSVGFilterFrame::RestoreTargetState should migrate to a class where
> the constructor sets the target state (i.e. the bit in CreateInstance) and the
> destructor contains tcode currently within RestoreTargetState. I think that
> would make the code less fragile as you could put in early returns in some of
> the functions that currently have to run on to call RestoreTargetState. You
> could use the class in nsSVGUtils::GetBBox too.

I created an AutoFilterInstance class which enapsulates CreateFilterInstance and RestoreTargetState. This simplifies the code a bit.

I noticed that there's really no reason we should be painting the un-filtered element in any situation, so I removed that code, which also simplifies things because we no longer have to distinguish "paint nothing" and "paint the unfiltered content".

> You've put an actual implementation of Get/SetMatrixPropagation in
> nsSVGDisplayContainerFrame. 
> 
> I assume that nsSVGDisplayContainerFrame didn't have an implementation because:
> i) it isn't designed to be a concrete class
> ii) a PRPackedBool there can't pack with anything else so all the classes
> derived from this will now be 4 bytes bigger.
> 
> I'd rather you just return PR_FALSE from
> nsSVGDisplayContainerFrame::GetMatrixPropagation and let the derived classes
> override this as they do now.

I don't want to violate the invariant of SetMatrixPropagation(foo); NS_ASSERTION(GetMatrixPropagation() == foo).

We can use a frame state bit to save the 4 bytes. As a followup we could eliminate the Get/SetMatrixPropagation methods and just use the state bits directly.

> If you're going to keep the nsSVGDisplayContainerFrame implementation then all
> the derived classes of nsSVGDisplayContainerFrame; nsSVGTSpanFrame,
> nsSVGTextFrame etc. should use that and their code and variables store matrix
> propagation should be removed.

Will do.

> Are you sure rect.Width() and rect.Height() can never be 0. We divide by
> GetAxisLength elsewhere in this file so I suspect that we need to deal with
> that like we do in the original GetAxisLength method.

OK

> Also I think its safer if unknown axis type returns 1.

OK

> > float
> > nsSVGLength2::GetUnitScaleFactor(nsSVGElement *aSVGElement) const
> > {
> ...
> >+  default:
> >+    NS_NOTREACHED("Unknown unit type");
> >+    return 0;
> 
> Why are you not calling GetUnitScaleFactor(aSVGElement->GetCtx()); I don't
> think this change is correct.

Those lines aren't in that function; I'm unsure what this comment is about.

> >+float
> >+nsSVGLength2::GetUnitScaleFactor(nsIFrame *aFrame) const
> >+{
> >+  if (aFrame->IsFrameOfType(nsIFrame::eSVG))
> >+    return GetUnitScaleFactor(static_cast<nsSVGElement*>(aFrame->GetContent()));
> 
> How could SVG frames get here? If XBL content with an SVG generic frame gets
> here it will crash. I think it would be safest to remove the above lines unless
> there's something I'm missing here.

For example, nsSVGFilterInstance::GetPrimitiveLength calling nsSVGUtils::UserSpace calling nsSVGLength2::GetAnimValue.

I replaced it with an IsNodeOfType(eSVG) check.

> >diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
> >--- a/layout/generic/nsFrame.cpp
> >+++ b/layout/generic/nsFrame.cpp
> >@@ -1172,21 +1176,29 @@ nsIFrame::BuildDisplayListForStackingCon
> > 
> >   nsRect absPosClip;
> >   const nsStyleDisplay* disp = GetStyleDisplay();
> >   PRBool applyAbsPosClipping =
> >       ApplyAbsPosClipping(aBuilder, disp, this, &absPosClip);
> >   nsRect dirtyRect = aDirtyRect;
> >+  nsPoint frameOffset = aBuilder->ToReferenceFrame(this);
> >   if (applyAbsPosClipping) {
> >-    dirtyRect.IntersectRect(dirtyRect,
> >-                            absPosClip - aBuilder->ToReferenceFrame(this));
> >-  }
> 
> Why this change. You don't seem to use the new variable subsequently.

Yeah. Fixed.

> > class nsDisplaySummary : public nsDisplayItem
> > {
> > 
> >-  PRBool isComposited = disp->mOpacity != 1.0f;
> >+  PRBool isComposited = disp->mOpacity != 1.0f ||
> >+#ifdef MOZ_SVG
> >+    nsSVGIntegrationUtils::UseEffectsForFrame(aChild)
> >+#endif
> >+    ;
> 
> I'd be surprised if this compiled without MOZ_SVG defined. I think the || needs
> to be inside MOZ_SVG too. 

Oops, fixed.

> >diff --git a/layout/svg/base/src/nsSVGFilterFrame.cpp b/layout/svg/base/src/nsSVGFilterFrame.cpp
> >--- a/layout/svg/base/src/nsSVGFilterFrame.cpp
> >+++ b/layout/svg/base/src/nsSVGFilterFrame.cpp
> >@@ -42,16 +42,17 @@
> > #include "nsGkAtoms.h"
> > #include "nsSVGUtils.h"
> > #include "nsSVGFilterElement.h"
> > #include "nsSVGFilterInstance.h"
> > #include "nsSVGFilters.h"
> > #include "gfxASurface.h"
> > #include "gfxContext.h"
> > #include "gfxImageSurface.h"
> >+#include "nsSVGRect.h"
> 
> I don't see why the above line is necessary.

Removed.

> >+      // We've passed in the source's dirty area so the instance knows about it.
> >+      // Now we can ask the instance to compute the area of the filter output
> >+      // that's dirty.
> 
> The comment is wrong.

Fixed

> >diff --git a/layout/svg/base/src/nsSVGFilterFrame.h b/layout/svg/base/src/nsSVGFilterFrame.h
> >--- a/layout/svg/base/src/nsSVGFilterFrame.h
> >+++ b/layout/svg/base/src/nsSVGFilterFrame.h
> >@@ -33,48 +33,86 @@
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > #ifndef __NS_SVGFILTERFRAME_H__
> > #define __NS_SVGFILTERFRAME_H__
> > 
> > #include "nsRect.h"
> >+#include "nsSVGUtils.h"
> 
> I don't know why you need this. Won't some forward class/struct declares do?

Fixed.

> >diff --git a/layout/svg/base/src/nsSVGFilterInstance.cpp b/layout/svg/base/src/nsSVGFilterInstance.cpp
> >--- a/layout/svg/base/src/nsSVGFilterInstance.cpp
> >+++ b/layout/svg/base/src/nsSVGFilterInstance.cpp
> >@@ -33,30 +33,33 @@
> >  * the terms of any one of the MPL, the GPL or the LGPL.
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > #include "nsSVGFilterInstance.h"
> > #include "nsSVGUtils.h"
> > #include "nsIDOMSVGUnitTypes.h"
> > #include "nsSVGMatrix.h"
> >+#include "nsSVGFilterFrame.h"
> 
> The frame should require the instance not vice versa. I hope this is redundant
> and can be removed.

We need the declaration of nsSVGFilterPaintCallback so we can call it. I moved that to its own file.

> >diff --git a/layout/svg/base/src/nsSVGGlyphFrame.h b/layout/svg/base/src/nsSVGGlyphFrame.h
> >--- a/layout/svg/base/src/nsSVGGlyphFrame.h
> >+++ b/layout/svg/base/src/nsSVGGlyphFrame.h
> >@@ -119,17 +119,18 @@ public:
> >   NS_IMETHOD UpdateCoveredRegion();
> >   NS_IMETHOD GetBBox(nsIDOMSVGRect **_retval);
> > 
> >   NS_IMETHOD_(nsRect) GetCoveredRegion();
> >   NS_IMETHOD InitialUpdate();
> >   virtual void NotifySVGChanged(PRUint32 aFlags);
> >   NS_IMETHOD NotifyRedrawSuspended();
> >   NS_IMETHOD NotifyRedrawUnsuspended();
> >-  NS_IMETHOD SetMatrixPropagation(PRBool aPropagate) { return NS_OK; }
> >+  NS_IMETHOD SetMatrixPropagation(PRBool aPropagate);
> >+  virtual PRBool GetMatrixPropagation();
> 
> I don't understand why this doesn't just return PR_FALSE always. I doubt this
> will ever get called will it as it's text elements that have effects, not the
> text itself.

That is true, but I'd rather keep things consistent. If we get rid of these methods and use a frame state bit, it will become a non-issue.

> >diff --git a/layout/svg/base/src/nsSVGIntegrationUtils.cpp b/layout/svg/base/src/nsSVGIntegrationUtils.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/layout/svg/base/src/nsSVGIntegrationUtils.cpp
> >@@ -0,0 +1,410 @@
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* ***** BEGIN LICENSE BLOCK *****
> >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >+ *
> >+ * The contents of this file are subject to the Mozilla Public License Version
> >+ * 1.1 (the "License"); you may not use this file except in compliance with
> >+ * the License. You may obtain a copy of the License at
> >+ * http://www.mozilla.org/MPL/
> >+ *
> >+ * Software distributed under the License is distributed on an "AS IS" basis,
> >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> >+ * for the specific language governing rights and limitations under the
> >+ * License.
> >+ *
> >+ * The Original Code is the Mozilla SVG project.
> >+ *
> >+ * The Initial Developer of the Original Code is IBM Corporation.
> >+ * Portions created by the Initial Developer are Copyright (C) 2005
> >+ * the Initial Developer. All Rights Reserved.
> 
> This file seems to contain shiny new code rather than stuff cadged from
> elsewhere in the codebase. If that is so then the text above is incorrect.

I copied and pasted code and then modified it. In such cases I prefer to err on the side of caution.

> >+    if (aTransform) {
> >+      // Transform by aTransform first
> >+      m = nsSVGUtils::ConvertSVGMatrixToThebes(aTransform);
> >+      gfxCtx->Multiply(m);
> >+    }
> >+    
> >+    // We're expected to paint with 1 unit equal to 1 CSS pixel. But
> >+    // mInnerList->Paint will expected 1 unit to equal 1 device pixel. So
> >+    // adjust.
> 
> s/will expected/will expect/ or alternatively /expects/

Fixed.

> > float
> > nsSVGUtils::UserSpace(nsSVGElement *aSVGElement, nsSVGLength2 *aLength)
> 
> Please change to const nsSVGLength2 *aLength while you're here.

Done

> > {
> >   return aLength->GetAnimValue(aSVGElement);
> >+}
> >+
> >+float
> >+nsSVGUtils::UserSpace(nsIFrame *aNonSVGContext, nsSVGLength2 *aLength)
> >+{
> 
> const nsSVGLength2 *aLength

Done. That required const-ifying a few nsSVGLength2 methods.

> >diff --git a/layout/svg/base/src/nsSVGUtils.h b/layout/svg/base/src/nsSVGUtils.h
> >--- a/layout/svg/base/src/nsSVGUtils.h
> >+++ b/layout/svg/base/src/nsSVGUtils.h
> 
> >+  /**
> >+   * Get bounding-box for aFrame.
> >+   * @param aDisableMatrixPropagation if true, we disable ancestor
> >+   * transformations when computing the bbox. if false, we don't touch the
> >+   * ancestor tranformation state (if they're disabled, we don't enable them).
> >+   */
> >+  static already_AddRefed<nsIDOMSVGRect>
> >+  GetBBox(nsIFrame *aFrame);
> 
> I see no aDisableMatrixPropagation parameter here. I suspect the comment
> leftover from an earlier iteration of your code and that it should just be
> removed.

Exactly :-).
Attached patch fix v2 (obsolete) — Splinter Review
Attachment #333489 - Attachment is obsolete: true
Attachment #333680 - Flags: superreview?(mats.palmgren)
Attachment #333680 - Flags: review?(longsonr)
Attachment #333489 - Flags: superreview?(mats.palmgren)
Comment on attachment 333680 [details] [diff] [review]
fix v2


>+static float
>+FixAxisLength(float aLength) {

Put the { above on a new line please.

Also nsSVGIntegrationUtils.cpp is missing from this patch although I saw enough of it in the previous version to be happy with it.
Attachment #333680 - Flags: review?(longsonr) → review+
Attached patch fix v3 (obsolete) — Splinter Review
Updated with that syntax fix and nsSVGIntegrationUtils in the patch.
Attachment #333680 - Attachment is obsolete: true
Attachment #333862 - Flags: superreview?(mats.palmgren)
Attachment #333862 - Flags: review+
Attachment #333680 - Flags: superreview?(mats.palmgren)
Comment on attachment 333862 [details] [diff] [review]
fix v3

The following reftests fails for me (x86_64 Linux) with this patch:
layout/reftests/svg/filters/feComposite-1.svg
layout/reftests/svg/filters/feComposite-2.svg
layout/reftests/svg/filters/feConvolveMatrix-1.svg
layout/reftests/svg/filters/feConvolveMatrix-2.svg
layout/reftests/svg/filters/feGaussianBlur-1.svg
layout/reftests/svg/filters/feMorphology-1.svg
layout/reftests/svg/filters/feMorphology-2.svg
layout/reftests/svg/clip-surface-clone-01.svg
layout/reftests/svg/objectBoundingBox-and-pattern-01b.svg
layout/reftests/svg/objectBoundingBox-and-pattern-01c.svg
layout/reftests/svg/text-gradient-01.svg
layout/reftests/svg/text-scale-01.svg

I had to resolve a few conflicts to make it compile so I could have
made a mistake, would you mind updating the patch to tip please?
Attached patch fix v4 (obsolete) — Splinter Review
Hmm ... some regressions crept in at some point :-(. When I developed this originally I did run the SVG reftests, not sure what happened.

Anyway, there was some nontrivial merging to the trunk due to the change to make SVG framerects in appunits, and a few plain bugs to fix. I bungled the Get/SetMatrixPropagation in a couple of places; nsSVGDisplayContainerFrame::Init wasn't setting the PROPAGATE state, and nsSVGTextFrame was still checking mPropagateTransform even though we weren't setting it anymore. There was also a problem in SVGPaintCallback where we weren't converting the dirty rect from filter space to user space. But all tests pass now.
Attachment #333862 - Attachment is obsolete: true
Attachment #336957 - Flags: superreview?(mats.palmgren)
Attachment #336957 - Flags: review+
Attachment #333862 - Flags: superreview?(mats.palmgren)
Attached file Outline testcase #1
The suggested nsCSSRendering::PaintOutline() changes the layout of an outline around a box shadow and around child overflow...  the new rendering doesn't look right to me.
Comment on attachment 336957 [details] [diff] [review]
fix v4

The rest looks good but I'm marking the patch sr- until the
outline issue above is resolved.  A few nits:

> layout/svg/base/src/nsSVGFilterInstance.cpp
+nsSVGFilterInstance::ComputeSourceNeededRect(nsIntRect* aDirty)
+{
+  *aDirty = nsIntRect();

I don't think this assignment is necessary. If the method fails
the caller should not attempt to use the rect.
 
+nsSVGFilterInstance::ComputeOutputBBox(nsIntRect* aDirty)
...
+  if (mPrimitives.IsEmpty()) {
+    // Nothing should be rendered.
+    *aDirty = nsRect();

You meant "*aDirty = nsIntRect();" ?


> layout/svg/base/src/nsSVGFilterInstance.h
+  nsSVGFilterInstance(nsIFrame *aTargetFrame,
+                      nsSVGFilterPaintCallback *aPaintCallback,
+                      nsSVGFilterElement* aFilterElement,

"*aFilterElement" for consistency.


> layout/svg/base/src/nsSVGFilterPaintCallback.h
+   * @param aDirtyRect the dirty rect *in user space pixels*

Is *...* the way to emphasize in doc comments? or <b> or <em> perhaps?


> layout/svg/base/src/nsSVGIntegrationUtils.cpp
+// Get the union the frame border-box rects over all continuations,
+// relative to aFirst. This defines "user space" for non-SVG frames.
+static nsRect GetNonSVGUserSpace(nsIFrame* aFirst)

Comparing this to nsLayoutUtils::GetAllInFlowRectsUnion() it seems this
would not handle special siblings and table captions etc?

+GetSVGBBox(nsIFrame* aNonSVGFrame, nsIFrame* aCurrentFrame,
+           const nsRect& aCurrentOverflow, const nsRect& aUserSpaceRect)

Same here.

+class RegularFramePaintCallback : public nsSVGFilterPaintCallback
...
+  virtual void Paint(nsSVGRenderState *aContext, nsIFrame *aTarget,
+                     const nsIntRect* aDirtyRect, nsIDOMSVGMatrix *aTransform)
+  {
+    nsIRenderingContext* ctx = aContext->GetRenderingContext(aTarget);
+    gfxContext* gfxCtx = aContext->GetGfxContext();

Do we need to null-check these?


> layout/svg/base/src/nsSVGIntegrationUtils.h
+#include "gfxPoint.h"

Unnecessary #include - there's no direct use of gfxPoint in this header.

  
> layout/svg/base/src/nsSVGUtils.h
   /* Computes the input length in terms of user space coordinates.
      Input: content - object to be used for determining user space
+     Input: aFrame - object to be used for determining user space
             length - length to be converted
   */
-  static float UserSpace(nsSVGElement *aSVGElement, nsSVGLength2 *aLength);
+  static float UserSpace(nsSVGElement *aSVGElement, const nsSVGLength2 *aLength);

There is no aFrame there so no need for the added comment line.
   
+  GetRelativeRect(PRUint16 aUnits, nsSVGLength2 *aXYWH, nsIDOMSVGRect *aBBox,
+                  nsIFrame *aFrame);
 
"const nsSVGLength2 *aXYWH" is better?
Attachment #336957 - Flags: superreview?(mats.palmgren) → superreview-
(In reply to comment #9)
> The suggested nsCSSRendering::PaintOutline() changes the layout of an outline
> around a box shadow and around child overflow...  the new rendering doesn't
> look right to me.

Yes, the patch changes the outline so that it's drawn around the outside of the border-rects of the elements' frames, instead of drawing it around their overflow areas. This change was intentional but I should have mentioned it. I think it's an improvement; it's definitely an improvement in interoperability, since Opera and Webkit both do it this way.

<div style="width:10px; outline:2px dotted blue;">Hello Kitty</div>

is a clearer testcase.
(In reply to comment #10)
> > layout/svg/base/src/nsSVGFilterInstance.cpp
> +nsSVGFilterInstance::ComputeSourceNeededRect(nsIntRect* aDirty)
> +{
> +  *aDirty = nsIntRect();
> 
> I don't think this assignment is necessary. If the method fails
> the caller should not attempt to use the rect.

OK.

> +nsSVGFilterInstance::ComputeOutputBBox(nsIntRect* aDirty)
> ...
> +  if (mPrimitives.IsEmpty()) {
> +    // Nothing should be rendered.
> +    *aDirty = nsRect();
> 
> You meant "*aDirty = nsIntRect();" ?

Yes, thanks.

> > layout/svg/base/src/nsSVGFilterInstance.h
> +  nsSVGFilterInstance(nsIFrame *aTargetFrame,
> +                      nsSVGFilterPaintCallback *aPaintCallback,
> +                      nsSVGFilterElement* aFilterElement,
> 
> "*aFilterElement" for consistency.

OK

> > layout/svg/base/src/nsSVGFilterPaintCallback.h
> +   * @param aDirtyRect the dirty rect *in user space pixels*
> 
> Is *...* the way to emphasize in doc comments? or <b> or <em> perhaps?

*...* may not produce real emphasis but it's good enough :-)

> > layout/svg/base/src/nsSVGIntegrationUtils.cpp
> +// Get the union the frame border-box rects over all continuations,
> +// relative to aFirst. This defines "user space" for non-SVG frames.
> +static nsRect GetNonSVGUserSpace(nsIFrame* aFirst)
> 
> Comparing this to nsLayoutUtils::GetAllInFlowRectsUnion() it seems this
> would not handle special siblings and table captions etc?

Yes. I should definitely do that. I should probably also check the GetFirstContinuation usage to account for those situations too.

> +GetSVGBBox(nsIFrame* aNonSVGFrame, nsIFrame* aCurrentFrame,
> +           const nsRect& aCurrentOverflow, const nsRect& aUserSpaceRect)
> 
> Same here.

Yep.

> +class RegularFramePaintCallback : public nsSVGFilterPaintCallback
> ...
> +  virtual void Paint(nsSVGRenderState *aContext, nsIFrame *aTarget,
> +                     const nsIntRect* aDirtyRect, nsIDOMSVGMatrix *aTransform)
> +  {
> +    nsIRenderingContext* ctx = aContext->GetRenderingContext(aTarget);
> +    gfxContext* gfxCtx = aContext->GetGfxContext();
> 
> Do we need to null-check these?

Not gfxCtx. Maybe ctx. I'll look into it.

> > layout/svg/base/src/nsSVGIntegrationUtils.h
> +#include "gfxPoint.h"
> 
> Unnecessary #include - there's no direct use of gfxPoint in this header.

OK

> > layout/svg/base/src/nsSVGUtils.h
>    /* Computes the input length in terms of user space coordinates.
>       Input: content - object to be used for determining user space
> +     Input: aFrame - object to be used for determining user space
>              length - length to be converted
>    */
> -  static float UserSpace(nsSVGElement *aSVGElement, nsSVGLength2 *aLength);
> +  static float UserSpace(nsSVGElement *aSVGElement, const nsSVGLength2
> *aLength);
> 
> There is no aFrame there so no need for the added comment line.

Oops!

> +  GetRelativeRect(PRUint16 aUnits, nsSVGLength2 *aXYWH, nsIDOMSVGRect *aBBox,
> +                  nsIFrame *aFrame);
> 
> "const nsSVGLength2 *aXYWH" is better?

Yes
I've fixed up all that stuff.

I'm not sure what to do about the overflow area/outline issue. What we do right now is rather weird; the overflow area associated with the children is calculated, then we inflate it by the outline width, then we union with the box-shadows, then we draw the outline on the inside of that area. Thus the outline is drawn on the inside of the bounding-rect for the box-shadows, ugh!

For now probably the best thing is just to add the box-shadows to the children overflow area, save that as the outline inner-box, then inflate by the outline width and add SVG effects.
Attached patch fix v5 (obsolete) — Splinter Review
Okay...

* Fix above comments
* Include box-shadows in overflow area before we apply the outline
* Fix GetOutlineInnerRect to return the frame's overflow area, not size, if it has no savedOutlineInnerRect (fixes the second part of your testcase)
* Makes everything consider the special-sibling chain. When unioning together the box rectangles, uses GetAllInFlowRectsUnion so we drill down through frames that don't correspond to CSS boxes.
* That required me to implement GetFirstContinuationOrSpecialSibling. Only problem there is thet IBSpecialPrevSibling was not set up on the inline *after* the block that split the inline. So I talked to Boris and we decided to set it for that case. That required changes in nsFrame so that IBSpecialPrevSibling is only used for the anonymous box.

I still think we should change our outline code to draw outlines right outside the border-edge, ignoring child overflow, but that should be done separately and needs more work.
Attachment #336957 - Attachment is obsolete: true
Attachment #337389 - Flags: superreview?(mats.palmgren)
Attachment #337389 - Flags: review+
Comment on attachment 337389 [details] [diff] [review]
fix v5

The v5 patch is missing the new files (except for the
clipPath-html-06 reftest).

If ran reftests with the new files added from patch v4:
clipPath-html-06 reftest fails: I see a 100x5 px green block
but clipPath-html-06-ref has a 100x10 px block.

So I assume you've made changes in the new files since v4.
Attachment #337389 - Flags: superreview?(mats.palmgren) → superreview-
Attached patch fix v6Splinter Review
Sorry about losing those files.

Here's the correct patch. Actually I realized there was a bug in the previous version. It could compute incorrect widths for an IB-split, because if the block that causes the split has a defined width, the correct bbox could be quite narrow, but the anonymous block we create for the split will be width:auto so just unioning the overflow areas of the special-siblings will return too large a box. So we need to drill down through anonymous boxes as GetAllInFlowRects does. So I abstracted that out into nsLayoutUtils::GetAllInFlowBoxes. I've modified clipPath-html-06.xhtml to test this.
Attachment #337794 - Flags: superreview?(mats.palmgren)
Attachment #337794 - Flags: review+
(In reply to comment #16)
> It could compute incorrect widths for an IB-split, because if the
> block that causes the split has a defined width, the correct bbox could be
> quite narrow, but the anonymous block we create for the split will be
> width:auto so just unioning the overflow areas of the special-siblings will
> return too large a box. So we need to drill down through anonymous boxes as
> GetAllInFlowRects does. So I abstracted that out into
> nsLayoutUtils::GetAllInFlowBoxes. I've modified clipPath-html-06.xhtml to test
> this.

I wonder if the code in nsCSSRendering::PaintOutline can share this too.
It probably should, yeah. GetAllInFlowBoxes would break an outer table down to the inner table plus caption, but that shouldn't make a difference for outlines.
(In reply to comment #14)
> I still think we should change our outline code to draw outlines right outside
> the border-edge, ignoring child overflow,

Yeah, I agree that's better for compatibility.  I looked briefly at IE8 beta2
and that seems to be what they do too (although their border boxes are wrong
in some cases).
Attachment #337794 - Flags: superreview?(mats.palmgren) → superreview+
Comment on attachment 337794 [details] [diff] [review]
fix v6

Looks good!  sr=mats

A couple of nits, fix/ignore as you see fit:

> layout/base/nsCSSRendering.cpp
+  nsRect outerRect = innerRect;
+  outerRect.Inflate(width, width);
  if (innerRect.Contains(aDirtyRect)) {
    return;
  }

'innerRect' isn't use beyond the 'if' so if you move the added two lines
below it I think it might help a compiler to see that 'outerRect' can
reuse the space for 'innerRect' and avoid the assignment (and avoid the
outerRect.Inflate() when we do return here).


> layout/svg/base/src/nsSVGIntegrationUtils.h
+#include "gfxPoint.h"

You forgot to remove this #include.

+  static PRBool
+  UseEffectsForFrame(nsIFrame* aFrame);

Maybe UsingEffectsForFrame is a better name for a "getter"?
'const nsIFrame*' would help too.
BTW I did what you suggested in comment #20. Thanks!
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 454945
Tests were in the patch.
Flags: in-testsuite+
Depends on: 455314
Depends on: 455630
Depends on: 457362
Depends on: 459512
Depends on: 467910
Depends on: 467914
verified, still, FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414030735

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre ID:20090414035212
Status: RESOLVED → VERIFIED
In the code "BoxToBorderRect" structure added in this bug's aptch, mCallback and mRelativeTo are initialized in backwards order from how they're declared, triggering this compile warning:

> mozilla/layout/base/nsLayoutUtils.cpp: In constructor ‘BoxToBorderRect::BoxToBorderRect(nsIFrame*, nsLayoutUtils::RectCallback*)’:
> mozilla/layout/base/nsLayoutUtils.cpp:1390: warning: ‘BoxToBorderRect::mCallback’ will be initialized after
> mozilla/layout/base/nsLayoutUtils.cpp:1389: warning:   ‘nsIFrame* BoxToBorderRect::mRelativeTo’
> mozilla/layout/base/nsLayoutUtils.cpp:1392: warning:   when initialized here

The attached followup patch flips their initialization order, fixing this warning.
Attachment #375643 - Flags: review?(roc)
Attachment #375643 - Flags: review?(roc) → review+
Depends on: 512192
Blocks: 547323
Depends on: 555220
Depends on: 649823
No longer depends on: 649823
Depends on: 679933
No longer depends on: 679933
Regressions: 1608139
You need to log in before you can comment on or make changes to this bug.