Remove aDoSetAttr arguments where callers always pass the same value

RESOLVED FIXED in mozilla10

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Robert Longson, Assigned: Robert Longson)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 564161 [details] [diff] [review]
patch
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+
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 9

6 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5eed5a82852c
https://hg.mozilla.org/mozilla-central/rev/5eed5a82852c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.