252637 - Implement inner <svg> elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Fritze]

Add support for nested <svg> elements.
Testcase: 
http://www.croczilla.com/~alex/reference/SVG/REC-SVG11-20030114/images/coords/PreserveAspectRatio.svg
And what it should look like:
http://www.croczilla.com/~alex/reference/SVG/REC-SVG11-20030114/images/coords/PreserveAspectRatio.png

Comment 1

[Alex Fritze]

Attachment: first attempt

This patch implements inner <svg> elements, fixes percentage units (not for
strokewidths - that's a different bug), and introduces some caching of
transform matrices in container frames (<defs>, <g>, <svg>).

Comment 2

[tor]

Comment on attachment 154013 [details] [diff] [review]
first attempt

Discussed main things with afri on irc.  A few minor nits:

  * you've changed a number of interfaces without creating new IIDs

  * nsSVGCoordCtx.cpp has only a couple trivial functions - why
      not put them in the header?

  * expand the comment for nsSVGCoordCtxHolder to explain the
      reason for theis class (hiding the observer interface)

  * nsSVGSVGElement::GetViewboxToViewportTransform - you've moved
      the initialization of "e,f" to top - don't need to rezero
      in the conditions

  * nsCSSFrameConstructor::ConstructSVGFrame - setting processChildren
      can be floated above the "if (!container) ..."

Comment 3

[Alex Fritze]

Attachment: 2nd attempt

New patch updated to trunk & containing tor's changes.

Comment 4

[Alex Fritze]

Attachment: 3rd attempt

New patch:
- updated to the trunk again
- stray directory/c-sdk diffs removed
- added some more exports to content/svg/content/src/Makefile.in so that the
build doesn't fail in nsCSSFrameConstructor.cpp

Comment 5

[Alex Fritze]

Attachment: 4th attempt

Patch updated to trunk again.

Comment 6

[tor]

Running with this patch, I came across a crash in the new code
on the following page:

  http://www.w3.org/Consortium/Offices/Presentations/SVG/0.svg

#0  0x07e3c79c in nsRefPtr<nsSVGCoordCtxHolder>::get() const (this=0x4)
    at nsAutoPtr.h:1022
#1  0x07e3c78e in nsRefPtr<nsSVGCoordCtxHolder>::operator
nsDerivedSafe<nsSVGCoordCtxHolder>*() const (this=0x4) at nsAutoPtr.h:1035
#2  0x07e5c50b in nsSVGCoordCtxProvider::GetContextX() (this=0x0)
    at nsSVGCoordCtxProvider.h:102
#3  0x07e995b0 in
nsSVGSVGElement::SetParentCoordCtxProvider(nsSVGCoordCtxProvider*)
(this=0x40b50440, parentCtx=0x0)
    at /home/tor/moz/marker/mozilla/content/svg/content/src/nsSVGSVGElement.cpp:1059
#4  0x07e31f0b in nsSVGInnerSVGFrame::Init() (this=0x40b6b400)
    at /home/tor/moz/marker/mozilla/layout/svg/base/src/nsSVGInnerSVGFrame.cpp:170
#5  0x07e32086 in nsSVGInnerSVGFrame::Init(nsPresContext*, nsIContent*,
nsIFrame*, nsStyleContext*, nsIFrame*) (this=0x40b6b400, aPresContext=0x8b34f18, 
    aContent=0x40b50440, aParent=0x40b6b250, aContext=0x40b6b354, 
    aPrevInFlow=0x0)
    at /home/tor/moz/marker/mozilla/layout/svg/base/src/nsSVGInnerSVGFrame.cpp:198
#6  0x079e5a1e in nsCSSFrameConstructor::InitAndRestoreFrame(nsPresContext*,
nsFrameConstructorState&, nsIContent*, nsIFrame*, nsStyleContext*, nsIFrame*,
nsIFrame*) (this=0x8b47e30, aPresContext=0x8b34f18, aState=@0xbff63350, 
    aContent=0x40b50440, aParentFrame=0x40b6b250, aStyleContext=0x40b6b354, 
    aPrevInFlow=0x0, aNewFrame=0x40b6b400)
    at
/home/tor/moz/marker/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:6582
...

Comment 7

[Alex Fritze]

So the problem here is that some of the parent frames of the inner <svg> aren't
svg frames (due to a spelling mistake: "xmnls" instead of "xmlns").
A condensed testcase is at http://www.croczilla.com/~alex/testcases/252637.svg.

New safer patch coming up.

Comment 8

[Alex Fritze]

Attachment: 5th attempt

This only differs from the previous patch in
nsSVGSVGElement::SetParentCoordCtxProvider() where I've put in a null check.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 155197 [details] [diff] [review]
5th attempt

sr=dbaron on the nsCSSFrameConstructor.cpp changes only, except that I think
you want to get rid of the isBlock variable in ConstructSVGFrame and always
pass PR_FALSE to ProcessChildren instead.

Also (for other parts of the patch), the Mozilla convention is to initialize
refcounts to 0, and it looks like you'd simplify the code by following that and
avoiding a bunch of dont_AddRef(new ...)

Comment 10

[Alex Fritze]

tor, dbaron: Thanks for the reviews!

Checked in with inBlock variable removed, as per dbaron's suggestion.
I've kept the refcounts initialized to 1, because we do stuff in ctors that
requires a stable ref-count (specifically setting observers in
nsSVGViewbox::nsSVGViewbox()).