295850 - Gradient implementation needs to be changed to support paint server

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Scooter Morris]

The current gradient implementation makes it difficult to support other SVG
paint servers (e.g. patterns).  The basic approach will be to split the
GetSVGGradient out into a more generic nsSVGUtils::GetPaintServerFrame.  Calling
routines can then use GetType to determine which frame type they have and cast
the frame to the right type.

Comment 1

[Scooter Morris]

Attachment: Set of patches to support eventual pattern addition

Here is a patch that makes some changes to the gradient implementation to
generalize things to support "paint servers" -- both gradients and patterns.

Comment 2

[tor]

Comment on attachment 184949 [details] [diff] [review]
Set of patches to support eventual pattern addition

r- for reasons discussed on irc.

Comment 3

[Scooter Morris]

Attachment: Updated patch to catch some error conditions and remove polluting patches in my tree

Comment 4

[tor]

Comment on attachment 185028 [details] [diff] [review]
Updated patch to catch some error conditions and remove polluting patches in my tree

> nsresult NS_GetSVGGradient(nsISVGGradient **aGrad, nsIURI *aURI, nsIContent *aContent, 
>                            nsIPresShell *aPresShell)
> {
>+  nsresult rv = nsSVGUtils::GetReferencedFrame((nsIFrame **)(&result), uriSpec, aContent, aPresShell);
>+  if (!NS_SUCCEEDED(rv))
>     return NS_ERROR_FAILURE;
>+  nsIAtom* frameType = result->GetType();
>+  if ((frameType != nsLayoutAtoms::svgLinearGradientFrame) && (frameType != nsLayoutAtoms::svgRadialGradientFrame))
>     return NS_ERROR_FAILURE;
>+  *aGrad = (nsISVGGradient*)result;
>+  
>   return rv;
> }

Since we're already returning if the return from GetReferencedFrame is not
NS_OK, storing/returning rv isn't needed.

>+#if 0
>+/* [noscript] void GetFillPattern(nsISVGPattern **aPat); */
>+NS_IMETHODIMP
>+nsSVGPathGeometryFrame::GetFillPattern(nsISVGPattern **aPat)
>+{
>+  nsresult rv = NS_OK;
>+  if (!mFillPattern) {
>+    nsIURI *aServer;
>+    aServer = GetStyleSVG()->mFill.mPaint.mPaintServer;
>+    if (aServer == nsnull)
>+      return NS_ERROR_FAILURE;
>+    // Now have the URI.  Get the pattern 
>+    rv = NS_GetSVGPattern(getter_AddRefs(mFillPattern), aServer, mContent, 
>+                           nsSVGPathGeometryFrameBase::GetPresContext()->PresShell());
>+    NS_ADD_SVGVALUE_OBSERVER(mFillPattern);
>+  }
>+  *aPat = mFillPattern;
>+  return rv;
>+}
>+#endif

Nit: not part of this change.

