1410895 - Make XBL know about its insertion point, not insertion point parent.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

That way we can know which insertion point to look up for distributed siblings.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1410895: Make XBL slots hold the insertion point, not the XBL parent.

This is pretty much a straight-forward change except for a single thing, the
UpdateInsertionParent calls.

However, I cannot make any sense of them. They go through the inserted children
setting the insertion point siblings, but then ClearInsertionPoints() is called.

ClearInsertionPoints calls XBLChildrenElement::ClearInsertedChildren, which sets
all the insertion points to null anyway.

Thus, I've removed that function completely.

Review commit: https://reviewboard.mozilla.org/r/192026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192026/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1410895: Multiple cleanups on top.

Review commit: https://reviewboard.mozilla.org/r/192036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192036/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8921059 [details]
Bug 1410895: Make XBL slots hold the insertion point, not the XBL parent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192026/diff/1-2/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8921059 [details]
Bug 1410895: Make XBL slots hold the insertion point, not the XBL parent.

https://reviewboard.mozilla.org/r/192026/#review197396

::: dom/xbl/nsBindingManager.cpp:222
(Diff revision 2)
>      }
>  
>      aContent->SetXBLBinding(nullptr, this);
>    }
>  
>    // Clear out insertion parent and content lists.

s/parent/point/

::: dom/xbl/nsXBLBinding.cpp:804
(Diff revision 2)
> -      UpdateInsertionParent(mInsertionPoints[i], mBoundElement);
> -    }
> -
> -    // Now that our inserted children no longer think they're inserted
> -    // anywhere, make sure our internal state reflects that as well.
>      ClearInsertionPoints();

It looks like when this code was added (<https://hg.mozilla.org/mozilla-central/rev/731a5fcf13289e02bbd86f06950a3b93c7357e12>) calling ClearInsertionPoints() just cleared the child arrays on the insertion points and did not update the insertion point on the elements inside those insertion points.  Which is why this code had to do the updating.

It also looks like <https://hg.mozilla.org/mozilla-central/rev/a98e26325e28> changed the behavior to have XBLChildrenElement::ClearInsertedChildren clear out the insertion parent unconditionally on the kids.  It did NOT use the same logic as UpdateInsertionParent, though: it just always set to null instead of sometimes setting to the bound element itself.  After this, the UpdateInsertionParent calls are in fact redundant...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Blake, do you know what the story was with the changes I mention in comment 4?  Why do we not need to preserve the "correct" insertion parent?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8921066 [details]
Bug 1410895: Multiple cleanups on top.

https://reviewboard.mozilla.org/r/192036/#review198174

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8921059 [details]
Bug 1410895: Make XBL slots hold the insertion point, not the XBL parent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192026/diff/2-3/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8921066 [details]
Bug 1410895: Multiple cleanups on top.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192036/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ea2429943c2
Make XBL slots hold the insertion point, not the XBL parent. r=bz
https://hg.mozilla.org/integration/autoland/rev/89e379338c15
Multiple cleanups on top. r=bz

Comment 10

[Attila Craciun [:arny]]

https://hg.mozilla.org/mozilla-central/rev/1ea2429943c2
https://hg.mozilla.org/mozilla-central/rev/89e379338c15

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I can't remember why that code was there. I've done some spelunking in the history but nothing jumped out at me. Sorry.