Closed Bug 471551 Opened 11 years ago Closed 11 years ago

Remove mBaseVal and change mAnimVal to nsAutoPtr in new style SVG string

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: craig.topper, Assigned: craig.topper)

Details

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081230 Shiretoko/3.1b3pre Firefox/3.1b3pre
Build Identifier: 

Remove mBaseVal and replace with calls to GetAttr on the element. Also change mAnimVal to an nsAutoPtr<nsString> so that we only have the string when the element is animated. If the mAnimVal is null just return the value from GetAttr. This will reduce the storage required for each SVG string.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Version: unspecified → Trunk
I made the changes outlined in the bug description.

It feels like the functions want to live in nsSVGElement and take the attrEnum as a parameter. They are all taking an element pointer now. The DOM class would then store a pointer to the element and the attrEnum. This would be similar to my SVGClassValue behavior. What do you think?
Attachment #354847 - Flags: review?(longsonr)
I think it's worth keeping nsSVGString here; we definitely need a class, since some elements want more than one animatable string attribute (and others don't want one at all). So having the methods in the class makes sense to me. With the class value we didn't really need a class at all.

Now I see that getting rid of the nsString mBaseVal means we have to copy the string to return it from GetBaseValue. That is annoying, but this still seems a bit better to me overall. What do you guys think?
The class only contains 1 variable now. I was thinking we could put an array of nsautoptr in the elements that need strings. We already get a pointer to the array in the element when we call getStringInfo, but we don't use it. So if we moved the methods to SVGElement we would still able to get everything we need.
We'd have to pass the attribute index around instead of using an nsSVGString pointer or reference. I think it's better the way it is.
Comment on attachment 354847 [details] [diff] [review]
Patch to make the proposed changes

General. I think it would be better to get rid of DidChangeString altogether and override AfterSetAttr instead in order to do the specialised string processing we need. One virtual function less that way.

>+  friend class nsSVGString;
>+

Best make whatever functions nsSVGString needs public instead.
Attachment #354847 - Flags: review?(longsonr) → review-
Assignee: nobody → craig.topper
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Addresses Robert's comments (obsolete) — Splinter Review
Attachment #354847 - Attachment is obsolete: true
Attachment #355250 - Flags: review?(longsonr)
Comment on attachment 355250 [details] [diff] [review]
Addresses Robert's comments

I know I said override AfterSetAttr but now that I think about it I think that's not quite right. We need our own SVG AfterSetAttr rather than hooking onto the generic element one since when animation comes along animated values may change independently of the attributes. I think we should have a DidChangeAttribute method with a similar signature to AfterSetAttr that we can override with the base implementation in nsSVGElement. What we're trying to get to is to move from lots of DidChangeXXX implementations to a single DidChangeAttribute(namespace, attribute) implementation so that we have one rather than have half a dozen virtual functions in nsSVGElement. I can only apologise for making you redo this yet again.

