As it seems to be a somewhat common authoring mistake to use "opacity" when "fill-opacity" or "stroke-opacity" was intended, we should try to avoid doing extra work when the same results are generated.

Comment on attachment 247099 [details] [diff] [review] update to tip >+PRBool >+nsSVGUtils::CanOptimizeOpacity(nsIFrame *aFrame) >+{ >+ if (!(aFrame->GetStateBits() & NS_STATE_SVG_FILTERED)) { >+ if (aFrame->GetType() == nsGkAtoms::svgImageFrame) { >+ return PR_TRUE; Since fill and stroke are meaningless on an image, can't the image case just be hardcoded and omitted from this function? >+ } else if (aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame) { The 'else' would be redundant given the return. >+ nsSVGGeometryFrame *geom = NS_STATIC_CAST(nsSVGGeometryFrame*, aFrame); >+ if (!(geom->HasFill() && geom->HasStroke())) That returns true if there's only a stroke, but not if there's only a fill. Think you want something like: PRBool hasFill = geom->HasFill(); PRBool hasStroke = geom->HasStroke(); if (hasFill && !hasStroke || !hasFill && hasStroke) >+ return PR_TRUE; >+ } >+ } >+ return PR_FALSE; >+}

> That returns true if... Uh, never mind that. Not reading it right.

(In reply to comment #3) > (From update of attachment 247099 [details] [diff] [review] [edit]) > >+PRBool > >+nsSVGUtils::CanOptimizeOpacity(nsIFrame *aFrame) > >+{ > >+ if (!(aFrame->GetStateBits() & NS_STATE_SVG_FILTERED)) { > >+ if (aFrame->GetType() == nsGkAtoms::svgImageFrame) { > >+ return PR_TRUE; > > Since fill and stroke are meaningless on an image, can't the image case just be > hardcoded and omitted from this function? The filter check would still be needed in nsSVGImageFrame, so I thought it might be best to just keep all the logic in one method.

Comment on attachment 247442 [details] [diff] [review] Remove else >+++ layout/svg/base/src/nsSVGGeometryFrame.h 4 Dec 2006 17:50:08 -0000 >@@ -110,11 +110,12 @@ protected: > nsSVGPaintServerFrame *GetPaintServer(const nsStyleSVGPaint *aPaint); > > NS_IMETHOD InitSVG(); > > private: > nsresult GetStrokeDashArray(double **arr, PRUint32 *count); > float GetStrokeDashoffset(); > void RemovePaintServerProperties(); >+ float OptimizeOpacity(float aOpacity); Can you call this "MaybeOptimizeOpacity" and add something like the following comment above it? // If we can avoid the expense of group opacity, we multiply the fill-opacity // or stroke-opacity (whichever one is present) by the value of the 'opacity' // property, and elsewhere pretend the 'opacity' property has a value of 1. >@@ -292,18 +292,21 @@ nsSVGImageFrame::PaintSVG(nsSVGRenderSta > > nsCOMPtr<nsIDOMSVGMatrix> fini = GetImageTransform(); > > if (GetStyleDisplay()->IsScrollableOverflow()) { > gfx->Save(); > nsSVGUtils::SetClipRect(gfx, ctm, x, y, width, height); > } > >- nsSVGUtils::CompositeSurfaceMatrix(gfx, mSurface, fini, >- mStyleContext->GetStyleDisplay()->mOpacity); >+ float opacity = 1.0f; >+ if (nsSVGUtils::CanOptimizeOpacity(this)) >+ opacity = GetStyleDisplay()->mOpacity; I have a bit of a post-Christmas party hangover right now, but I believe this logic should be inverted. >+ /* Using opacity instead of fill or stroke opacity on a geometry >+ * object seems to be a common authoring mistake. If we're not >+ * applying filters and not both stroking and filling, we can >+ * generate the same result without going through a push/pop >+ * group. */ >+ static PRBool >+ CanOptimizeOpacity(nsIFrame *aFrame); Can you make that "Using group opacity..." and "without going through the overhead of...". With those changes r=jwatt.

(In reply to comment #7) > >+ float OptimizeOpacity(float aOpacity); > > Can you call this "MaybeOptimizeOpacity" and add something like the following > comment above it? > > // If we can avoid the expense of group opacity, we multiply the fill-opacity > // or stroke-opacity (whichever one is present) by the value of the 'opacity' > // property, and elsewhere pretend the 'opacity' property has a value of 1. Renamed and added a modification of the suggested comment. > >@@ -292,18 +292,21 @@ nsSVGImageFrame::PaintSVG(nsSVGRenderSta > >- nsSVGUtils::CompositeSurfaceMatrix(gfx, mSurface, fini, > >- mStyleContext->GetStyleDisplay()->mOpacity); > >+ float opacity = 1.0f; > >+ if (nsSVGUtils::CanOptimizeOpacity(this)) > >+ opacity = GetStyleDisplay()->mOpacity; > > I have a bit of a post-Christmas party hangover right now, but I believe this > logic should be inverted. I think the logic is correct, and have added a comment to try clarifying the issue.

Comment on attachment 250218 [details] [diff] [review] updated patch Ah, I see. r=jwatt

Comment on attachment 250218 [details] [diff] [review] updated patch + if (nsSVGUtils::CanOptimizeOpacity(this)) + aOpacity *= GetStyleDisplay()->mOpacity; Braces please + if (nsSVGUtils::CanOptimizeOpacity(this)) + opacity = GetStyleDisplay()->mOpacity; Ditto. + if (aFrame->GetType() == nsGkAtoms::svgImageFrame) + return PR_TRUE; + if (aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame) { Pull up the call to aFrame->GetType() to a variable so we only call it once.

Checked in.