>Index: layout/svg/base/src/nsSVGUseFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGUseFrame.cpp,v
>retrieving revision 1.5
>diff -p -u -8 -r1.5 nsSVGUseFrame.cpp
>--- layout/svg/base/src/nsSVGUseFrame.cpp	27 Apr 2005 16:54:12 -0000	1.5
>+++ layout/svg/base/src/nsSVGUseFrame.cpp	1 Jun 2005 16:20:31 -0000
>@@ -120,17 +120,17 @@ NS_NewSVGUseFrame(nsIPresShell* aPresShe
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsSVGUseFrame::InitSVG()
> {
>   nsresult rv = nsSVGUseFrameBase::InitSVG();
>   if (NS_FAILED(rv)) return rv;
>- 
>+
>   nsCOMPtr<nsIDOMSVGUseElement> UseElement = do_QueryInterface(mContent);

Nit: not part of this change.

>+nsresult nsSVGUtils::GetReferencedFrame(nsIFrame **aServer, nsCAutoString& uriSpec, nsIContent *aContent, 
>+                                        nsIPresShell *aPresShell)
>+{
>+  nsresult rv = NS_OK;
>+  *aServer = nsnull;
>+
>+  // Get the ID from the spec (no ID = an error)
>+  PRInt32 pos = uriSpec.FindChar('#');
>+  if (pos == -1) {
>+    NS_ASSERTION(pos != -1, "URI Spec not a reference");
>+    return NS_ERROR_FAILURE;
>+  }

Should we also be checking a document isn't specified too, so
xlink:href="foobar.svg#baz" doesn't pick "#baz" from the current document?

>Index: layout/svg/renderer/public/nsISVGGeometrySource.idl
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/renderer/public/nsISVGGeometrySource.idl,v
>retrieving revision 1.8
>diff -p -u -8 -r1.8 nsISVGGeometrySource.idl
>--- layout/svg/renderer/public/nsISVGGeometrySource.idl	3 Feb 2005 22:20:13 -0000	1.8
>+++ layout/svg/renderer/public/nsISVGGeometrySource.idl	1 Jun 2005 16:20:31 -0000

Update the uuid.

Comment 5

[Scooter Morris]

Attachment: Updated to check for off-document references

Comment 6

[Scooter Morris]

Attachment: Updated with comments from irc

Comment 7

[tor]

Comment on attachment 185613 [details] [diff] [review]
Updated with comments from irc

>--- layout/svg/renderer/src/cairo/nsSVGCairoPathBuilder.cpp	25 Jan 2005 03:55:02 -0000	1.5
>+++ layout/svg/renderer/src/cairo/nsSVGCairoPathBuilder.cpp	7 Jun 2005 22:29:07 -0000
>@@ -201,17 +201,17 @@ nsSVGCairoPathBuilder::Arcto(float x2, f
>-  double cydash = -root*ry*x1dash/rx;
>+  double cydash = -(root*ry*x1dash)/rx;

Not part of this patch.  Without this, r=tor.

Comment 8

[Scooter Morris]

Attachment: Patch with unrelated diffs removed

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't know when I might be able to get to this; may well not be till after I
get back.

Comment 10

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

Comment on attachment 185807 [details] [diff] [review]
Patch with unrelated diffs removed

+      } else
+	 cairo_stroke(ctx);

Put in the extra braces, please

Otherwise, looks good. As I discussed with tor, we probably want to move this
(along with most of the rest of SVG) into content/, so we don't need to deal
with "paint server frames" which aren't really meaningful, but this is OK for
now.

Comment 11

[Scooter Morris]

Attachment: Updated to current tree & added brackets as requested by roc

OK, ready for 1.8b3 approval.

Comment 12

[chris hofmann]

Comment on attachment 187414 [details] [diff] [review]
Updated to current tree & added brackets as requested by roc

a=chofmann

Comment 13

[tor]

I'm seeing some regressions (missing strokes) with the latest patch.

Comment 14

[Scooter Morris]

Attachment: Minor change to fix stroke regression

Need to use cairo_fill_preserve rather than cairo_fill or strokes don't work.

Comment 15

[Scooter Morris]

Attachment: Updates to fix crash and to update other frames to use GetReferencedFrame

This patch is an update that catches a potential crasher when we repaint the
screen in the middle of tearing down the document.  I also went ahead and
modified  marker frames and clippath frames to use the same mechanism.

Comment 16

[Scooter Morris]

Attachment: Cleaned up a little based on comments on IRC

Comment 17

[tor]

Comment on attachment 190730 [details] [diff] [review]
Cleaned up a little based on comments on IRC

Crasher found - discussing on irc.

Comment 18

[Scooter Morris]

Attachment: Little over zealous on the clean-up

Comment 19

[tor]

Comment on attachment 190734 [details] [diff] [review]
Little over zealous on the clean-up

>+  if (!NS_SUCCEEDED(nsSVGUtils::GetReferencedFrame((nsIFrame **)(&result), 
>+                                                   uriSpec, aContent, aPresShell))) {

Instead of playing casting games, how about making result a nsIFrame and doing
  return result->QueryInterface(NS_GET_IID(nsISVGGradient), (void **)aGrad);

With that, r=tor.

Comment 20

[Scooter Morris]

Attachment: Changed as requested by tor

Comment 21

[Scooter Morris]

Attachment: Changed as requested by tor

Comment 22

[Scooter Morris]

Attachment: GDI+ Changes

Comment 23

[Scooter Morris]

Attachment: Update to GDI+ changes -- check for patterns

Comment 24

[Scooter Morris]

Attachment: Fixes random-colored fills on pattern elements

This combined patch includes some extra error checking requested by tor and
fixes the random-colored fills on patterns until we actually implement
patterns.

Comment 25

[tor]

Latest patch is missing the nsLayoutAtomList.h change.

Comment 26

[tor]

Comment on attachment 191542 [details] [diff] [review]
Fixes random-colored fills on pattern elements

r=tor with the new atom restored.

Comment 27

[Scooter Morris]

Attachment: Oops -- missing file now included

Comment 28

[Scooter Morris]

Checking in layout/base/nsLayoutAtomList.h;
/cvsroot/mozilla/layout/base/nsLayoutAtomList.h,v  <--  nsLayoutAtomList.h
new revision: 1.103; previous revision: 1.102
done
Checking in layout/svg/base/src/nsSVGClipPathFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGClipPathFrame.cpp,v  <-- 
nsSVGClipPathFrame.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in layout/svg/base/src/nsSVGGlyphFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v  <--  nsSVGGlyphFrame.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in layout/svg/base/src/nsSVGGradientFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp,v  <-- 
nsSVGGradientFrame.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in layout/svg/base/src/nsSVGMarkerFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGMarkerFrame.cpp,v  <-- 
nsSVGMarkerFrame.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in layout/svg/base/src/nsSVGPathGeometryFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp,v  <-- 
nsSVGPathGeometryFrame.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in layout/svg/base/src/nsSVGUtils.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.cpp,v  <--  nsSVGUtils.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in layout/svg/base/src/nsSVGUtils.h;
/cvsroot/mozilla/layout/svg/base/src/nsSVGUtils.h,v  <--  nsSVGUtils.h
new revision: 1.2; previous revision: 1.1
done
Checking in layout/svg/renderer/public/nsISVGGeometrySource.idl;
/cvsroot/mozilla/layout/svg/renderer/public/nsISVGGeometrySource.idl,v  <-- 
nsISVGGeometrySource.idl
new revision: 1.9; previous revision: 1.8
done
Checking in layout/svg/renderer/src/cairo/nsSVGCairoGlyphGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoGlyphGeometry.cpp,v 
<--  nsSVGCairoGlyphGeometry.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.cpp,v  <--
 nsSVGCairoPathGeometry.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp,v
 <--  nsSVGGDIPlusGlyphGeometry.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in layout/svg/renderer/src/gdiplus/nsSVGGDIPlusPathGeometry.cpp;
/cvsroot/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusPathGeometry.cpp,v
 <--  nsSVGGDIPlusPathGeometry.cpp
new revision: 1.11; previous revision: 1.10
done