Last Comment Bug 182533 - (svgbranch) SVG backend rewrite
(svgbranch)
: SVG backend rewrite
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 10 votes (vote)
: ---
Assigned To: Alex Fritze
: Bradley Baetz (:bbaetz)
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 101300 110979 111152 111317 122363 124921 133964 137382 141252 148623 148914 150070 150120 160882 166469 190760 199670 214912
  Show dependency treegraph
 
Reported: 2002-11-28 10:42 PST by Alex Fritze
Modified: 2005-03-03 11:29 PST (History)
60 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove NS_INIT_ISUPPORTS from svg source files (13.79 KB, patch)
2003-02-07 09:00 PST, david avery
no flags Details | Diff | Splinter Review
other cheges from trunk to branch (4.75 KB, patch)
2003-02-07 09:02 PST, david avery
no flags Details | Diff | Splinter Review
Changes to allmakefiles.sh/configure.in/autoconf.mk.in (3.04 KB, patch)
2003-12-15 15:50 PST, Alex Fritze
benjamin: review-
Details | Diff | Splinter Review
Changes to content/ (38.86 KB, patch)
2003-12-15 15:53 PST, Alex Fritze
dbaron: review+
Details | Diff | Splinter Review
Changes to dom/ (14.15 KB, patch)
2003-12-15 15:54 PST, Alex Fritze
jonas: review+
Details | Diff | Splinter Review
Changes to gfx/ (6.94 KB, patch)
2003-12-15 15:56 PST, Alex Fritze
tor: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Changes to htmlparser/ (2.62 KB, patch)
2003-12-15 15:57 PST, Alex Fritze
jonas: review+
jst: superreview+
Details | Diff | Splinter Review
Changes to layout/ (15.43 KB, patch)
2003-12-15 15:59 PST, Alex Fritze
dbaron: review+
Details | Diff | Splinter Review
Changes to dom/ (25.23 KB, patch)
2003-12-17 12:05 PST, Alex Fritze
jonas: review+
jst: superreview+
Details | Diff | Splinter Review
Changes to allmakefiles.sh/configure.in/autoconf.mk.in (3.31 KB, patch)
2003-12-18 15:02 PST, Alex Fritze
benjamin: review+
tor: superreview+
Details | Diff | Splinter Review
Changes to content/ (39.62 KB, patch)
2003-12-22 14:36 PST, Alex Fritze
bzbarsky: superreview+
Details | Diff | Splinter Review
Changes to layout (15.83 KB, patch)
2004-02-05 15:14 PST, Alex Fritze
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Alex Fritze 2002-11-28 10:42:26 PST
I've rewritten the SVG backend code to allow for native and cross-platform 
renderers.
The code is checked in on the minibranch 'SVG_20020806_BRANCH'. It modifies 152 
files, introduces 105 new files and obsoletes 17 files.
Two rendering backends are implemented so far: A Windows-native backend using 
GDI+ and a cross-platform backend based on libart.

The code also introduces some new features, most notably support for svg text, 
printing and presentation attributes. Text is currently only implemented for 
the gdi+ backend, but it is being worked on for the libart cross-platform 
backend by Leon Sha of SUN. 

The code fixes many issues with the current trunk SVG, including bugs #101300, 
#110979, #111152, #111317, #122363, #124921, #133964, #137382, #141252, 
#148623, #148914, #150070, #150120, #160882 and #166469.

I hope that eventually this code will be chedked in on the trunk. Until then, 
this is how to build:

libart build (crossplatform)
----------------------------
Currently we only have makefiles for windows & unix, not for mac.
You'll need a .mozconfig containing
   ac_add_options --enable-svg
   ac_add_options --enable-svg-renderer-libart
   mk_add_options MOZ_INTERNAL_LIBART_LGPL=1
   MOZ_INTERNAL_LIBART_LGPL=1
Then check out the code with
  cvs checkout -r SVG_20020806_BRANCH mozilla/client.mk
  cd mozilla
  make -f client.mk checkout
and build:
  make -f client.mk build

gdiplus build (Windows >=98 only )
----------------------------------
You need to obtain a recent version of the microsoft platform sdk; one that 
contains the gdiplus dll & headers. It can be downloaded from 
www.microsoft.com/msdownload/platformsdk/sdkupdate/.
After installing set up your $MOZCONFIG file to contain
  --enable-svg
  --enable-svg-renderer-gdiplus
  --disable-activex
Then pull the source:
  cvs checkout -r SVG_20020806_BRANCH mozilla/client.mk
  cd mozilla
  make -f client.mk checkout
Now, optionally edit your mozilla/config/myconfig.mk file to contain
  DEFINES += -DSVG_FORCE_PIXEL_ACCESS_SURFACES
This should improve SVG performance substantially while moderately 
impacting 'normal' Mozilla rendering.
Then build:
  make -f client.mk build
