Closed
Bug 691298
Opened 10 years ago
Closed 10 years ago
Remove aDoSetAttr arguments where callers always pass the same value
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: longsonr, Assigned: longsonr)
Details
Attachments
(1 file)
36.65 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → longsonr
Attachment #564161 -
Flags: review?(dholbert)
Comment 2•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 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)
Comment 8•10 years ago
|
||
Makes sense, thanks for the explanation. r=me
Assignee | ||
Comment 9•10 years ago
|
||
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5eed5a82852c
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5eed5a82852c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in
before you can comment on or make changes to this bug.
Description
•