Closed Bug 691298 Opened 8 years ago Closed 8 years ago

Remove aDoSetAttr arguments where callers always pass the same value

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #564161 - Flags: review?(dholbert)
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?
(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.)
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 on attachment 564161 [details] [diff] [review]
patch

r=me, with comment 2 addressed / responded-to.
Attachment #564161 - Flags: review?(dholbert) → review+
(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.
(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)
Makes sense, thanks for the explanation.  r=me
https://hg.mozilla.org/mozilla-central/rev/5eed5a82852c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.