Finally, if you're not running on Windows XP you'll need to copy 
the 'gdiplus.dll' redistributable from the SDK into your dist/bin folder.
Comment 1 Robert Kaiser 2002-11-28 14:01:00 PST
hmm, sounds like goodies... :)

Any chance or roadmap to get this checked into trunk soon? (it seems the
upcoming "alpha" would be an ideal point for getting it in...)
Comment 2 Alex Fritze 2002-11-28 15:25:55 PST
Some more comments:

- I've uploaded Windows and Linux builds of this branch to 
http://www.croczilla.com/svg/
- The libart backend currently doesn't support foreignObject tags.
Comment 3 Roland Mainz 2002-11-29 00:11:31 PST
How do you handle devices which do not support an alpha channel ?
Comment 4 Alex Fritze 2002-11-29 00:54:21 PST
roland: Neither libart nor gdi+ need devices to have an alpha channel
Comment 5 Roland Mainz 2002-11-29 01:33:28 PST
Alex Fritze wrote:
> Neither libart nor gdi+ need devices to have an alpha channel

Is the "solution" still to try to render SVG as one large image (which isn't a
good idea for printers which operate at huge resolutions like 6600x6600 pixels
(=DIN-A4/600DPI)) ?
Comment 6 Alex Fritze 2002-11-29 01:57:46 PST
roland: yes, libart works in that way and I think gdi+ does behind the scenes 
as well. AFAIKS it is the only generic way of printing SVG and it works well 
for most situations. 
Comment 7 Roland Mainz 2002-11-29 02:21:30 PST
Alex Fritze wrote:
> AFAIKS it is the only generic way of printing SVG and it works well 
> for most situations.

Mhhh, what about larger paper sizes ? Printing on DIN-A1 and DIN-A0 would result
in a "problem" here for 32bit machines since the resulting image would be larger
than the memory addressable by a 32bit pointer...
Comment 8 Alex Fritze 2002-11-29 02:41:18 PST
roland: Have you got any ideas on how to handle these cases in a generic way? 
Maybe banded printing? Or a specialised postscript rendering backend (for those 
rare cases where the user actually has a postscript printer)?
Comment 9 Alex Fritze 2002-11-29 05:29:14 PST
roland: I suppose we should continue the printing discussion on bug#133964. See 
my comments there.
Comment 10 david avery 2002-11-29 08:05:03 PST
how close to checkin is this - since libart now works this seems to be at parity
with the existing SVG in the trunk - checking in would help prevent bitrot 

we spin off 1.3a on Dec4 getting this merged in the next days would be a big win



Comment 11 Alex Fritze 2002-11-29 08:17:53 PST
It is _nearly_ at parity. <ForeignObject> doesn't work for libart yet (that's 
the price we pay for getting bug#111152 fixed). Also there are no makefiles for 
Mac yet. 
Then there's the 'small' issue of getting the code reviewed. It's all not part 
of the default build, but still there are some changes to makefiles and the 
configure file that should be looked at carefully.
Comment 12 Andrew Hagen 2003-01-08 12:09:50 PST
Nominating for 1.3.
Comment 13 david avery 2003-02-07 09:00:12 PST
Created attachment 113802 [details] [diff] [review]
remove NS_INIT_ISUPPORTS from svg source files
Comment 14 david avery 2003-02-07 09:02:33 PST
Created attachment 113803 [details] [diff] [review]
other cheges from trunk to branch
Comment 15 david avery 2003-02-07 16:28:54 PST
Comment on attachment 113803 [details] [diff] [review]
other cheges from trunk to branch

drop this patch - not needed or duplicate on branch
Comment 16 Samuel Sieb 2003-10-04 17:08:12 PDT
What's the current status of this?  If I checkout from that branch will I get a
really old mozilla or is it merged with the current trunk?
Comment 17 david avery 2003-10-04 23:37:35 PDT
the branch is a "partial" branch and is normally resynced at least once a week.

if you have problems the client.mk on the branch has the "merge_svg" target that
merges you local tree up to the tip of the main tree, this normally is all that
is needed, but occasionally changes are made on the main tree that require some
hacking on the branch. alex usually catches that anc checks in as needed


