Closed Bug 470653 Opened 11 years ago Closed 11 years ago

nsSVGElement::AfterSetAttr should probably use insertionParent, not insertionParent of the bindingParent

Categories

(Core :: SVG, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: longsonr)

References

Details

Attachments

(1 file, 4 obsolete files)

.
It looks like you are right. GetInsertionParent needs to be documented though.
bug 387466 comment 39 is where this voodoo originated. This does not mean it is right in this circumstance of course.
Attached patch patch (obsolete) — Splinter Review
Existing reftests pass, but then they would as they don't use binding parents.

Not sure how to do a test specifically for this.
Assignee: nobody → longsonr
Attachment #358480 - Flags: superreview?(Olli.Pettay)
Attachment #358480 - Flags: review?(Olli.Pettay)
Comment on attachment 358480 [details] [diff] [review]
patch

>+      nsBindingManager *bindingManager = ownerDoc->BindingManager();
>+      if (bindingManager) {
>+        parent = bindingManager->GetInsertionParent(this);
BindingManager() never returns null, so just call
parent = ownerDoc->BindingManager()->GetInsertionParent(this);

I think the testcase for this all could be something where
<svg:switch> is inside a binding and under it is the <xbl:children> element.
Then the binding is added to some svg element which has child elements, and
one of the relevant attributes in some of those child elements is changed by a script.
Attachment #358480 - Flags: superreview?(roc)
Attachment #358480 - Flags: superreview?(Olli.Pettay)
Attachment #358480 - Flags: review?(Olli.Pettay)
Attachment #358480 - Flags: review+
Attachment #358480 - Flags: superreview?(roc) → superreview?(bzbarsky)
Attached patch address review comment (obsolete) — Splinter Review
Attachment #358480 - Attachment is obsolete: true
Attachment #358593 - Flags: superreview?(bzbarsky)
Attachment #358480 - Flags: superreview?(bzbarsky)
How about factoring out this code to get the parent?  It's also there in nsSVGElement::GetOwnerSVGElement except that one sometimes uses the wrong binding manager as far as I can tell.

And yeah, a test would be good.
Comment on attachment 358593 [details] [diff] [review]
address review comment

sr- pending response
Attachment #358593 - Flags: superreview?(bzbarsky) → superreview-
I'll work on a more extensive patch.
Attached patch address sr comment (obsolete) — Splinter Review
The patch for bug 361920 fixed the issue in this bug's title. This patch merely rationalises all the GetParent implementations.
Attachment #358593 - Attachment is obsolete: true
Attachment #363742 - Flags: superreview?(bzbarsky)
Attachment #363742 - Flags: review?(bzbarsky)
Comment on attachment 363742 [details] [diff] [review]
address sr comment

This looks wrong to me in terms of the binding managers it uses.  I'd think that GetParentElement would just take a node and get the relevant binding manager from it...  That's not what this code does, though.  Why not?
Attachment #363742 - Flags: superreview?(bzbarsky)
Attachment #363742 - Flags: superreview-
Attachment #363742 - Flags: review?(bzbarsky)
Attachment #363742 - Flags: review-
Attached patch patch (obsolete) — Splinter Review
Attachment #363742 - Attachment is obsolete: true
Attachment #364265 - Flags: review?(bzbarsky)
It'd be good if someone familiar with the SVG code reviewed this, since there are some nontrivial logic changes here that I can't review reasonably (including various casting stuff, etc).
I was planning to get roc to sr it.
He can probably just do both reviews, then, unless you think I really need to be one of the reviewers....
As long as you're happy with the logic ragarding the correct binding parents.
Attachment #364265 - Flags: superreview?(roc)
Attachment #364265 - Flags: review?(roc)
Yeah, that part looks fine (though I'm not sure why we need the strong nsIContent ref in GetParentElement).
Just the way it was originally written. I can fix it before check in.
Attachment #364265 - Flags: superreview?(roc)
Attachment #364265 - Flags: review?(roc)
Attachment #364265 - Flags: review?(bzbarsky)
Comment on attachment 364265 [details] [diff] [review]
patch

going to need updating when bug 361920 is backed out.
Attached patch patchSplinter Review
Attachment #364265 - Attachment is obsolete: true
Attachment #366174 - Flags: superreview?(roc)
Attachment #366174 - Flags: review?(roc)
Attachment #366174 - Flags: superreview?(roc)
Attachment #366174 - Flags: superreview+
Attachment #366174 - Flags: review?(roc)
Attachment #366174 - Flags: review+
checked in http://hg.mozilla.org/mozilla-central/rev/f8c83e876d37
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.