572697 - SVG paint server changes for -moz-element

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Markus Stange [:mstange]]

Attachment: part 1: Add eSVGGeometry and eSVGPaintServer frame types

I'll need eSVGGeometry in part 2 and eSVGPaintServer in DrawPaintServer which I'll add directly in bug 506826.

Comment 1

[Markus Stange [:mstange]]

Attachment: part 2

In bug 506826 I need arbitrary HTML frames to be able to act as the target of at SVG paint server fill.

This patch does several things:
 - Renames SetupPaintServer to GetPaintServerPattern. Getting a gfxPattern that
   can be passed around matches better with my use case.
 - Reimplement SetupPaintServer in nsSVGPaintServerFrame.
 - Add aOverrideBounds parameter to GetPaintServerPattern. Non-SVG target
   frames can have different ideas about the fill target area, so don't just
   use the bbox for them. This will be set to something non-null in
   nsSVGIntegrationUtils::DrawPaintServer which I'll add in bug 506826.
 - Don't assume the target frame is an SVG geometry frame. There's one place
   where it's important to check whether the target frame is an SVG geometry
   frame:
+  if (aSource->IsFrameOfType(nsIFrame::eSVGGeometry)) {
+    // Set the geometrical parent of the pattern we are rendering
+    patternFrame->mSource = static_cast<nsSVGGeometryFrame*>(aSource);
+  }
   ... and that's why I added eSVGGeometry in the previous part.
   All the other uses of the target frame can get by with a generic nsIFrame
   and things like nsSVGUtils::GetCanvasTM(aTarget) instead of
   aTarget->GetCanvasTM(). At least I think they can, please double-check.

Tests pass with this patch.

Comment 2

[Robert Longson [:longsonr]]

I don't think this will work with html text as the target. You will suffer the same issue as with SVG text won't you? That's what the if (callerType ==  nsGkAtoms::svgGlyphFrame) is for.

I think that (callerType == nsGkAtoms::svgGlyphFrame) is fragile and should be replaced with frame->GetContent()->IsNodeOfType(nsINode::eTEXT).

I may have more comments later but I have to go out now.

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 451959 [details] [diff] [review]
part 2

>--- a/layout/svg/base/src/nsSVGPaintServerFrame.cpp
>+++ b/layout/svg/base/src/nsSVGPaintServerFrame.cpp
>@@ -30,10 +30,25 @@
> 
> #include "nsSVGPaintServerFrame.h"
>+#include "nsSVGGeometryFrame.h"
>+#include "gfxDrawable.h"

Why do you need the gfxDrawable include?

>diff --git a/layout/svg/base/src/nsSVGPatternFrame.cpp b/layout/svg/base/src/nsSVGPatternFrame.cpp
>--- a/layout/svg/base/src/nsSVGPatternFrame.cpp
>+++ b/layout/svg/base/src/nsSVGPatternFrame.cpp

> nsresult
> nsSVGPatternFrame::PaintPattern(gfxASurface** surface,
>                                 gfxMatrix* patternMatrix,
>-                                nsSVGGeometryFrame *aSource,
>-                                float aGraphicOpacity)
>+                                nsIFrame *aSource,
>+                                float aGraphicOpacity,
>+                                gfxRect *aOverrideBounds)

const gfxRect *

