Last Comment Bug 691298 - Remove aDoSetAttr arguments where callers always pass the same value
: Remove aDoSetAttr arguments where callers always pass the same value
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-03 05:28 PDT by Robert Longson
Modified: 2011-10-10 11:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (36.65 KB, patch)
2011-10-03 05:29 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description Robert Longson 2011-10-03 05:28:07 PDT

    
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 10 Matt Brubeck (:mbrubeck) 2011-10-10 11:10:55 PDT
https://hg.mozilla.org/mozilla-central/rev/5eed5a82852c

Note You need to log in before you can comment on or make changes to this bug.