Closed Bug 374111 Opened 13 years ago Closed 11 years ago

DeCOMtaminate SVG viewBox

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: jwatt, Assigned: craig.topper)

References

Details

Attachments

(1 file, 4 obsolete files)

My current thinking is that we should should store an internal viewBox data structure as part of a new viewBox nsAttrValue type. That way we don't use any space for viewBox attributes if they aren't set, and we can check for them/get their value efficiently enough using GetAttr.
It's worth noting that viewBox can be animated and we currently use the whole nsISVGValue notification mechanism thing to notify dependent code when they change.
I'm also wondering if we should change the nsSVGElement::DidChangeLength method that was introduced when nsSVGLength was deCOMtaminated to something more generic.
Summary: DeCOMtaminate SVG viewBox → DeCOMtaminate SVG viewBox and preserveAspectRatio
So I've rewritten the SVG code to use new deCOMtaminated viewBox and preserveAspectRatio objects. Attached is my new nsSVGPreserveAspectRatio class which encodes base vals, anim vals, set/unset state and animation count all within 32 bits so that it can be used as a member of naAttrValue's MiscContainer union.

Unfortunately, I've just realized that storing animation state on nsAttrValue objects would preclude animation of attributes that aren't set. Now I'm wondering if I should have nsSVGViewBox/nsSVGPreserveAspectRatio members directly on element classes (so all applicable elements have memory allocated for viewBox and preserveAspectRatio, whether the attributes are set or not), or if I should continue the nsAttrValue route for the base vals and have the animated values stored "somewhere else". If the latter approach is taken, how does the painting code get the right values (base or animated) in an efficient way? Any thoughts from anyone?
Note to self: consider storing the viewBox to viewport transform in the attribute too.
Blocks: 413960
Blocks: 435525
I would do what we did for lengths etc: declare members directly on element classes, but ensure that the structure is small for default, non-animated values. For preserve-aspect ratio that's trivial, for viewbox we might want to just have a pointer where null means no viewbox and we heap-allocate a (base, anim) pair of rectangles as necessary.
That means we allocate memory for viewBox/PAR regardless of whether they're set or not. Since they're often not set, I'd like to avoid that.

How about we store the base values as an nsAttrValue as necessary, and we have a single pointer on element's for allocating a unified chunk of memory for viewBox/PAR animated values in the *extremely* rare case that one or both of them is animated?