>   // OK, now render -- note that we use "firstKid", which
>   // we got at the beginning because it takes care of the
>   // referenced pattern situation for us
> 
>-  // Set the geometrical parent of the pattern we are rendering
>-  patternFrame->mSource = aSource;
>+  if (aSource->IsFrameOfType(nsIFrame::eSVGGeometry)) {

or you could make geometry frames work with do_queryFrame but that's really roc's call. I suspect your way is faster so I'm fine with it.

> 
>   if (viewBox.height > 0.0f && viewBox.width > 0.0f) {
>-    nsSVGSVGElement *ctx = aTargetContent->GetCtx();
>-    float viewportWidth = GetWidth()->GetAnimValue(ctx);
>-    float viewportHeight = GetHeight()->GetAnimValue(ctx);
>+    float viewportWidth = GetWidth()->GetAnimValue(aTarget);
>+    float viewportHeight = GetHeight()->GetAnimValue(aTarget);
... 
>-    float refX = GetX()->GetAnimValue(ctx);
>-    float refY = GetY()->GetAnimValue(ctx);
>+    float refX = GetX()->GetAnimValue(aTarget);
>+    float refY = GetY()->GetAnimValue(aTarget);

You're going to end up getting the SVG ctx (which is the nearest parent <svg> element) 4 times here as each GetAnimValue with an aTarget will look it up. Please stick with the ctx version here. Maybe add a comment in case someone else tries this.

>-
>-  // Make sure the callerContent is an SVG element.  If we are attempting
>-  // to paint a pattern for text, then the content will be the #text, so we
>-  // actually want the parent, which should be the <svg:text> or <svg:tspan>
>-  // element.
>+  // If we are attempting to paint a pattern for text, then the content will be
>+  // the #text, so we actually want the parent, which should be the <svg:text>
>+  // or <svg:tspan> element.

Or html:text now?

>+  if (aOverrideBounds) {
>+    *aBBox = *aOverrideBounds;
>+  }

Are you for or against { and } round single line if statments since you remove them elsewhere?

> 
>-  aContext->SetMatrix(matrix);
>-  if (NS_FAILED(rv)) {
>-    return PR_FALSE;
>-  }
>+  if (NS_FAILED(rv))
>+    return nsnull;
> 

Best just to leave the { and } both here and elsewhere you remove them.

Please check that in
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-index.html
tests 62-78, and 81 still work.

The replacement 79 and 80 (since the originals are broken) are:
http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/pservers-grad-18-b.html
and 
http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/pservers-grad-19-b.html

You need some html tests, I assume those will come in the other bug though.

r=longsonr with comments (including comment 2) addressed.

Comment 4

[Markus Stange [:mstange]]

(In reply to comment #2)
> I don't think this will work with html text as the target. You will suffer the
> same issue as with SVG text won't you?

What is this issue? I tried to trace the path that callerContent takes, and it only seems to be used for unit conversions somewhere in nsSVGLength2.cpp, specifically for GetMMPerPixel, GetAxisLength, GetEmLength and GetExLength. But looking at those functions I don't understand what the issue would be.

Also, these functions will only be called on nsIFrames that have CSS background-image:-moz-element(...) set on them, and I'm not sure how you could do that for html text frames. To be honest, I don't know how to get hold of html text frames in the first place.

> I think that (callerType == nsGkAtoms::svgGlyphFrame) is fragile and should be
> replaced with frame->GetContent()->IsNodeOfType(nsINode::eTEXT).

I can do that, sure.

(In reply to comment #3)
> (From update of attachment 451959 [details] [diff] [review])
> >--- a/layout/svg/base/src/nsSVGPaintServerFrame.cpp
> >+++ b/layout/svg/base/src/nsSVGPaintServerFrame.cpp
> >@@ -30,10 +30,25 @@
> > 
> > #include "nsSVGPaintServerFrame.h"
> >+#include "nsSVGGeometryFrame.h"
> >+#include "gfxDrawable.h"
> 
> Why do you need the gfxDrawable include?

I don't need it, I'll remove it.

> >   if (viewBox.height > 0.0f && viewBox.width > 0.0f) {
> >-    nsSVGSVGElement *ctx = aTargetContent->GetCtx();
> >-    float viewportWidth = GetWidth()->GetAnimValue(ctx);
> >-    float viewportHeight = GetHeight()->GetAnimValue(ctx);
> >+    float viewportWidth = GetWidth()->GetAnimValue(aTarget);
> >+    float viewportHeight = GetHeight()->GetAnimValue(aTarget);
> ... 
> >-    float refX = GetX()->GetAnimValue(ctx);
> >-    float refY = GetY()->GetAnimValue(ctx);
> >+    float refX = GetX()->GetAnimValue(aTarget);
> >+    float refY = GetY()->GetAnimValue(aTarget);
> 
> You're going to end up getting the SVG ctx (which is the nearest parent <svg>
> element) 4 times here as each GetAnimValue with an aTarget will look it up.

For SVG frames, yes. For HTML frames there's no SVG ctx and the alternative nsIFrame* length conversion functions in nsSVGLength2.cpp will be called (see above).

> Please stick with the ctx version here.

I don't think I can.

> >+  if (aOverrideBounds) {
> >+    *aBBox = *aOverrideBounds;
> >+  }
> 
> Are you for or against { and } round single line if statments since you remove
> them elsewhere?

Against them for early returns, for them for everything else - that's what roc taught me.

> 
> > 
> >-  aContext->SetMatrix(matrix);
> >-  if (NS_FAILED(rv)) {
> >-    return PR_FALSE;
> >-  }
> >+  if (NS_FAILED(rv))
> >+    return nsnull;
> > 
> 
> Best just to leave the { and } both here and elsewhere you remove them.

OK sure.

> Please check that in
> http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-index.html
> tests 62-78, and 81 still work.

I'll do that.

> You need some html tests, I assume those will come in the other bug though.

Right.

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #4)
> For SVG frames, yes. For HTML frames there's no SVG ctx and the alternative
> nsIFrame* length conversion functions in nsSVGLength2.cpp will be called (see
> above).
> 
> > Please stick with the ctx version here.
> 
> I don't think I can.

You could test for a frame type of SVG and call each version as appropriate.

Comment 6

[Markus Stange [:mstange]]

Attachment: part 2, v2

(In reply to comment #5)
> You could test for a frame type of SVG and call each version as appropriate.

OK, I'm doing that in this patch.

(In reply to comment #3)
> Please check that in
> http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-index.html
> tests 62-78, and 81 still work.
> 
> The replacement 79 and 80 (since the originals are broken) are:
> http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/pservers-grad-18-b.html
> and 
> http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/pservers-grad-19-b.html

Looks good. pservers-grad-06-b and the replacement version of pservers-grad-19-b fail, but they fail for me in a current trunk build, too.

Comment 7

[Robert Longson [:longsonr]]

I take it you're on a Mac. pservers-grad-06-b works for me.

And you're right pservers-grad-19-b, it should fail for you the animateColor patch hasn't landed yet.

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #7)
> I take it you're on a Mac.

Right. I've filed bug 573769.

Comment 9

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/aef10f984368
http://hg.mozilla.org/mozilla-central/rev/fad8c83b11dd