Comment 18 Alex Fritze 2003-12-15 15:50:16 PST
Created attachment 137489 [details] [diff] [review]
Changes to allmakefiles.sh/configure.in/autoconf.mk.in
Comment 19 Alex Fritze 2003-12-15 15:53:00 PST
Created attachment 137490 [details] [diff] [review]
Changes to content/
Comment 20 Alex Fritze 2003-12-15 15:54:30 PST
Created attachment 137493 [details] [diff] [review]
Changes to dom/
Comment 21 Alex Fritze 2003-12-15 15:56:13 PST
Created attachment 137494 [details] [diff] [review]
Changes to gfx/
Comment 22 Alex Fritze 2003-12-15 15:57:47 PST
Created attachment 137495 [details] [diff] [review]
Changes to htmlparser/
Comment 23 Alex Fritze 2003-12-15 15:59:17 PST
Created attachment 137497 [details] [diff] [review]
Changes to layout/
Comment 24 Alex Fritze 2003-12-15 16:07:59 PST
I'd like to get the branch landed on the trunk soon, so I've attached a few
patches. The SVG code is all #ifdef MOZ_SVG'ed, so it should be sufficient to
get only get shared files reviewed at this stage. The six patches above cover
all modifications made on the SVG branch in the respective areas. All other
changes are confined to content/svg, layout/svg and other-licenses/libart-lgpl.
Now I just need to find some volunteers to review the patches and we should be
ready to go...
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-12-15 16:21:10 PST
Comment on attachment 137493 [details] [diff] [review]
Changes to dom/

>Index: dom/public/nsIDOMClassInfo.h
>@@ -238,7 +238,14 @@
>   eDOMClassInfo_SVGPathSegCurvetoQuadraticSmoothRel_id,
>   eDOMClassInfo_SVGRect_id,
>   eDOMClassInfo_SVGAnimatedRect_id,
>-#endif
>+  eDOMClassInfo_SVGAnimatedLengthList_id,
>+  eDOMClassInfo_SVGLengthList_id,
>+  eDOMClassInfo_SVGNumber_id,
>+  eDOMClassInfo_SVGTextElement_id,
>+  eDOMClassInfo_SVGTSpanElement_id,
>+  eDOMClassInfo_SVGAnimatedString_id,
>+  eDOMClassInfo_SVGImageElement_id,
>+#endif //MOZ_SVG

Unfortunatly i think you need to add these to the end of the list to avoid
breaking binary compatibility. Though I've never understood how we can have
#ifdefs in that list without breaking binary compatibility. The RightThing is
probably to remove the #ifdefs entierly and allocate the id's even for non-svg
builds. If you do that you can proabably put all the svg-ids at the bottom of
the list.

r=me assuming you get peterv or jst to sr
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-12-15 16:50:58 PST
Comment on attachment 137490 [details] [diff] [review]
Changes to content/

hmm.. i think i'd be more comfortable having someone that knows the style-code
look at this patch
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-12-15 16:56:38 PST
Comment on attachment 137493 [details] [diff] [review]
Changes to dom/

btw, we might want to use another scripthelper for things like lengthlists so
that you can access members in them using [] in javascript. But that can be
done separatly IMHO.
Comment 28 Alex Fritze 2003-12-17 12:05:10 PST
Created attachment 137596 [details] [diff] [review]
Changes to dom/

Updated patch incorporating Jonas's comments. I've moved all the SVG classinfo
ids to the end of the lists, but kept them in #ifdef's for now.
Comment 29 Benjamin Smedberg [:bsmedberg] 2003-12-18 07:52:31 PST
Comment on attachment 137489 [details] [diff] [review]
Changes to allmakefiles.sh/configure.in/autoconf.mk.in

>Index: allmakefiles.sh
>@@ -1215,6 +1215,11 @@
> 	layout/svg/Makefile
> 	layout/svg/base/Makefile
> 	layout/svg/base/src/Makefile
>+    layout/svg/renderer/Makefile
>+    layout/svg/renderer/public/Makefile
>+    layout/svg/renderer/src/Makefile
>+    layout/svg/renderer/src/gdiplus/Makefile
>+    layout/svg/renderer/src/libart/Makefile

Please be consisitent: one tab here, instead of spaces.

>Index: configure.in
> MOZ_ARG_ENABLE_BOOL(svg,
>-[  --enable-svg            Enable SVG support],
>+[  --enable-svg            Enable SVG support ],
>     MOZ_SVG=1,
>     MOZ_SVG= )
> if test -n "$MOZ_SVG"; then
>-    AC_DEFINE(MOZ_SVG)
>+  AC_DEFINE(MOZ_SVG)
> fi
>+AC_SUBST(MOZ_SVG)

Please leave the AC_SUBST where it was at the end. We try to group them all
together. Is there any reason we need the extra space in the title?

>+
>+MOZ_ARG_ENABLE_BOOL(svg-renderer-gdiplus,
>+[  --enable-svg-renderer-gdiplus Enable SVG gdi+ renderer ],
>+    MOZ_SVG_RENDERER_GDIPLUS=1,
>+    MOZ_SVG_RENDERER_GDIPLUS= )
>+if test -n "$MOZ_SVG_RENDERER_GDIPLUS"; then
>+  AC_DEFINE(MOZ_SVG_RENDERER_GDIPLUS)
>+fi
>+AC_SUBST(MOZ_SVG_RENDERER_GDIPLUS)
>+
>+MOZ_ARG_ENABLE_BOOL(svg-renderer-libart,
>+[  --enable-svg-renderer-libart Enable SVG libart renderer ],
>+    MOZ_SVG_RENDERER_LIBART=1,
>+    MOZ_SVG_RENDERER_LIBART= )
>+if test -n "$MOZ_SVG_RENDERER_LIBART"; then
>+  AC_DEFINE(MOZ_SVG_RENDERER_LIBART)
>+fi
>+AC_SUBST(MOZ_SVG_RENDERER_LIBART)