Alternatively, I'm wondering if we should store memory for animated values in a similar way to the way we store attributes. That way we're not continually bulking up each element class with in-case-we-animate memory as we add animation capabilities to yet more of it's attributes.
(In reply to comment #6)
> That means we allocate memory for viewBox/PAR regardless of whether they're set
> or not. Since they're often not set, I'd like to avoid that.

so that's 8 bytes per <svg> and <g> element, say? I think that's OK.
Blocks: 444970
Apparently Robert and Craig fixed preserveAspectRatio in bug 470911.

Craig as now looking at viewBox, so assigning this to him.
Assignee: jwatt → craig.topper
Summary: DeCOMtaminate SVG viewBox and preserveAspectRatio → DeCOMtaminate SVG viewBox
Attached patch Patch (obsolete) — Splinter Review
Couple notes on this patch.

I had to remove the code that made nsSVGLength an observer of the viewbox in the <svg> element. I'm not sure this observer was needed. Seems it wouldn't have worked if there was no viewbox and the length instead relied on the width and height of the <svg> element for its relative size. In that case it wouldn't have been notified about the width/height changes.

Also removed the DidModify/WillModify calls from nsSVGLength::SetContext because all they would do is notify a SVGLengthList about the change in context. But SVGLengthList is the only caller of SetContext and already knew the context was changing.

This patch also fixes a bug where changes to the <svg> element viewbox through the AnimatedRect interface didn't cause a redraw. This was because AfterSetAttr was the only detection of a change in viewbox, but that only gets called on the SetAttribute path, because nsSVGElement::DidModifySVGObservable passes nsnull to the aValueForAfterSetAttr of SetAttrAndNotify. Though even it didn't, nsSVGSVGElement::DidModifySVGObservable didn't actually call nsSVGElement.
Attachment #261270 - Attachment is obsolete: true
Attachment #357628 - Flags: review?(roc)
Oh forgot to mention I also ripped the nsSVGValue stuff out of the nsSVGRect because now that nsSVGAnimatedRect is gone there are no observers of it.
In nsSVGMarkerElement::UnsetAttr and nsSVGMarkerElement::DidChangeLength:

+    mViewBox.SetBaseValue(0, 0, mLengthAttributes[MARKERWIDTH].GetAnimValue(mCoordCtx),
+                          mLengthAttributes[MARKERHEIGHT].GetAnimValue(mCoordCtx),
+                          this, PR_FALSE);

This worries me; if the units are percentages and something changes in the outer nsSVGSVGElement that affects the result of nsSVGSVGElement::GetLength, how does mViewBox get updated here?

I think a better way to go would be to have nsSVGViewBox not store this default rect. Instead it should have a flag to indicate whether it's the default; the getter methods should check this flag and dynamically look up the default if that flag is set.

 nsSVGMarkerElement::SetParentCoordCtxProvider(nsSVGSVGElement *aContext)
 {
   mCoordCtx = aContext;
   mViewBoxToViewportTransform = nsnull;
 
   if (mCoordCtx && !HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
-    nsCOMPtr<nsIDOMSVGRect> vb;
-    mViewBox->GetAnimVal(getter_AddRefs(vb));
-    vb->SetWidth(mLengthAttributes[MARKERWIDTH].GetAnimValue(mCoordCtx));
-    vb->SetHeight(mLengthAttributes[MARKERHEIGHT].GetAnimValue(mCoordCtx));
+    mViewBox.SetBaseValue(0, 0, mLengthAttributes[MARKERWIDTH].GetAnimValue(mCoordCtx),
+                          mLengthAttributes[MARKERHEIGHT].GetAnimValue(mCoordCtx),
+                          this, PR_FALSE);
   }

This is gets weird. SetParentCoordCtxProvider is called in nsSVGMarkerFrame::PaintMark (via AutoMarkerReferencer) to temporarily transform the marker's coordinates while we paint the mark. Do we really want to change mViewBox here, even temporarily, to match the viewbox of the container of the path we're drawing markers for? If we do need to use that viewbox, then again I think we should just use a flag on it to say "I'm the default", and then when we query it, it can find the right mCoordCtx at that time. Would be cool to have a test for this situation...

> This patch also fixes a bug where changes to the <svg> element viewbox through
> the AnimatedRect interface didn't cause a redraw.

Could you write a test for that? It should be easy using MozReftestInvalidate.

  nsSVGViewBoxRect viewbox;
   if (HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
-    nsCOMPtr<nsIDOMSVGRect> vb;
-    mViewBox->GetAnimVal(getter_AddRefs(vb));
-    NS_ASSERTION(vb, "could not get viewbox");
-    vb->GetX(&viewboxX);
-    vb->GetY(&viewboxY);
-    vb->GetWidth(&viewboxWidth);
-    vb->GetHeight(&viewboxHeight);
+    viewbox = mViewBox.GetAnimValue();
   } else {
-    viewboxX = viewboxY = 0.0f;
-    viewboxWidth = viewportWidth;
-    viewboxHeight = viewportHeight;
+    viewbox.x = viewbox.y = 0.0f;
+    viewbox.width  = viewportWidth;
+    viewbox.height = viewportHeight;
   }

Shouldn't this just always use mViewBox.GetAnimValue()? The value might be animated while no attribute is set.

   if (HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
-    mViewBox->GetAnimVal(getter_AddRefs(vb));
-    vb->GetWidth(&w);
-    vb->GetHeight(&h);
+    const nsSVGViewBoxRect& viewbox = mViewBox.GetAnimValue();
+    w = viewbox.width;
+    h = viewbox.height;
   } else {
     nsSVGSVGElement *ctx = GetCtx();
     if (ctx) {
       w = mLengthAttributes[WIDTH].GetAnimValue(ctx);
       h = mLengthAttributes[HEIGHT].GetAnimValue(ctx);
     } else {
       w = mViewportWidth;
       h = mViewportHeight;
     }
   }

Same here.

+  nsTextFormatter::snprintf(buf, sizeof(buf)/sizeof(PRUnichar),

NS_ARRAY_LENGTH

We really should do something about this parsing and unparsing code here and elsewhere ... it doesn't really need to heap-allocate, and it's kind of hard to read. We really need some kind of micro-parser-generator... I'll think about it.
+struct nsSVGViewBoxRect
+{
+  float x, y;
+  float width, height;

Why not just use gfxRect here?

     // XXXjwatt we need to fix our viewBox code so that we can tell whether the
     // viewBox attribute specifies a valid rect or not.

jwatt, is this comment still true?
(In reply to comment #12)
> +struct nsSVGViewBoxRect
> +{
> +  float x, y;
> +  float width, height;
> 
> Why not just use gfxRect here?

Because gfxRect is 4 doubles rather than 4 floats.
Roc, seems like your comments are all bugs that already exist in the code. Should this be addressed in a separate bug?
(In reply to comment #12)
> jwatt, is this comment still true?

Yup, but it doesn't need to be addressed here.
Attached patch Updated to tip (obsolete) — Splinter Review
My SVG string cleanup patch conflicted with the previous version of this one.
Attachment #357628 - Attachment is obsolete: true
We should be checking this in, right?
Keywords: checkin-needed
Whiteboard: [needs landing]
Yeah. Sorry forgot to put the tag on there. I had held off cause I knew the string patch would conflict and then forgot to do it when I updated the patch.
Just noticed that nsSVGSymbolElement is missing an override of GetViewBox(). I'll submit a new patch tomorrow.
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #358145 - Attachment is obsolete: true
Guess it helps to put the checkin-needed back on the bug
Keywords: checkin-needed
Whiteboard: [needs landing]
This could do with some tests.

content/svg/content/test/test_dataTypes.html should have tests for functionality and content/svg/content/test/test_valueLeaks.xhtml should be updated to test this for leaks.
It's in the leak test. I put it in pre-emptively when adding other changes to that test.

I'll work on the dataTypes test.
Attachment #358990 - Attachment is obsolete: true
Comment on attachment 359875 [details] [diff] [review]
Adds test cases to test_dataTypes.html
[Checkin: Comment 26]


http://hg.mozilla.org/mozilla-central/rev/13b5a7af35bb
Attachment #359875 - Attachment description: Adds test cases to test_dataTypes.html → Adds test cases to test_dataTypes.html [Checkin: Comment 26]
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.