Comment 1 Robert Longson 2011-10-03 05:29:11 PDT

Created attachment 564161 [details] [diff] [review]
patch

Comment 2 Daniel Holbert [:dholbert] 2011-10-03 12:43:11 PDT

Comment on attachment 564161 [details] [diff] [review]
patch

>+++ b/content/svg/content/src/nsSVGIntegerPair.cpp
> void
> nsSVGIntegerPair::SetBaseValues(PRInt32 aValue1, PRInt32 aValue2,
>-                                nsSVGElement *aSVGElement,
>-                                bool aDoSetAttr)
>+                                nsSVGElement *aSVGElement)
[...]
>-  aSVGElement->DidChangeIntegerPair(mAttrEnum, aDoSetAttr);
>+  aSVGElement->DidChangeIntegerPair(mAttrEnum, true);
> }

It looks like the above chunk might need s/true/false/, per this caller from higher up in the patch:

>+++ b/content/svg/content/src/nsSVGFilterElement.cpp
> NS_IMETHODIMP
> nsSVGFilterElement::SetFilterRes(PRUint32 filterResX, PRUint32 filterResY)
> {
>-  mIntegerPairAttributes[FILTERRES].SetBaseValues(filterResX, filterResY, this, PR_FALSE);
>+  mIntegerPairAttributes[FILTERRES].SetBaseValues(filterResX, filterResY, this);

right?

Comment 3 Daniel Holbert [:dholbert] 2011-10-03 12:47:38 PDT

(Or maybe that PR_FALSE is a typo in existing code? I don't immediately see why SetFilterRes would pass PR_FALSE but SetStdDeviation would pass PR_TRUE.)

Comment 4 Daniel Holbert [:dholbert] 2011-10-03 12:58:48 PDT

It might also be cleaner to have this split into 2 patches -- one that simply removes the *completely* unused 'aDoSetAttr' parameters (which seem to mostly be in "SetBaseValueString"), and one that substitutes in the always-the-same-value ones.

I think I'd prefer that (makes the patch easier to read / grok), but I won't insist on it if it's too much work. :)

(random observation: it looks like the "mIntegerPairAttributes[FILTERRES].SetBaseValues()" call from comment 2 is the only line in this patch where we pass in a PR_FALSE value that actually gets used.  All the other PR_FALSE passed-in-values are for SetBaseValueString() methods that ignore their aDoSetAttr param.)

Comment 5 Daniel Holbert [:dholbert] 2011-10-03 13:00:12 PDT

Comment on attachment 564161 [details] [diff] [review]
patch

r=me, with comment 2 addressed / responded-to.

Comment 6 Robert Longson 2011-10-03 13:55:14 PDT

(In reply to Daniel Holbert [:dholbert] from comment #4)
> "mIntegerPairAttributes[FILTERRES].SetBaseValues()" call from comment 2 is
> the only line in this patch where we pass in a PR_FALSE value that actually
> gets used.  All the other PR_FALSE passed-in-values are for
> SetBaseValueString() methods that ignore their aDoSetAttr param.)

That's a bug. The caller should have passed true. mIntegerPairAttributes[FILTERRES].SetBaseValues() is a DOM call - javascript calls it. If we pass false then there's no refresh as aSVGElement->DidChangeIntegerPair(mAttrEnum, false) will just return without doing anything, it should have been called with true. So it was fixed in passing. I should have pointed that out.

Comment 7 Robert Longson 2011-10-03 13:57:34 PDT

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 564161 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >+++ b/content/svg/content/src/nsSVGIntegerPair.cpp
> > void
> > nsSVGIntegerPair::SetBaseValues(PRInt32 aValue1, PRInt32 aValue2,
> >-                                nsSVGElement *aSVGElement,
> >-                                bool aDoSetAttr)
> >+                                nsSVGElement *aSVGElement)
> [...]
> >-  aSVGElement->DidChangeIntegerPair(mAttrEnum, aDoSetAttr);
> >+  aSVGElement->DidChangeIntegerPair(mAttrEnum, true);

true is correct. We need DidChange to do something here.

SetBaseValueString should become false generally (as it's only called from ParseAttribute and that does an AttributeChanged so you get an update anyway)
SetBaseValue should become true generally (as it's called from DOM and you won't get a graphics refresh with false as you won't get an AttributeChanged otherwise)

Comment 8 Daniel Holbert [:dholbert] 2011-10-03 22:21:02 PDT

Makes sense, thanks for the explanation.  r=me

Comment 9 Robert Longson 2011-10-09 08:27:06 PDT

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5eed5a82852c

Comment 10 Matt Brubeck (:mbrubeck) 2011-10-10 11:10:55 PDT

https://hg.mozilla.org/mozilla-central/rev/5eed5a82852c