Is there any situation where you would build both of these? if not, we should
use one configure option instead of two:

MOZ_ARG_ENABLE_STRING(svg-renderer,
[  --enable-svg-renderer=gdiplus|libart
			  select the SVG rendering library],
[MOZ_SVG_RENDERER_LBIRARY=`echo $withval | tr A-Z a-z`])

if test -z "$MOZ_CHROME_FILE_FORMAT"; then
    MOZ_CHROME_FILE_FORMAT=jar
fi

if test "$MOZ_CHROME_FILE_FORMAT" != "gdiplus" && 
    test "$MOZ_CHROME_FILE_FORMAT" != "libart"; then
    AC_MSG_ERROR([--enable-svg-renderer must be set to either gdiplus or
libart])
fi

Also, please add a platform test so that we don't enable gdiplus on non-windows
platforms.
Comment 30 Robert Kaiser 2003-12-18 12:46:10 PST
bsmergberg:
I guess the MOZ_CHROME_FILE_FORMAT in your post above is a copy/paste error ;-)

Just wanted to note it before someone might take your lines and simply
copy/paste them without checking if they're right ;-)
Comment 31 Alex Fritze 2003-12-18 15:02:19 PST
Created attachment 137674 [details] [diff] [review]
Changes to allmakefiles.sh/configure.in/autoconf.mk.in

Updated patch incorporating Benjamin's comments:

1. Whitespace in allmakefiles.sh should now be consistent.
2. I've moved all svg-related AC_SUBSTs to the list towards the end of
configure.in
3. I've left the renderers as 2 separate options that can be built at the same
time. I'm aiming for some sort of user preferences-driven selection of
rendering backend, maybe using different backends for different output devices
(screen, printer)...
4. I've added a test checking for the Gdiplus.h header.
Comment 32 David Baron :dbaron: ⌚️UTC-10 2003-12-19 13:15:20 PST
Comment on attachment 137490 [details] [diff] [review]
Changes to content/

>+const nsStyleStruct* 
>+nsRuleNode::ComputeSVGResetData(nsStyleStruct* aStartStruct, const nsCSSStruct& aData, 

The second parameter should be |const nsRuleDataStruct& aData|.

>Index: content/base/src/nsStyleContext.cpp
>           (int)svg->mFill.mType,
>           svg->mFillOpacity);
>   fprintf(out, "\" />\n");

There are some additional properties in the SVG struct that you should print.

>Index: content/shared/public/nsRuleNode.h
>+  const nsStyleStruct* ComputeSVGResetData(nsStyleStruct* aStartSVG, const nsCSSStruct& aSVGData, 
>+                                           nsStyleContext* aContext,  
>+                                           nsRuleNode* aHighestNode,
>+                                           const RuleDetail& aRuleDetail, PRBool aInherited);

Likewise (comment before last).

