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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

.

Comment 1

[Jonathan Watt [:jwatt]]

It looks like you are right. GetInsertionParent needs to be documented though.

Comment 2

[Robert Longson [:longsonr]]

bug 387466 comment 39 is where this voodoo originated. This does not mean it is right in this circumstance of course.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Existing reftests pass, but then they would as they don't use binding parents.

Not sure how to do a test specifically for this.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 6

[Boris Zbarsky [: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 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 358593 [details] [diff] [review]
address review comment

sr- pending response

Comment 8

[Robert Longson [:longsonr]]

I'll work on a more extensive patch.

Comment 9

[Robert Longson [:longsonr]]

Attachment: address sr comment

The patch for bug 361920 fixed the issue in this bug's title. This patch merely rationalises all the GetParent implementations.

Comment 10

[Boris Zbarsky [: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?

Comment 11

[Robert Longson [:longsonr]]

Attachment: patch

Comment 12

[Boris Zbarsky [: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).

Comment 13

[Robert Longson [:longsonr]]

I was planning to get roc to sr it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

He can probably just do both reviews, then, unless you think I really need to be one of the reviewers....

Comment 15

[Robert Longson [:longsonr]]

As long as you're happy with the logic ragarding the correct binding parents.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, that part looks fine (though I'm not sure why we need the strong nsIContent ref in GetParentElement).

Comment 17

[Robert Longson [:longsonr]]

Just the way it was originally written. I can fix it before check in.

Comment 18

[Robert Longson [:longsonr]]

Comment on attachment 364265 [details] [diff] [review]
patch

going to need updating when bug 361920 is backed out.

Comment 19

[Robert Longson [:longsonr]]

Attachment: patch

Comment 20

[Robert Longson [:longsonr]]

checked in http://hg.mozilla.org/mozilla-central/rev/f8c83e876d37