> void nsSVGElement::StringAttributesInfo::Reset(PRUint8 aAttrEnum)
> {
>   mStrings[aAttrEnum].Init(aAttrEnum);

Define a Reset method on the nsSVGString class (see below)

> void
> nsSVGString::SetBaseValue(const nsAString& aValue,
>                           nsSVGElement *aSVGElement,
>                           PRBool aDoSetAttr)
> {
>-  mAnimVal = mBaseVal = aValue;
>-  aSVGElement->DidChangeString(mAttrEnum, aDoSetAttr);
>+  NS_ASSERTION(aSVGElement, "Null element passed to SetBaseValue");
>+
>+  mAnimVal = nsnull;
>+
>+  if (aDoSetAttr) {
>+    nsSVGElement::StringAttributesInfo info = aSVGElement->GetStringInfo();
>+
>+    NS_ASSERTION(info.mStringCount > 0,
>+                 "SetBaseValue on element with no string attribs");
>+
>+    NS_ASSERTION(mAttrEnum < info.mStringCount, "aAttrEnum out of range");
>+
>+    aSVGElement->SetAttr(info.mStringInfo[mAttrEnum].mNamespaceID,
>+                         *info.mStringInfo[mAttrEnum].mName,
>+                         aValue, PR_TRUE);
>+  }
>+}

I'm not sure we gain anything by moving this code, although I suppose I could live it. We have to make far more of the implementation in nsSVGElement public. Perhaps we need a non-virtual UpdateStringAttribute method in nsSVGElement that does the SetAttr.

>+
>+void
>+nsSVGString::GetBaseValue(nsAString& aResult, const nsSVGElement* aSVGElement) const
>+{
>+  NS_ASSERTION(aSVGElement, "Null element passed to GetBaseValue");
>+
>+  nsSVGElement::StringAttributesInfo info = const_cast<nsSVGElement*>(aSVGElement)->GetStringInfo();

If you need a const_cast then maybe its better not to have the function take a const pointer in the first place.

> 
> class nsSVGString
> {
> 
> public:
>   void Init(PRUint8 aAttrEnum) {
>-    mAnimVal.Truncate();
>-    mBaseVal.Truncate();
>+    mAnimVal = nsnull;
>     mAttrEnum = aAttrEnum;
>   }

I think it might be simpler to have a Reset method i.e.

   void Init(PRUint8 aAttrEnum) {
     mAttrEnum = aAttrEnum;
     Reset();
   }

   void Reset() {
     mBaseVal.Truncate();
     mAnimVal = nsnull;
   }

Then UnsetAttr can call Reset rather than Init.

> nsIContent*
> nsSVGUseElement::CreateAnonymousContent()
> {
> #ifdef DEBUG_tor
>-  const nsString &href = mStringAttributes[HREF].GetAnimValue();
>+  const nsString &href = mStringAttributes[HREF].GetAnimValue(this);

I don't think this will compile for poor old tor? You need a nsAutoString here like you do elsewhere.
Attachment #355250 - Flags: review?(longsonr) → review-
> > void
> > nsSVGString::SetBaseValue(const nsAString& aValue,
> >                           nsSVGElement *aSVGElement,
> >                           PRBool aDoSetAttr)
> > {
> >-  mAnimVal = mBaseVal = aValue;
> >-  aSVGElement->DidChangeString(mAttrEnum, aDoSetAttr);
> >+  NS_ASSERTION(aSVGElement, "Null element passed to SetBaseValue");
> >+
> >+  mAnimVal = nsnull;
> >+
> >+  if (aDoSetAttr) {
> >+    nsSVGElement::StringAttributesInfo info = aSVGElement->GetStringInfo();
> >+
> >+    NS_ASSERTION(info.mStringCount > 0,
> >+                 "SetBaseValue on element with no string attribs");
> >+
> >+    NS_ASSERTION(mAttrEnum < info.mStringCount, "aAttrEnum out of range");
> >+
> >+    aSVGElement->SetAttr(info.mStringInfo[mAttrEnum].mNamespaceID,
> >+                         *info.mStringInfo[mAttrEnum].mName,
> >+                         aValue, PR_TRUE);
> >+  }
> >+}
> 
> I'm not sure we gain anything by moving this code, although I suppose I could
> live it. We have to make far more of the implementation in nsSVGElement public.
> Perhaps we need a non-virtual UpdateStringAttribute method in nsSVGElement that
> does the SetAttr.
> 

I can do that if you want.

> >+
> >+void
> >+nsSVGString::GetBaseValue(nsAString& aResult, const nsSVGElement* aSVGElement) const
> >+{
> >+  NS_ASSERTION(aSVGElement, "Null element passed to GetBaseValue");
> >+
> >+  nsSVGElement::StringAttributesInfo info = const_cast<nsSVGElement*>(aSVGElement)->GetStringInfo();
> 
> If you need a const_cast then maybe its better not to have the function take a
> const pointer in the first place.
> 

I don't like the const_cast either, but at least one method that called getBaseValue/getAnimValue was declared const. So I either had to change that method and deal with possible further ripple or use a const_cast. Figured the const on the parameter list at least told the truth, this routine won't alter the element.

> > 
> > class nsSVGString
> > {
> > 
> > public:
> >   void Init(PRUint8 aAttrEnum) {
> >-    mAnimVal.Truncate();
> >-    mBaseVal.Truncate();
> >+    mAnimVal = nsnull;
> >     mAttrEnum = aAttrEnum;
> >   }
> 
> I think it might be simpler to have a Reset method i.e.
> 
>    void Init(PRUint8 aAttrEnum) {
>      mAttrEnum = aAttrEnum;
>      Reset();
>    }
> 
>    void Reset() {
>      mBaseVal.Truncate();
>      mAnimVal = nsnull;
>    }
> 
> Then UnsetAttr can call Reset rather than Init.

There is no mBaseVal anymore so Reset would be

   void Reset() {
     mAnimVal = nsnull;
   }
 
> > nsIContent*
> > nsSVGUseElement::CreateAnonymousContent()
> > {
> > #ifdef DEBUG_tor
> >-  const nsString &href = mStringAttributes[HREF].GetAnimValue();
> >+  const nsString &href = mStringAttributes[HREF].GetAnimValue(this);
> 
> I don't think this will compile for poor old tor? You need a nsAutoString here
> like you do elsewhere.

Oops I'll fix.
> I can do that if you want.

Please do, ta.

> I don't like the const_cast either, but at least one method that called
> getBaseValue/getAnimValue was declared const. So I either had to change that
> method and deal with possible further ripple or use a const_cast. Figured the
> const on the parameter list at least told the truth, this routine won't alter
> the element.

OK, leave it as it is then.

> There is no mBaseVal anymore so Reset would be
> 
>    void Reset() {
>      mAnimVal = nsnull;
>    }

OK.
I'm a little confused by your reset method proposal. nsSVGString::Init is called by StringAttributesInfo::Reset. StringAttributesInfo::Reset is called by nsSVGElement::Init and by UnsetAttr. The nsSVGElement::Init version still needs to pass the attribute number into the Init routine. Are you proposing removing StringAttributesInfo::Reset and calling the appropriate method in the two places?
Seems there are already some other overrides of AfterSetAttr that deal xlink:href which are what the two DidChangeStrings I removed dealt with. Perhaps we should leave AfterSetAttr in my patch and deal with changing them all in a separate bug?
(In reply to comment #10)
> I'm a little confused by your reset method proposal. nsSVGString::Init is
> called by StringAttributesInfo::Reset. StringAttributesInfo::Reset is called by
> nsSVGElement::Init and by UnsetAttr. The nsSVGElement::Init version still needs
> to pass the attribute number into the Init routine. Are you proposing removing
> StringAttributesInfo::Reset and calling the appropriate method in the two
> places?

StringAttributesInfo would have to have Init and Reset methods. nsSVGElement::Init would call Init and nsSVGElement::UnsetAttr would call Reset. Perhaps this is best done as a follow up bug though as it applies to all types.
(In reply to comment #11)
> Seems there are already some other overrides of AfterSetAttr that deal
> xlink:href which are what the two DidChangeStrings I removed dealt with.
> Perhaps we should leave AfterSetAttr in my patch and deal with changing them
> all in a separate bug?

Lets just have everything as DidChangeString then and do everything else as a follow-up. I realise this is back to square one but as you say, one step at a time is best.
At least one of the places that is using AfterSetAttr is supposedly doing so because DidChangeString is too early for HasAttr to work correctly. Presumably because DidChangeString is called from UnsetAttr before the attribute is removed. That is why I suggested sticking with AfterSetAttr for now.
Leave that one as it is then, but don't change any others as we'll likely just have to change them again.
Attached patch Addresses some of the comments (obsolete) — Splinter Review
Moves most of the logic for updating and retrieving the attribute into SVGElement to avoid making too much of SVGElement public.

Changes AfterSetAttr calls back to DidChangeString.
Attachment #355250 - Attachment is obsolete: true
Attachment #356142 - Flags: review?(longsonr)
Attachment #356142 - Flags: review?(longsonr) → review+
Comment on attachment 356142 [details] [diff] [review]
Addresses some of the comments

Thanks for persevering.
Attachment #356142 - Flags: superreview?(roc)
Attachment #356142 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
Pushed fb2d014e0352. I forgot to set -u, sorry!!!
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backed out --- caused reftest failures and a crash in mochitests. Maybe I merged it badly?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232358039.1232363144.22806.gz#err0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232358039.1232361560.19822.gz#err2

The problem seems to be that nsSVGScriptElement::Clone doesn't call Init on the new element, so mStringAttributes doesn't get initialized for the clone.

I checked the other Clone methods in SVG elements and they're all OK.
Fixing that fixes the mochitest crash, but there's still reftest failures in feMerge. I guess there's another regression there to hunt down.
Attached patch updated patch (obsolete) — Splinter Review
Updated with the Clone fix. Still need to figure out the feMerge bug.
Attachment #356142 - Attachment is obsolete: true
OK, the feMerge bug is because nsSVGFilterInstance::BuildPrimitives is calling sources[j]->GetAnimValue(str, filter), passing the filter primitive element (feMerge) as the second element. But for feMerge the string actually belongs to the <feMergeNode> child element, so we're looking for the attribute value of the wrong element.

So the filter code can't keep using just nsSVGStrings. Probably we should declare a struct that's a pair of nsSVGString*, nsSVGElement* and use an array of those in the signature of GetSourceImageNames. GetResultImageName does not need to change. I hope one of you guys can take care of that...
I'm work on it.
Attached patch Fixes femerge problem (obsolete) — Splinter Review
Attachment #357726 - Attachment is obsolete: true
Attachment #357907 - Flags: review?(roc)
Comment on attachment 357907 [details] [diff] [review]
Fixes femerge problem

+  const nsSVGString& mString;

I'd prefer this to be a pointer so it's clear wherever we use it that it's not a value stored in the struct.

Also, in the final version of the patch could you include the change that marks the fixed reftest as passing?
Attachment #357907 - Flags: superreview+
Attachment #357907 - Flags: review?(roc)
Attachment #357907 - Flags: review+
(In reply to comment #26)
> Also, in the final version of the patch could you include the change that marks
> the fixed reftest as passing?

Never mind, I confused this with another bug.
Attachment #357907 - Attachment is obsolete: true
Pushed https://bugzilla.mozilla.org/show_bug.cgi?id=471551
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.