>Index: content/shared/src/nsCSSProps.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/shared/src/nsCSSProps.cpp,v
>retrieving revision 3.92
>retrieving revision 3.69.8.23
>diff -u -r3.92 -r3.69.8.23
>--- content/shared/src/nsCSSProps.cpp	30 Oct 2003 01:50:59 -0000	3.92
>+++ content/shared/src/nsCSSProps.cpp	16 Nov 2003 07:46:58 -0000	3.69.8.23
>@@ -865,12 +865,50 @@
> 
> #ifdef MOZ_SVG
> // keyword tables for SVG properties
>+
>+const PRInt32 nsCSSProps::kDominantBaselineKTable[] = {

>+  eCSSKeyword_text_top, NS_STYLE_DOMINANT_BASELINE_TEXT_TOP,
>+  eCSSKeyword_text_bottom, NS_STYLE_DOMINANT_BASELINE_TEXT_BOTTOM,

where did these values come from?

>Index: content/shared/src/nsStyleStruct.cpp

>+PRInt32 

This should return nsChangeHint (header file too).

>+nsStyleSVGReset::CalcDifference(const nsStyleSVGReset& aOther) const

I'm skeptical (here and for nsStyleSVG) that everything should return
NS_STYLE_HINT_VISUAL.

r=dbaron with those changes and an explanation of where those extra value came
from
Comment 33 Alex Fritze 2003-12-22 14:36:00 PST
Created attachment 137858 [details] [diff] [review]
Changes to content/

Updated patch incorporating David's changes:

1. Corrected signatures of ComputeSVGResetData(.) in nsRuleNode.{h,cpp}. (I'm
surprised that my compiler didn't give me a warning here!).

2. Printing all SVG properties in nsStyleContext::DumpRegressionData(.) in
nsStyleContext.cpp.

3. About eCSSKeyword_text_top, ...,
	 eCSSKeyword_text_bottom ... :
   I presume your question is why these constants are commented out in 
   nsCSSKeywordList.h? This is because they are already defined outside of the 

   ifdef'ed SVG section in nsCSSKeywordList. I wanted to keep them in the  
   ifdef'ed part just for completeness until the svg keys get spliced into 
   the list properly.

4. In nsStyleStruct.{h,cpp}: Corrected signature of
nsStyleSVGReset::CalcDifference(.).

5. I'm not sure whether SVG changes should return NS_STYLE_HINT_VISUAL or
NS_STYLE_HINT_NONE. None of the changes cause framechanges or reflows and SVG
doesn't use the nsIFrame::AttributeChanged() mechanism at all. (I think there
was a reason for returning NS_STYLE_HINT_VISUAL, but I can't remember...).
Comment 34 David Baron :dbaron: ⌚️UTC-10 2004-01-01 09:28:23 PST
> 3. About eCSSKeyword_text_top, ...,
> 	 eCSSKeyword_text_bottom ... :

My question was about why they're there.  I don't see them in the SVG spec.  If
they're not in a spec, they should be prefixed with -moz-.

> 5. I'm not sure whether SVG changes should return NS_STYLE_HINT_VISUAL or
> NS_STYLE_HINT_NONE.

How are dynamic changes to these properties handled?  If a dynamic change to
these properties needs to cause a repaint, then you should return
NS_STYLE_HINT_VISUAL.  If it needs to cause a reflow, then NS_STYLE_HINT_REFLOW,
etc.  I find it hard to believe that you don't need a reflow for some of these
properties.  (Do you have testcases for dynamic changes to these properties?)
Comment 35 David Baron :dbaron: ⌚️UTC-10 2004-01-01 09:37:24 PST
Comment on attachment 137497 [details] [diff] [review]
Changes to layout/

>Index: layout/base/public/nsStyleConsts.h
>+// Some of our constants must map to the same values as those defined in
>+// nsISVG{,Path,Glyph}GeometrySource.idl/
>+// I don't want to add a dependency on the SVG module
>+// everywhere by #include'ing nsISVG{,Path,Glyph}GeometrySource.h, so these consts
>+// have to be kept in sync manually.

You don't need to introduce a header dependency -- you can just define the
constants as nsISVGGeometrySource::FOO and require anybody who uses the
constants to include the appropriate header.  Either way is fine with me,
though.

>Index: layout/html/base/src/nsFrameManager.cpp
>+#ifdef MOZ_SVG
>+  nsCOMPtr<nsISVGContent> svg(do_QueryInterface(aContent));
>+  if (svg) {
>+    svg->IsPresentationAttribute(aAttribute, aResult);
>+    if (*aResult)
>+      return NS_OK;
>+  }
>+#endif

There ought to be a more appropriate place for this.  How do these atttributes
influence style?  Isn't there a stylesheet object somewhere in whose
implementation of HasAttributeDependentStyle this belongs?

Also, this will have merge conflicts with my patch in bug 15608.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp

I'm not wild about the changes to ConstructTextFrame, but it's probably simpler
than writing a custom ProcessChildren.
Comment 36 Jonas Sicking (:sicking) No longer reading bugmail consistently 2004-01-01 16:51:23 PST
The only properties (css or dom-attributes) that should need to cause a reflow
are ones that affect the size of the image itself. All other changes happen in
the existing imageframe, just as for gif-animations.
Comment 37 Alex Fritze 2004-01-07 10:46:10 PST
> 3. About eCSSKeyword_text_top, ...,
>        eCSSKeyword_text_bottom ... :

These were valid values for 'dominant-baseline' in the SVG 1.0 standard, but it
looks like they have since been removed in SVG 1.1 to make 'dominant-baseline'
"consistent with XSL definitions".I've removed them from our implementation now.
Affected files: content/shared/src/nsCSSProps.cpp,
content/shared/public/nsCSSKeywordList.h, layout/svg/base/src/nsSVGTextFrame.cpp.

> How are dynamic changes to these properties handled?  

SVG implements its own observer-based notification system to handle dynamic
changes to attributes/dom properties.
As a typical example let's e.g. assume a change to the "r" (radius) attribute of
a <circle> element. 

The nsSVGCircleElement object, has a child "mR" of type nsSVGAnimatedLength.
This child holds the state of the "r" attribute in native form and implements
the required interfaces for the SVG DOM, in this case 'nsIDOMSVGAnimatedLength',
so that it can be accessed as something like circle.r.baseVal.value = 5. 

nsSVGAnimatedLength also implements the nsISVGValue interface which defines
facilities for setting/getting the attribute's value as a string and facilities
for adding/removing observers (of type nsISVGValueObserver).

A call such as circle.setAttribute("r",5) will ultimately feed through to mR's
SetValueString(). This method, in turn, will inform all observers of the value
change with a call to nsISVGValueObserver::WillModify() and
nsISVGValueObserver::DidModify(). Directly setting the value through the SVG DOM
(with a call such as circle.r.baseVal.value=5) has the same effect, only the
conversion from string to native value is not performed.

The circle element's corresponding frame (nsSVGCircleFrame) sets itself up as a
listener to mR when constructed. When the frame is informed of a change in mR
through a call to nsSVGCircleFrame::DidModifySVGObservable, it updates its
associated (renderer-specific) path information, collecting information about
the areas that need to be redrawn from the renderer. Finally, the frame informs
the outer svg frame (nsSVGOuterSVGFrame) of the dirty areas. The outer svg frame
will then call UpdateView() for these areas on its associated view.

(This is somewhat simplified - there is also a mechanism for batching attribute
changes)

> If a dynamic change to
> these properties needs to cause a repaint, then you should return
> NS_STYLE_HINT_VISUAL.  If it needs to cause a reflow, then
> NS_STYLE_HINT_REFLOW, etc.  

Most svg attribs work in the way described above, and on further reflection I
think they should return NS_STYLE_HINT_NONE, since afaics all that
NS_STYLE_HINT_VISUAL does is to call UpdateView(), which for SVG is done by the
outer svg frame.

> I find it hard to believe that you don't need a 
> reflow for some of these properties.  

As Jonas says, the only attributes that require reflow are those that affect the
size of the outer svg element (i.e. "width" and "height", but *only* in the
context of an *outer* svg element). The way this is handled at the moment is by
the outer svg element creating a reflow command and posting it with
mPresShell->AppendReflowCommand().

> (Do you have testcases for dynamic 
> changes to these properties?)

Not formal ones, but some of the examples on croczilla.com are dynamic (e.g.
http://www.croczilla.com/svg/xbl-shapes.xml or
http://www.croczilla.com/svg/xbl1.xml or 
http://www.croczilla.com/svg/svgtetris.xml).

Comment 38 Alex Fritze 2004-01-08 10:37:22 PST
> How are dynamic changes to these properties handled?  

Please ignore my reply in comment 37. I got confused between SVG style
properties and non-style properties, describing how the latter work in SVG,
which was of course irrelvant in the context of your comment.

At the moment style properties are simply handled by updating a frame's
associated path data when DidSetStyleContext() is being called, e.g.:

NS_IMETHODIMP
nsSVGPathGeometryFrame::DidSetStyleContext(nsIPresContext* aPresContext)
{
  // XXX: we'd like to use the style_hint mechanism and the
  // ContentStateChanged/AttributeChanged functions for style changes
  // to get slightly finer granularity, but unfortunately the
  // style_hints don't map very well onto svg. Here seems to be the
  // best place to deal with style changes:

  UpdateGraphic(nsISVGGeometrySource::UPDATEMASK_ALL);

  return NS_OK;
}

This is of course not an optimal solution, but it's the way in which it already
works in the trunk SVG code now.
Again, I haven't got any formal testcases, but some of the examples on
croczilla.com have dynamic style (e.g.
http://www.croczilla.com/svg/polygons3.xml or
http://www.croczilla.com/svg/circles2.xml).


>>Index: layout/base/public/nsStyleConsts.h
>>+// Some of our constants must map to the same values as those defined in
>>+// nsISVG{,Path,Glyph}GeometrySource.idl/
>>+// I don't want to add a dependency on the SVG module
>>+// everywhere by #include'ing nsISVG{,Path,Glyph}GeometrySource.h, so these
>consts
>>+// have to be kept in sync manually.
>
>You don't need to introduce a header dependency -- you can just define the
>constants as nsISVGGeometrySource::FOO and require anybody who uses the
>constants to include the appropriate header.  Either way is fine with me,
>though.

Since you don't mind it's probably easiest if we keep the code as it is for now.
The constants are currently used by nsCSSProps.cpp and nsStyleStruct.cpp, so we
would need to include the appropriate headers there and add the appropriate
include path to the corresponding makefiles...


>>Index: layout/html/base/src/nsFrameManager.cpp
>>+#ifdef MOZ_SVG
>>+  nsCOMPtr<nsISVGContent> svg(do_QueryInterface(aContent));
>>+  if (svg) {
>>+    svg->IsPresentationAttribute(aAttribute, aResult);
>>+    if (*aResult)
>>+      return NS_OK;
>>+  }
>>+#endif
>
>There ought to be a more appropriate place for this.  How do these atttributes
>influence style?  Isn't there a stylesheet object somewhere in whose
>implementation of HasAttributeDependentStyle this belongs?

Yeah, there has to be a better way of doing this.
Style-affecting SVG attribs such as "stroke-width" get mapped into a content
style rule which gets picked up in nsSVGElement::WalkContentStyleRules(). For
this to work dynamically we need to identify these attributes as influencing
style in some HasAttributeDependentStyle() method. We haven't got any SVG
stylesheets, hence the test in nsFrameManager. I don't know the style system
very well at all, so I'd appreciate any pointers as to how to implement this
correctly.

>>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>
>I'm not wild about the changes to ConstructTextFrame, but it's probably simpler
>than writing a custom ProcessChildren.

I think a custom ProcessChildren wouldn't be enough, since frames might get
inserted dynamically in which case we'd end up in the regular
ConstructFrame/ConstructTextFrame again if there's no special casing somewhere. 
Comment 39 Alex Fritze 2004-02-05 14:56:41 PST
I changed the occurrences of $(NSNULL) in layout/build/Makefile.in to $(NULL).
Thanks to biesi for spotting this.
Also removed the comment line in nsCSSFrameConstructor.cpp:
//XXX uh oh. the block we need to reframe has no parent!");
and left in the equivalent NS_ERROR() instead.
SVG code occasionally hits this error when manipulating content in iframes in
foreignObject tags IIRC.

New patch coming shortly.
Comment 40 Alex Fritze 2004-02-05 15:14:22 PST
Created attachment 140699 [details] [diff] [review]
Changes to layout
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2004-02-06 00:17:45 PST
Comment on attachment 140699 [details] [diff] [review]
Changes to layout

>Index: layout/html/base/src/nsFrameManager.cpp
>+#ifdef MOZ_SVG
>+#include "nsISVGContent.h"
>+#endif

This file is not in the content or layout diffs.... I assume you didn't bother
to attach changes to svg-only code?

>+    svg->IsPresentationAttribute(aAttribute, &isPresAttr);

Like dbaron said, we really want to make this happen "naturally" and kill this
ifdef.	Please talk to sicking about attribute mapping and sharing the generic
attribute implementation, ok?

sr=bzbarsky on the layout changes.
Comment 42 Alex Fritze 2004-02-06 02:32:17 PST
(In reply to comment #41)
> (From update of attachment 140699 [details] [diff] [review])
> >Index: layout/html/base/src/nsFrameManager.cpp
> >+#ifdef MOZ_SVG
> >+#include "nsISVGContent.h"
> >+#endif
> 
> This file is not in the content or layout diffs.... I assume you didn't bother
> to attach changes to svg-only code?

No, the patches would have become too chunky and the thinking was that only the
changes to non-svg-exclusive files could affect stability of non-svg builds.
At some point, of course, (definitly before this code has any change of being
switched on by default), the whole svg code will need to get reviewed.
 
> >+    svg->IsPresentationAttribute(aAttribute, &isPresAttr);
> 
> Like dbaron said, we really want to make this happen "naturally" and kill this
> ifdef.	

I'm really going to need help from you content/layout-savvy people, so the best
way to proceed is probably if I open a specific bug on this after the branch lands.

> Please talk to sicking about attribute mapping and sharing the generic
> attribute implementation, ok?

Yeah, I am doing that already.

> sr=bzbarsky on the layout changes.
> 
Thanks! 
1 "r" & 2 "sr"s to go! We might just make it for 1.7!
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2004-02-06 10:24:54 PST
Comment on attachment 137858 [details] [diff] [review]
Changes to content/

>Index: content/base/src/nsRuleNode.cpp
>+nsRuleNode::ComputeSVGResetData(nsStyleStruct* aStartStruct,
>+    parentSVGReset = NS_STATIC_CAST(const nsStyleSVGReset*,
>+                                    parentContext->GetStyleData(eStyleStruct_SVGReset));

Shouldn't nsStyleContext have a GetStyleSVGReset() method that returns a |const
nsStyleSVGReset*|?  You want to use that here.

>Index: content/base/src/nsStyleContext.cpp
>+  const nsStyleSVGReset* svgReset = (const nsStyleSVGReset*)GetStyleData(eStyleStruct_SVGReset);

Same.

sr=bzbarsky with those two changes.
Comment 44 Boris Zbarsky [:bz] (still a bit busy) 2004-02-06 10:27:54 PST
Comment on attachment 137494 [details] [diff] [review]
Changes to gfx/

sr=bzbarsky on this too, so once it gets review you can land this....
Comment 45 tor 2004-02-06 10:56:18 PST
Comment on attachment 137494 [details] [diff] [review]
Changes to gfx/

r=tor, but keep in mind that we hope/plan to remove the freetype 
code in the future.
Comment 46 Alex Fritze 2004-02-07 07:45:20 PST
The branch has now landed, so marking FIXED.
Thanks to all reviewers for the quick reviews and to David Avery for helping to
keep this branch going for so long.
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2004-02-07 12:13:38 PST
I filed bug 233370 on the attribute-mapping code.
Comment 48 David Baron :dbaron: ⌚️UTC-10 2004-02-07 12:39:00 PST
On the monkeypox LinuxPPC tinderbox, I disabled SVG because of:

/builds/tinderbox/SeaMonkey/Linux_2.2.15pre3_Depend/mozilla/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:313:
#error "No SVG renderer."
Comment 49 David Baron :dbaron: ⌚️UTC-10 2004-02-07 12:41:16 PST
The AIX tinderbox (laredo) on Seamonkey-Ports is failing with:

art_affine.c
"/usr/include/math.h", line 236.9: 1506-213 (S) Macro name M_PI cannot be redefined.
"/usr/include/math.h", line 242.9: 1506-213 (S) Macro name M_SQRT2 cannot be
redefined.

In the directory 
/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/obj-tinder/other-licenses/libart_lgpl
The following command failed to execute properly:
xlc_r -o art_affine.o -c -DOSTYPE="AIX5.1" -DOSARCH="AIX" -DLIBART_COMPILATION
-I../../dist/include/libart_lgpl -I../../dist/include
-I/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/obj-tinder/dist/include/nspr
-qflag=w:w -DNDEBUG -DTRIMMED -O2 -qmaxmem=-1 -qalias=noansi -DAIX=1
-DHAVE_SYS_INTTYPES_H=1 -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1 -DD_INO=d_ino
-DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1
-DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_INT64=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1
-DHAVE_UINT16_T=1 -DHAVE_DIRENT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1
-DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1
-DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_LIBC_R=1 -DHAVE_LIBM=1
-DHAVE_LIBDL=1 -DHAVE_LIBC_R=1 -DFUNCPROTO=15 -DHAVE_XSHM=1 -D_REENTRANT=1
-DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1
-DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_NL_LANGINFO=1
-DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHA!
 VE_RES_NINI
T=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_DEFAULT_TOOLKIT="gtk" -DMOZ_WIDGET_GTK=1
-DMOZ_ENABLE_XREMOTE=1 -DMOZ_X11=1 -DMOZ_ENABLE_COREXFONTS=1
-DMOZ_EXTRA_X11CONVERTERS=1 -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1
-DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DMOZ_MATHML=1 -DMOZ_SVG=1
-DMOZ_LOGGING=1 -DMOZ_USER_DIR=".mozilla" -DMOZ_XUL=1 -DMOZ_PROFILESHARING=1
-DMOZ_PROFILELOCKING=1 -DSUNCTL=1 -DMOZ_BYPASS_PROFILE_AT_STARTUP=1
-DMOZ_DLL_SUFFIX=".so" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -DJS_THREADSAFE=1
-DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1 -DMOZILLA_VERSION="1.7a"
-DMOZILLA_LOCALE_VERSION="1.7a" -DMOZILLA_REGION_VERSION="1.7a"
-D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT
/home/tbox/builds/tinderbox/AIX_5.1_Clobber/mozilla/other-licenses/libart_lgpl/art_affine.c
gmake[3]: *** [art_affine.o] Error 1

I guess on monkeypox and otaku what I should really do is add:
mk_add_options MOZ_INTERNAL_LIBART_LGPL=1
MOZ_INTERNAL_LIBART_LGPL=1
Comment 50 David Baron :dbaron: ⌚️UTC-10 2004-02-07 12:52:07 PST
Er, they already had that, but I added --enable-svg-renderer-libart .  Should
have read comment 0, although it would have been nice to have a heads up that
we'd need tinderbox configuration changes for the machines building SVG.
Comment 51 David Baron :dbaron: ⌚️UTC-10 2004-02-08 13:26:11 PST
While the includes in art_affine.c mentioned in comment 49 were reordered, the
same problem is now happening in another file.  Isn't the real problem that
art_misc.h does:

#ifndef M_PI
#define M_PI 3.14159265358979323846

without including math.h?  Shouldn't art_misc.h just include math.h?
Comment 52 Alex Fritze 2004-02-08 14:31:06 PST
(In reply to comment #51)
> While the includes in art_affine.c mentioned in comment 49 were reordered, the
> same problem is now happening in another file.  Isn't the real problem that
> art_misc.h does:
> 
> #ifndef M_PI
> #define M_PI 3.14159265358979323846
> 
> without including math.h?  Shouldn't art_misc.h just include math.h?

Done.

Note You need to log in before you can comment on or make changes to this bug.