653881 - Fix up our XBL1 shadow tree implementation so that we can reuse it for XBL2

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The current code for dealing with things like insertion points, attribute forwarding and content lists is incredibly complex. We can do better.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP

Attaching for safe keeping. The browser starts, but things like the options window and about:addons have serious issues.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP2

This appears to be almost working. It causes some assertions and it breaks a11y. I'm fairly sure that a11y breaks because default content in insertion points no longer have their insertionpoint-parent as parent, but rather the insertion point is the parent.

Working on fixing that.

Comment 3

[Neil Deakin]

Attachment: Updated patch, but still has some test failures

Comment 4

[Neil Deakin]

Attachment: Patch v3

This updated patch fixes all but three test failures.

content/xbl/test/test_bug378866.xhtml
 - this test moves an insertion point into an explicit child of the bound node. The parent hierarchy used when building event targets then has an infinite loop.

toolkit/themes/pinstripe/mochitests/test_bug510426.xul
 - notifications have a style rule,
     notification > button > .button-box > .button-text
   which now no longer matches as there is now a <children> element in-between

layout/generic/crashtests/382745-1.xhtml
 - ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.:
   '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)', file ../../../layout/generic/nsFrame.cpp, line 625

Comment 5

[Neil Deakin]

Attachment: Rollup patch

This patch contains:
 - the contents of the previous patch
 - removes some now unused code
 - a rolled up patch (originally of several subpatches) that implements a simpler flattened content tree iterator and uses it instead of node lists and binding manager methods.
 - fixes the hang with content/xbl/test/test_bug378866.xhtml by supporting removing insertion points from a binding
- quick hack to skip over <xbl:children> elements in child/descedant selectors. This, i assume, isn't the right fix here, but it demonstrates fixing the failing notificationbox test.

Comment 6

[Neil Deakin]

Attachment: Part 1 - rework insertion points

Comment 7

[Neil Deakin]

Attachment: Part 2 - remove nsBindingManager::GetContentListFor

Comment 8

[Neil Deakin]

Attachment: Part 3 - add an iterator to iterate over the flattened tree

Comment 9

[Neil Deakin]

Attachment: Part 4 - use the new iterator in the frame constructor

Comment 10

[Neil Deakin]

Attachment: Part 5 - Remove nsBindingManager::GetXBLChildNodesFor

Comment 11

[Neil Deakin]

Attachment: Part 6 - fix up dom inspector to use iterators

Comment 12

[Neil Deakin]

Attachment: Part 1 - rework insertion points

OK, I think we can have these reviewed, and we can fix the two remaining issues (bug 758695 and style descendant selector matching) while the review is occurring.

This part was mostly written by Jonas, but it generally changes the insertion point handling to generate an nsXBLChildrenElement class for the <xbl:children> element which keeps track of the inserted and default content.

Comment 13

[Neil Deakin]

Attachment: Part 2 - remove nsBindingManager::GetContentListFor

Comment 14

[Neil Deakin]

Attachment: Part 3 - add an iterator to iterate over the flattened tree

Comment 15

[Neil Deakin]

Attachment: Part 4 - use the new iterator in the frame constructor

I was a bit concerned the new iterator might cause a performance regression although I didn't see one when testing.

Comment 16

[Neil Deakin]

Attachment: Part 5 - Remove nsBindingManager::GetXBLChildNodesFor

Comment 17

[Neil Deakin]

Comment on attachment 634869 [details] [diff] [review]
Part 1 - rework insertion points

I want to make sure the accessibility changes are ok here.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Neil, chances are I won't get to this review this week.  Just fyi.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 634869 [details] [diff] [review]
Part 1 - rework insertion points

Review of attachment 634869 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ChildIterator.cpp
@@ +4,5 @@
> +
> +#include "ChildIterator.h"
> +#include "nsXBLChildrenElement.h"
> +
> +nsIContent* nsExplicitChildIterator::nextChild()

Newline after nsIContent*

@@ +15,5 @@
> +    }
> +    mIndexInInserted = 0;
> +    mChild = mChild->GetNextSibling();
> +  }
> +  else if (mDefaultChild) {

Cuddle else

::: content/base/src/ChildIterator.h
@@ +6,5 @@
> +
> +// Iterates over the children on a node. If a child is an insertion point,
> +// iterates over the children inserted there instead, or the default content
> +// if no children are inserted there.
> +class nsExplicitChildIterator

mozilla::dom::ExplicitChildIterator or so

@@ +18,5 @@
> +      mIsFirst(true)
> +  {
> +  }
> +  
> +  nsIContent* nextChild();

NextChild()

@@ +24,5 @@
> +protected:
> +  nsIContent* mParent;
> +  nsIContent* mChild;
> +  PRUint32 mIndexInInserted;
> +  nsIContent* mDefaultChild;

Pointers first, then the PRUint32

::: content/base/src/nsGenericElement.cpp
@@ +1458,5 @@
>  nsIContent::GetFlattenedTreeParent() const
>  {
> +  if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +    nsIContent* parent = OwnerDoc()->BindingManager()->
> +      GetInsertionParent(const_cast<nsIContent*>(this));

How about making the method not be const instead?

::: content/xbl/src/nsBindingManager.cpp
@@ +474,5 @@
>  nsresult
>  nsBindingManager::GetXBLChildNodesFor(nsIContent* aContent, nsIDOMNodeList** aResult)
>  {
> + NS_IF_ADDREF(*aResult = GetXBLChildNodesFor(aContent));
> + return NS_OK;

Indentation is off here

@@ +1152,5 @@
>                                    PRInt32 aIndexInContainer)
>  {
> +  // It's not anonymous.
> +  NS_ASSERTION(aIndexInContainer >= 0, "Bogus index");
> +  HandleChildInsertion(aContainer, aChild, aIndexInContainer, PR_FALSE);

false

@@ +1202,5 @@
> +    static_cast<nsXBLChildrenElement*>(aContent)->ClearInsertedChildrenAndInsertionParents(this);
> +  }
> +
> +  PRInt32 childCount = aContent->GetChildCount();
> +  for (PRInt32 c = 0; c < childCount; c++) {

Unsigned ints, please

::: content/xbl/src/nsBindingManager.h
@@ +154,5 @@
>  
> +  nsIContent* FindNestedInsertionPoint(nsIContent* aContainer,
> +                                       nsIContent* aChild);
> +
> +  nsIContent* FindNestedSingleInsertionPoint(nsIContent* aContainer, bool& aMulti);

bool*, not bool&, please

::: content/xbl/src/nsXBLBinding.cpp
@@ +282,5 @@
> +nsXBLBinding*
> +nsXBLBinding::GetBindingWithContent()
> +{
> +  if (mContent)
> +    return this;

{}

@@ +402,5 @@
>      nsBindingManager *bindingManager = doc->BindingManager();
>  
> +    nsCOMPtr<nsIDOMNode> clonedNode;
> +    nsCOMArray<nsINode> nodesWithProperties;
> +    nsNodeUtils::Clone(content, PR_TRUE, doc->NodeInfoManager(),

true

@@ +429,5 @@
> +      for (nsIContent* child = iter.nextChild(); child; child = iter.nextChild()) {
> +        mDefaultInsertionPoint->AppendInsertedChild(child, bindingManager);
> +      }
> +    }
> +    else if (!mInsertionPoints.IsEmpty()) {

Cuddle

::: content/xbl/src/nsXBLChildrenElement.cpp
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL2

@@ +152,5 @@
> +{
> +  nsINode* item = GetNodeAt(aIndex);
> +  if (!item) {
> +    //*aReturn = nsnull;
> +    return NS_ERROR_FAILURE;

Uncomment, please

::: content/xbl/src/nsXBLChildrenElement.h
@@ +99,5 @@
> +  }
> +
> +  void RemoveInsertedChild(nsIContent* aChild)
> +  {
> +    // Can't use this assertion as we cheat for dunamic insertions and

dynamic

::: content/xbl/src/nsXBLPrototypeBinding.cpp
@@ +577,5 @@
>      return nsnull;
> +
> +  nsIContent *copyParent =
> +    templParent == aTemplRoot ? aCopyRoot :
> +                   LocateInstance(aBoundElement, aTemplRoot, aCopyRoot, templParent);

Indentation is weird

::: layout/base/nsCSSFrameConstructor.cpp
@@ +6858,5 @@
> + 
> +   // Otherwise, we've got parent content. Find its frame.
> + #ifdef DEBUG
> +  NS_ASSERTION(!parentFrame || parentFrame->GetContent() == aContainer, "New XBL code is possibly wrong!");
> + #endif

No point in the #ifdef

@@ +8786,5 @@
>      bool multiple;
> +    insertionElement = bindingManager->FindNestedSingleInsertionPoint(aContainer, multiple);
> +    
> +    if (multiple) {
> +      *aMultiple = PR_TRUE;

true

@@ +8793,5 @@
> +  }
> +
> +  if (!insertionElement) {
> +    insertionElement = aContainer;
> +    //return nsnull;

Hmm?

@@ +8799,5 @@
> +
> +  nsIFrame* insertionPoint = GetFrameFor(insertionElement);
> +
> +  if (aMultiple && insertionElement->IsHTML(nsGkAtoms::fieldset)) {
> +    *aMultiple = PR_TRUE;

true

::: layout/style/nsCSSRuleProcessor.cpp
@@ +3278,5 @@
>  AncestorFilter::AssertHasAllAncestors(Element *aElement) const
>  {
>    nsINode* cur = aElement->GetNodeParent();
>    while (cur && cur->IsElement()) {
> +//    MOZ_ASSERT(mElements.Contains(cur));

Hmm?

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 634871 [details] [diff] [review]
Part 3 - add an iterator to iterate over the flattened tree

Review of attachment 634871 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ChildIterator.h
@@ +34,5 @@
> +// content. Otherwise, iterate over the explicit children.
> +class FlattenedChildIterator : public nsExplicitChildIterator
> +{
> +public:
> +  FlattenedChildIterator(nsIContent* aParent);

explicit

::: layout/xul/base/src/tree/src/Makefile.in
@@ +33,5 @@
>  
>  LOCAL_INCLUDES	= \
>  		-I$(srcdir) \
>  		-I$(srcdir)/../../../../../../content/events/src \
> +		-I$(srcdir)/../../../../../../content/base/src \

Why's that?

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 634873 [details] [diff] [review]
Part 4 - use the new iterator in the frame constructor

Review of attachment 634873 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ChildIterator.h
@@ +36,5 @@
>  {
>  public:
>    FlattenedChildIterator(nsIContent* aParent);
> +
> +  nsIContent* get();

Get(), etc

Comment 22

[Neil Deakin]

> ::: layout/style/nsCSSRuleProcessor.cpp
> @@ +3278,5 @@
>  AncestorFilter::AssertHasAllAncestors(Element *aElement) const
>  {
>    nsINode* cur = aElement->GetNodeParent();
>    while (cur && cur->IsElement()) {
> +//    MOZ_ASSERT(mElements.Contains(cur));

You can ignore this change when reviewing. I removed this assertion to investigate the style descendant selector matching issue described above (causing test_bug510426.xul to fail).

Comment 23

[alexander :surkov (:asurkov)]

It's not trivial to review because XBL usage is spread through a11y code, I bet mochitests don't cover all cases, thus we should be careful about regressions. The safe way is to use GetFlattenedTreeParent() whenever GetParent() was used but it sounds as overkill. How did you find those cases you changed in the patch?

Comment 24

[Neil Deakin]

Jonas made those changes so that would be a question for him. I suspect he just fixed them as he found test failures.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here...  I really need to find several days in a row when I can sit down and review this.  :(

I might get to parts of it next week, but note that I'm going to be on vacation Aug 11 - 20 or so.

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 634869 [details] [diff] [review]
Part 1 - rework insertion points

Review of attachment 634869 [details] [diff] [review]:
-----------------------------------------------------------------

So XBL2 is something that is far away (if not in implementation then in usage in the wild). GetFlattenedTreeParent() is basically slower than GetParent(), thus using it everywhere might be not reasonable. So I'm fine with the patch if doesn't break XUL a11y.

Neil, since you know toolkit better than me then can I ask you check several things.

if those GetBindingParent()/GetParent() (and below) are still ok: http://mxr.mozilla.org/mozilla-central/source/accessible/src/generic/Accessible.cpp#1120

DocAccessible::RecreateAccessible should be fixed to stay on the safe side.

please check XULListboxAccessible constructor, it works under assumption that xul:listbox has a nsIAutoCompletePopup parent (and other places in this file).

it seems XULMenupopupAccessible::GetNameInternal should be fixed as well to keep the code equivalent (I assume menupopup can be anonymous child of some XUL widget).

I assume there's no tabs and tabpanels widgets that contain tab and tabpanel elements as anonymous children, otherwise XULTabAccessible.cpp should be fixed as well

XULTreeAccessible is similar to XULListboxAccessible issue from above

::: accessible/src/base/nsCoreUtils.cpp
@@ +192,5 @@
>    if (aContent->IsElement())
>      return aContent;
>  
>    if (aContent->IsNodeOfType(nsINode::eTEXT))
> +    return aContent->GetFlattenedTreeParent();

I don't really have an idea what mochitest it breaks but if you do this then you should fix nsTextEquivUtils::AppendTextEquivFromTextContent

::: accessible/src/html/HTMLTableAccessible.cpp
@@ +370,5 @@
>    }
>  
>    // Assume it's columnheader if there are headers in siblings, oterwise
>    // rowheader.
> +  // This should iterate the flattened tree

Why?

::: accessible/src/xul/XULElementAccessibles.cpp
@@ +60,5 @@
>  {
>    Relation rel = HyperTextAccessibleWrap::RelationByType(aType);
>    if (aType == nsIAccessibleRelation::RELATION_LABEL_FOR) {
>      // Caption is the label for groupbox
> +    nsIContent *parent = mContent->GetFlattenedTreeParent();

nit: type* name

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Neil, would you be able to rebase your patches? Or better yet, can we get this patch into a user repo?

Comment 28

[Neil Deakin]

Attachment: Part 1.2 - rework insertion points

Comment 29

[Neil Deakin]

Attachment: Part 2.2 - remove nsBindingManager::GetContentListFor

Comment 30

[Neil Deakin]

Attachment: Part 3.2 - add an iterator to iterate over the flattened tree

Comment 31

[Neil Deakin]

Attachment: Part 4.2 - use the new iterator in the frame constructor

Comment 32

[Neil Deakin]

Attachment: Part 5.2 - Remove nsBindingManager::GetXBLChildNodesFor

Comment 33

[Neil Deakin]

Attachment: Part 6.2 - fix up dom inspector to use iterators

Comment 34

[William Chen [:wchen]]

There have been some concerns about the performance impact of these patches. I ran tpaint in talos locally on my machine with release build configuration and I see a regression of approximately 2%.

Comment 35

[William Chen [:wchen]]

Here is a patch repo with the patches rebased to the latest m-c
http://hg.mozilla.org/users/wchen_mozilla.com/xblrefactor
And here is the try run:
https://tbpl.mozilla.org/?tree=Try&rev=0b51053b75fc

Comment 36

[William Chen [:wchen]]

https://tbpl.mozilla.org/?tree=Try&rev=e6147ae40898

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 updated to tip

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to tip

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to tip

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 updated to tip

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 updated to tip

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 updated to tip

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703989 [details] [diff] [review]
Part 1 updated to tip

The changes to /accessible/ could use review from someone who knows that code.

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Additional patch needed while debugging

This fixes a bug introduced in the patches here that bit me while I was debugging the try server failures. We should make sure it makes it in with the rest of the patches.

Comment 45

[Neil Deakin]

The try server failures that remain are described by the last two points of comment 4. The latter is convered by bug 758695 which has lots of details. The former requires a change to the selector matching handling.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703989 [details] [diff] [review]
Part 1 updated to tip

I'm sorry for the lag here; this patch was ... hard to review.  :(

>Bug 653881 - Part 1: Rework insertion points. r=bz

This needs a ... much more extensive checkin comment.  A decent description of what the new setup is should go in here, for example.  It sure would have made this easier to review.  :(

The changes to accessible/ may want a quick once-over from an a11y peer.

>+++ b/content/base/src/ChildIterator.cpp

>+nsIContent* nsExplicitChildIterator::nextChild()
>+  if (mIndexInInserted) {

MOZ_ASSERT(mChild) and perhaps its namespace and localname here please?

>+  else if (mDefaultChild) {

Likewise.

>+++ b/content/base/src/ChildIterator.h

Please put the comment describing what this class does in /** ... */ and before all #includes so mxr will pick it up as a doc comment for this file?

>+class nsExplicitChildIterator

The members here need some serious documentation.  For example, should document that mIndexInInserted is nonzero only when mChild is an nsXBLChildrenElement and so forth.

It's also worth documenting exactly what ths class iterates: the normal DOM kids of the element, with <children> nodes replaced by the elements that have been filtered into that insertion point, if any bindings on the given element are ignored.

>+++ b/content/base/src/Element.cpp
>@@ -2218,36 +2144,34 @@ Element::List(FILE* out, int32_t aIndent
>-  if (anonymousChildren) {

This looks wrong to me.  GetAnonymousNodesFor can still return null, so we need a null-check here.

>+++ b/content/base/src/FragmentOrElement.cpp
>+    nsIContent* parent = OwnerDoc()->BindingManager()->
>+    GetInsertionParent(const_cast<nsIContent*>(this));

Please fix the indentation.

This subtly changes behavior in case "this" and parent can ever have different OwnerDoc(), but the chances of that ever happening seem slim to non, so that's probably OK.

>+    if (parent) {
>+      return parent;

Hmm.  So this returns the DOM parent for elements which are not filtered into any insertion point, right?

That's probably OK...

>+++ b/content/base/src/nsReferencedElement.cpp
>-        if (anonymousChildren) {

Again, why is this change ok?  Is it because the assumption is that the return value of GetBindingParent() will always have an XBL binding?  I think that's not true, given native anonymous content.

>+++ b/content/xbl/src/nsBindingManager.cpp
> nsBindingManager::SetBinding(nsIContent* aContent, nsXBLBinding* aBinding)
>+      // Update binding parent for all explicit children

The binding parent, or the insertion parent?

>+        if (child->GetParentNode() == aContent) {
>+          SetInsertionParent(child, nullptr);
>+        }
>+        else {
>+          SetInsertionParent(child, aContent);
>+        }

Why?  This needs at least comments explaining what it's trying to do, because it's not making any sense to me.  Presumably we're distinguishing between content that's direct kids of the <content> and content that's default content in <children> elements in there (which itself can use a comment), but why do we want to set the insertion parent to ourselves on the latter?

> nsBindingManager::GetContentListFor(nsIContent* aContent)
>+    return aContent->GetChildNodesList();

aContent->ChildNodes(), right?

>+  return contentList ? contentList : aContent->GetChildNodesList();

And here.

> nsBindingManager::ContentAppended(nsIDocument* aDocument,
>-  if (aNewIndexInContainer != -1 &&

Why is losing that check ok?  That's ... highly non-obvious to me.

>+  // We found a insertion point to put all kids in, find the index to insert

"an insertion"

>+  uint32_t insertionIndex = point->mInsertedChildren.Length();

Hmm.  So this is looking for the insertion index of the last kid before aNewIndexInContainer.  But how does this handle finding the right insertion index when insertion points are nested, exactly?  Put another way, say element X has a binding which has the following bit in its content:

  <span id="Y">
    <h1 />
    <children />
    <h2 />
  </span>

and the <span id="Y"> itself has a binding that has a single <children>.  Say X has no kids and one is added.  Will it end up between h1 and h2, or after h2?

>+    nsIContent* currentSibling = aContainer->GetChildAt(parentIndex);

Why not just start with aFirstNewContent->GetPrevSibling() and walk the PrevSibling() chain instead of using GetChildAt?

>+    point->InsertInsertedChildAt(aContainer->GetChildAt(j), insertionIndex, this);

And avoiding GetChildAt here too would be good!

> nsBindingManager::ContentInserted(nsIDocument* aDocument,
>-  if (aIndexInContainer != -1 &&

Why is that OK?

>+  // It's not anonymous.

How do we know that?

>+  HandleChildInsertion(aContainer, aChild, aIndexInContainer, PR_FALSE);

s/PR_FALSE/false/

> nsBindingManager::ContentRemoved(nsIDocument* aDocument,
>+      // Check if this is actually a removal of an anonymous child. If so,
>+      // check if there are any insertion points (children elements) and remove
>+      // the explicit content inserted there, since it is no longer valid.
>+      if (aChild->GetBindingParent() != aContainer) {

The comment and code don't match....  aChild->GetBindingParent() != aContainer means removing a child that is not an anonymous root.  Why do we care whether it's an anonymous root, though?

>+        ClearInsertionPointsRecursively(aChild);

How about doing it iteratively instead of recursively anyway?  But I'd like us to do it at the right times, and at first glance the above doesn't.

And in particular, it looks to me like this code will always end up calling ClearInsertionPointsRecursively on every single removal of normal DOM nodes that have nothing to do with XBL.  That seems rather bad for performance.

In any case this code really needs to document _why_ it's doing what it's doing.

>+    point->RemoveInsertedChild(aChild);

I don't get it.

This code is removing the child from all nested insertion points going down the tree.  But the code in ContentAppended only inserts into the innermost insertion point.  One or the other is buggy; which one is it?

>@@ -1752,21 +1272,126 @@ nsBindingManager::HandleChildInsertion(n

And this also seems to insert only into the most-deeply-nested point.

Is there a clear description somewhere in here for what actual invariants the new insertion point setup maintains?  I've now spent weeks trying to reconstruct such a description and failing....  That's part of what's making this so hellish to review, and gives me no confidence that I'm actually finding all the broken bits.

>+  for (uint32_t parentIndex = aIndexInContainer - 1; parentIndex != uint32_t(-1); --parentIndex) {

See the comments in ContentAppended.

It's possible that this is no worse than what we have right now, of course.  In that case, at least get a followup filed, XXX/FIXME comments in here (I see the patch removes the existing XXX comments about FindInsertionPointAndIndex being bogus), and maybe share the identical code with ContentAppended?

>+nsBindingManager::FindNestedSingleInsertionPoint(nsIContent* aContainer,
>+                                                 bool& aMulti)

Please make the out param a pointer, not a reference, so it's clearer that it's an out param....

The "find the insertion point" pattern here is repeated a lot...  I wonder whether we can factor it out somehow.  :(

>+++ b/content/xbl/src/nsXBLBinding.cpp
>@@ -772,179 +588,69 @@ nsXBLBinding::GenerateAnonymousContent()
>+    nsNodeUtils::Clone(content, PR_TRUE, doc->NodeInfoManager(),

s/PR_TRUE/true/

>+    mContent = do_QueryInterface(clonedNode);

mContent = clonedNode->AsElement();

>+        if (point->IsDefaultInsertion()) {
>+          mDefaultInsertionPoint = point;

If we have two <children/> with no @includes, which one should end up as mDefaultInsertionPoint?  As in, should we be testing whether we already have one before setting?

It looks like the old code did something pretty similar to what we're doing now, so maybe it's not a huge worry....

>+nsXBLBinding::ClearInsertionPoints()

Why does this not need to clear the default insertion point?

>@@ -1292,24 +1077,18 @@ nsXBLBinding::ChangeDocument(nsIDocument
>-          mInsertionPointTable->Enumerate(ChangeDocumentForDefaultContent,

Why do we no longer need to UninstallAnonymousContent on things other than mContent?  This sort of keeps coming back to nothing clearly explaining what the new setup is supposed to be.  :(

>+++ b/content/xbl/src/nsXBLBinding.h
>+  void ClearInsertionPoints();
>+  nsAnonymousContentList* GetAnonymousNodeList();
>+  nsAnonymousContentList* GetContentListFor(nsIContent* aContent);

These all need documentation.

>+  nsRefPtr<nsXBLChildrenElement> mDefaultInsertionPoint;
>+  nsTArray<nsRefPtr<nsXBLChildrenElement> > mInsertionPoints;
>+  nsRefPtr<nsAnonymousContentList> mAnonymousContentList;
>+  nsTArray<nsRefPtr<nsAnonymousContentList> > mInsertionPointContentLists;

These _really_ need documentation.

>+++ b/content/xbl/src/nsXBLChildrenElement.cpp
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Fix to the MPL2 license block, please.

>+DOMCI_NODE_DATA(XBLChildrenElement, nsXBLChildrenElement)
>+  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(XBLChildrenElement)

No need for that.  Just return null from GetClassInfo.

>+nsXBLChildrenElement::ParseAttribute(int32_t aNamespaceID,
>+  return PR_FALSE;

false.

>+NS_INTERFACE_TABLE_HEAD(nsAnonymousContentList)

This looks wrong to me: it never QIs to cycle collection stuff.  Worth having smaug or mccr8 double-check this bit, but I'm pretty sure cc for this class is broken.

>+  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(NodeList)

That needs to go away.

>+++ b/content/xbl/src/nsXBLChildrenElement.h
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Fix the license here too.

>+  nsXBLChildrenElement(already_AddRefed<nsINodeInfo> aNodeInfo)

SetIsDOMBinding().  And add a WrapNode that wraps using ElementBinding, please.

>+  void RemoveInsertedChild(nsIContent* aChild)
>+    // Can't use this assertion as we cheat for dunamic insertions and

"dynamic".

>+  virtual JSObject* WrapObject(JSContext *cx, JSObject *scope, bool *triedToWrap)

MOZ_OVERRIDE, fix the signature, and put the impl in the .cpp file (and include NodeListBinding.h in there, not here).


>+++ b/dom/base/nsDOMClassInfo.cpp
>+  NS_DEFINE_CLASSINFO_DATA(XBLChildrenElement, nsElementSH,

Don't need this.

>+  DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(XBLChildrenElement, nsIDOMElement)

Or this.

>+++ b/dom/base/nsDOMClassInfoClasses.h
>+DOMCI_CLASS(XBLChildrenElement)

Or this.

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+      // XXXndeakin This test doesn't work in the new world. Or rather, it works, but
>+      // it's slow

Is there a plan for this?

Also, the comment is a bit confusing, seeing as it's not next to any tests... which test is it talking about?

>@@ -6989,47 +6990,50 @@ nsCSSFrameConstructor::ContentRangeInser
>+  // The xbl:children element won't have a frame, but default content can have the children as

Why did we not need to handle this in ContentAppended?

>+nsCSSFrameConstructor::GetInsertionPoint(nsIContent*   aContainer,
>+      *aMultiple = PR_TRUE;

true.

But also, need to null-check aMultiple, no?

>+  if (aMultiple && insertionElement->IsHTML(nsGkAtoms::fieldset)) {

There used to be a comment explaining why this is needed.  Please put it back.

>+    *aMultiple = PR_TRUE;

true.

>+++ b/layout/style/nsCSSRuleProcessor.cpp
> AncestorFilter::AssertHasAllAncestors(Element *aElement) const
>+    // MOZ_ASSERT(mElements.Contains(cur));

Uh... no.  Just no.  ;)

>+++ b/toolkit/content/tests/chrome/test_bug562554.xul

Why are the changes to this test needed?

r- pending the above fixed and lots more comments, but at least I should be able to review interdiffs fairly fast....

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703990 [details] [diff] [review]
Part 2 updated to tip

>+++ b/content/base/src/Element.cpp
>@@ -2161,39 +2162,32 @@ Element::List(FILE* out, int32_t aIndent
>+  for (nsIContent* child = iter.nextChild(); child; child = iter.nextChild()) {

Won't this list the kids of the node twice in the common case?

>+++ b/layout/inspector/src/inDOMUtils.cpp
>+// XXXndeakin fix this
>+//          bindingManager->GetContentListFor(content, getter_AddRefs(kids));

Er.... yes, please.

>+++ b/layout/inspector/src/inDeepTreeWalker.cpp
>+// XXXndeakin fix this
>+//        if (!kids)
>+//          bindingManager->GetContentListFor(content, getter_AddRefs(kids));


And this.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703991 [details] [diff] [review]
Part 3 updated to tip

>+++ b/content/base/src/ChildIterator.h
>+// were added into the insertion point, return the insetion point's default

s/insetion/insertion/

>+++ b/layout/xul/base/src/nsListBoxObject.cpp
>+FindBodyContent(nsIContent* aParent)
>-    // start from the end, cuz we're smart and we know the listboxbody is probably at the end

We're losing that optimization.  Can we try adding a way to iterate backwards, perhaps?  Followup is fine.

r=me

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703992 [details] [diff] [review]
Part 4 updated to tip

>+++ b/content/base/src/ChildIterator.cpp
>+      mHasAnonymous = true;

I'm not sure that does what we want.

In particular, what the frame constructor cares about here is whether the flattened tree matches the DOM tree, especially for things like the whitespace optimizations...  So it thinks "xbl is involved" if there are _any_ insertion points or any other funny business under the element, no?  At least before this patch.  After this patch we seem to only set that boolean for bound elements....

>+nsIContent* FlattenedChildIterator::get()

Can you assert !mIsFirst, since we seem to rely on that?

And generally document how this is supposed to be used....

>+nsIContent* FlattenedChildIterator::previousChild()
>+    if (--mIndexInInserted) {
>+      return point->mInsertedChildren[mIndexInInserted - 1];

That looks wrong to me.  Why do we need the if check on mIndexIsInserted there?  Why are we subtracting twice?

>+  else if (mDefaultChild) {
>+  // If we're already in default content, check if there are more nodes there

Please fix the indent.

>+++ b/content/base/src/ChildIterator.h
>+    // Sometimes, child won't be found, but bz thinks that's not necessarily
>+    // the case when called from ContentInserted if first-letter frames are about.

I don't understand this comment....

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+nsCSSFrameConstructor::FindNextSibling(FlattenedChildIterator aIter,
>-    // XXXbz Can happen when XBL lies to us about insertion points.  This check
>-    // might be able to go away once bug 474324 is fixed.

Looks like we removed this check... is that because it can't happen anymore?  I'd like to understand why if so.

>@@ -6090,53 +6081,49 @@ nsCSSFrameConstructor::GetInsertionPrevS
>-    // The check for IsRootOfAnonymousSubtree() is because editor is
>-    // severely broken and calls us directly for native anonymous
>-    // nodes that it creates.

Please put that comment back.

>-    // If there is no previous sibling, then find the frame that follows

And this one.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703993 [details] [diff] [review]
Part 5 updated to tip

r=me modulo the xblinvolved issue I mentioned earlier.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703994 [details] [diff] [review]
Part 6 updated to tip

>+++ b/layout/inspector/src/inDOMUtils.cpp
> inDOMUtils::GetChildrenForNode(nsIDOMNode* aNode,
>+    nsCOMPtr<nsIContent> node = do_QueryInterface(aNode);
>+    nsSimpleContentList* list = new nsSimpleContentList(node);

Said to say, node might be null there, because nsIDOMNode is not builtinclass...

I'd rather we held a ref to the list while we play with it.  Make it an nsRefPtr, and forget() into aChildren, ok?

>+++ b/layout/inspector/src/inDeepTreeWalker.cpp
>+      nsSimpleContentList* list = new nsSimpleContentList(node);

And here, set kids right after that line.

r=me

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703990 [details] [diff] [review]
Part 2 updated to tip

Given part 6, the only issue here in part 2 is the double-listing....

Comment 53

[Blake Kaplan (:mrbkap) (inactive)]

By the way, Neil, do you have time to address the review comments? If not, I could take this.

Comment 54

[Neil Deakin]

(In reply to Blake Kaplan (:mrbkap) from comment #53)
> By the way, Neil, do you have time to address the review comments? If not, I
> could take this.

Yes, feel free to work on it. Thank you.

Comment 55

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #46)

> The changes to accessible/ may want a quick once-over from an a11y peer.

I commented in in comment #26 (it doesn't seem they were addressed)

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Taking per comment 54.

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

Any comments not here should be answered in the interdiff... Also note that some comments were dealt with by later patches already attached here.

I still haven't dealt with the comments in comment 26. I'll get to them after I deal with the main meat of this patch. There are two comments at the end that I haven't addressed fully yet.

(In reply to Boris Zbarsky (:bz) from comment #46)
> This needs a ... much more extensive checkin comment.  A decent description
> of what the new setup is should go in here, for example.  It sure would have
> made this easier to review.  :(

Here's a beginning, let me know how best to add to it or what else is unclear:

Rework XBL insertion points and clean up related code to more closely follow the Web Components model. Instead of maintaining a hashtable of insertion points in bindings (and removing insertions points from the tree) leave the insertion points in the tree as explicit placeholders and teach all other relevant code how to walk the explicit children of elements via two iterators (ExplicitChildIterator and FlattenedChildIterator). Note that this patch does not maintain 100% compatibility with the previous code: there are bug fixes and behavior changes included. For example, by having explicit insertion points in the bindings, it is now easier to handle dynamic changes to the bound element correctly (as well as, eventually, handling dynamic changes to the binding correctly).

> >+++ b/content/base/src/FragmentOrElement.cpp
> >+    nsIContent* parent = OwnerDoc()->BindingManager()->
> >+    GetInsertionParent(const_cast<nsIContent*>(this));
> >+    if (parent) {
> >+      return parent;
> 
> Hmm.  So this returns the DOM parent for elements which are not filtered
> into any insertion point, right?
> 
> That's probably OK...

Yes, and I think that's the only sane thing to do.

> > nsBindingManager::GetContentListFor(nsIContent* aContent)
> >+    return aContent->GetChildNodesList();

> Hmm.  So this is looking for the insertion index of the last kid before
> aNewIndexInContainer.  But how does this handle finding the right insertion
> index when insertion points are nested, exactly?  Put another way, say
> element X has a binding which has the following bit in its content:

I tried your example and the newly-added child gets added after the h2 (it's the first non-binding non-insertion-point node starting from the end of the anonymous child list). This imitates the current behavior, so I'm going to leave things as they are for now and fix this in a followup bug. I'm hoping it should be a simple fix.

> >+        ClearInsertionPointsRecursively(aChild);
> 
> In any case this code really needs to document _why_ it's doing what it's
> doing.

It looks like this code is trying to make sure that when node with an <xbl:children> element child is removed from the tree, that we no longer try to use the <xbl:children> element. In that case, we can already restrict it to only doing the recursive walk when GetBindingParent returns non-null. However, it looks to me like we should do that in nsXBLChildrenElement::UnbindFromTree. Also, this code doesn't update the associated binding to let it know that it shouldn't use this <xbl:children> element anymore. I'd like to do that stuff as a followup patch, though.

> >+    point->RemoveInsertedChild(aChild);
> 
> This code is removing the child from all nested insertion points going down
> the tree.  But the code in ContentAppended only inserts into the innermost
> insertion point.  One or the other is buggy; which one is it?

ContentAppended is buggy. In the static case, children get inserted into all insertion points going down the tree as we construct the tree and set up bindings on elements as appropriate. The dynamic case needs to imitate that behavior.

> >+nsBindingManager::FindNestedSingleInsertionPoint(nsIContent* aContainer,
> >+                                                 bool& aMulti)
> The "find the insertion point" pattern here is repeated a lot...  I wonder
> whether we can factor it out somehow.  :(

I don't see a good way of doing so... I'd rather tackle this as a followup.

> >+++ b/content/xbl/src/nsXBLBinding.cpp
> >@@ -772,179 +588,69 @@ nsXBLBinding::GenerateAnonymousContent()
> >+        if (point->IsDefaultInsertion()) {
> >+          mDefaultInsertionPoint = point;
> 
> If we have two <children/> with no @includes, which one should end up as
> mDefaultInsertionPoint?  As in, should we be testing whether we already have
> one before setting?
> 
> It looks like the old code did something pretty similar to what we're doing
> now, so maybe it's not a huge worry....

I can't understand enough of the old code to figure out what it did in this case. Web components uses the first insertion point in this case, so I'd prefer to change this. I'll do that in a separate patch.

> >@@ -1292,24 +1077,18 @@ nsXBLBinding::ChangeDocument(nsIDocument
> >-          mInsertionPointTable->Enumerate(ChangeDocumentForDefaultContent,
> 
> Why do we no longer need to UninstallAnonymousContent on things other than
> mContent?  This sort of keeps coming back to nothing clearly explaining what
> the new setup is supposed to be.  :(

Correct me if I'm wrong, but it appears that the old code removed <xbl:children> elements from the <xbl:content> element when it initialized the binding and then did some sort of dance to recuperate the original location when actually populating a binding's anonymous content. Then, it appears that in order to make default content work correctly, it then bound the <xbl:children> element to the document, but didn't put them in the binding parent's child list. We no longer do the weird dance with removing the elements, so we no longer have to explicitly do anything to make default content work.
> 
> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> >+      // XXXndeakin This test doesn't work in the new world. Or rather, it works, but
> >+      // it's slow
> 
> Is there a plan for this?
> 
> Also, the comment is a bit confusing, seeing as it's not next to any
> tests... which test is it talking about?

Neil, if you want to weigh in here, I'd appreciate it. I think the "test" referred to is further down where we decide whether to do multiple ContentInserted calls as opposed to a ContentAppended. I don't see at all why this check would be more or less effective now...

> >@@ -6989,47 +6990,50 @@ nsCSSFrameConstructor::ContentRangeInser
> >+  // The xbl:children element won't have a frame, but default content can have the children as
> 
> Why did we not need to handle this in ContentAppended?

I have to admit that I don't understand why it's necessary in ContentRangeInserted, but I've added it to ContentAppended since the two (insertion and appending) cases should be the same.

> 
> >+++ b/layout/style/nsCSSRuleProcessor.cpp
> > AncestorFilter::AssertHasAllAncestors(Element *aElement) const
> >+    // MOZ_ASSERT(mElements.Contains(cur));
> 
> Uh... no.  Just no.  ;)

I'm pushing a try run (https://tbpl.mozilla.org/?tree=Try&rev=05ec54efaa2a) to see if/why this was done.

> >+++ b/toolkit/content/tests/chrome/test_bug562554.xul
> 
> Why are the changes to this test needed?

The test as originally written now fails. I'll debug and update the bug with my results.

Comment 58

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: interdiff on part 1 addressing ms2ger's comments

This is mechanical.

Comment 59

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: interdiff on part 1 addressing bz's comments

This is not at all mechanical.

Comment 60

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) from comment #57)
> I'm pushing a try run (https://tbpl.mozilla.org/?tree=Try&rev=05ec54efaa2a)
> to see if/why this was done.

My try push failed due to user error, but the assertion fires because we're now seeing <xbl:children> elements in AncestorFilter. I've written a patch that makes sure that we no longer push <xbl:children> into the AncestorFilter, but I don't know if that's correct.

Comment 61

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix for the assertion

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744926 [details] [diff] [review]
Possible fix for the assertion

r=me if you use GetParentNode() instead of GetParent().

Comment 63

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) from comment #57)
> > >+++ b/toolkit/content/tests/chrome/test_bug562554.xul
> > Why are the changes to this test needed?
> The test as originally written now fails. I'll debug and update the bug with
> my results.

The test as written is bogus. It uses a <toolbarbutton> binding with a <stack> child. In the old XBL1 world, this caused us to completely ignore the binding on the <toolbarbutton> (meaning that we'd build frames and lay out the contents of the <stack>) whereas in the new world, we still apply the button, and don't do anything with the children that don't match. The new behavior is more sane. The rewritten test no longer depends on this quirk.

Comment 64

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #47)
> Won't this list the kids of the node twice in the common case?

I've moved the content-list outputting into the case where the binding manager gave us an anonymous node list back (since that's really the only case we care about).

> >+++ b/layout/inspector/src/inDOMUtils.cpp
> >+++ b/layout/inspector/src/inDeepTreeWalker.cpp

These have since moved to use GetChildren and no longer require changes.

Comment 65

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: interdiff on part 2

Comment 66

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #49)
> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> >+nsCSSFrameConstructor::FindNextSibling(FlattenedChildIterator aIter,
> >-    // XXXbz Can happen when XBL lies to us about insertion points.  This check
> >-    // might be able to go away once bug 474324 is fixed.
> 
> Looks like we removed this check... is that because it can't happen anymore?
> I'd like to understand why if so.

I put a breakpoint in the check and loaded the testcases in bug 474324 without hitting this case. In the new code, bug 474324 doesn't seem like it would apply anymore anyway, since we no longer do the binding content dropping, right?

Comment 67

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: interdiff on part 4

With this patch, I think that all of the comments have answers or have been addressed except for the accessibility ones. I'll do another sweep tomorrow to make sure (and I'm pushing this to try).

Comment 68

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: interdiff on part 2 v1.1

Comment 69

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: interdiff on part 4 v1.1

Only change here is to "prime the iterator" (found on try server thanks to bz's suggested assertion in FlattenedChildIterator::get).

Comment 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744348 [details] [diff] [review]
interdiff on part 1 addressing ms2ger's comments

Sorry for the lag on this....

r=me, though it might in fact make sense to s/nextChild()/GetNextChild()/ (since it can return null) and make GetFlattenedTreeParent non-const.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 746125 [details] [diff] [review]
interdiff on part 2 v1.1

r=me

Comment 72

[Boris Zbarsky [:bzbarsky]]

Please do file followups as needed for the various things from comment 57 that need followups.

As for the rest of comment 57:

> I don't see at all why this check would be more or less effective now..

Likewise.  I would really like to either make this comment clearer or remove it if it's wrong.

> I have to admit that I don't understand why it's necessary in ContentRangeInserted

The point is that if one of the kids of an <xbl:children> comes through here, the parentFrame will end up null because GetFrameFor on the xbl:children will return null.  In every single other case, having no parent frame means there is no need for a frame on the kid, but that's not the case if the parent is <xbl:children>.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 746589 [details] [diff] [review]
interdiff on part 4 v1.1

The previousChild implementation still doesn't quite make sense to me.  In particular, now it's testing mIndexInInserted twice for no obvious reason.

Looking at this carefully, it seems to me that after nextChild() returns something that comes from an insertion point, mIndexInInserted is one greater than its index in the insertion point.  So the invariant seems to be that if mIndexInInserted is nonzero then we're pointing at inserted content, and that content is at (mIndexInInserted - 1).

Does that match your understanding?  If so, we should document!

If the above is correct, then I think the original code here:

   if (mIndexInInserted) {
     nsXBLChildrenElement* point = static_cast<nsXBLChildrenElement*>(mChild);
     if (--mIndexInInserted) {
       return point->mInsertedChildren[mIndexInInserted - 1];
     }
     mChild = mChild->GetPreviousSibling();

was in fact technically correct: first we move mIndexInInserted down one, then if that lands it at 0 that means we want a non-inserted child instead.  Otherwise we want the child at mIndexInInserted-1.  I'm not quite sure how to write that more clearly, short of adding comments.  :(

r=me with this issue addressed.

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744350 [details] [diff] [review]
interdiff on part 1 addressing bz's comments

The checkin comment from comment 57 seems fine.

>+++ b/content/base/src/ChildIterator.h

>+// This class iterates normal DOM child nodes of a given DOM node with with

s/with with/with/

>+// insertion point, if any bindings on the given element are ignored.

That last clause is really confusing.  How about a period after "point" and then "Any bindings on the given element are ignored for purposes of determining which insertion point children are filtered into." or something?

>+  // The current child. When we encounter an <xbl:children> insertion point,
>+  // this remains as the insertion point whose content we're iterating 

Maybe s/this/mChild/ to be really clear about what we mean?

>+  // If non-null, this points to the current default content for the current
>+  // insertion point that we're inserting 

s/inserting/iterating/ ?

>   uint32_t mIndexInInserted;

We should document that it's one larger than the index of our current child in mChild or something along those lines...

>+++ b/content/xbl/src/nsXBLBinding.h
>+  void ClearInsertionPoints();
>+  nsAnonymousContentList* GetAnonymousNodeList();
>+  nsAnonymousContentList* GetContentListFor(nsIContent* aContent);

I think these still need documentation.

>+++ b/content/xbl/src/nsXBLChildrenElement.cpp
>+#include "mozilla/dom/ElementBinding.h"

Is this actually used?

>+NS_INTERFACE_TABLE_HEAD(nsAnonymousContentList)

This still needs to QI to the CC interfaces.  Otherwise it will never traverse mParent.

>+++ b/layout/base/nsCSSFrameConstructor.cpp

>+  if (!parentFrame && !aContainer->NodeInfo()->Equals(nsGkAtoms::children,

You want to take out the previous return if !parentFrame, right?

r=me with the above fixed.

Comment 75

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #74)
> >+NS_INTERFACE_TABLE_HEAD(nsAnonymousContentList)
> 
> This still needs to QI to the CC interfaces.  Otherwise it will never
> traverse mParent.

This already has:

  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsAnonymousContentList)

so I didn't do anything here. On the other hand, I did add a conspicuously missing nsISupports implementation to its QI.

(In reply to Boris Zbarsky (:bz) from comment #70)
> make GetFlattenedTreeParent non-const.

I left the method const because there are already other consumers that require it to be const (and semantically it should be const, anyway).

Comment 76

[:Gavin Sharp [email: [e-mail]]]

Given the amount of bugs filed on fixing our internal users, and the fact that we haven't even been able to identify all of those problem cases (bug 875165 comment 3), I'm really worried about the compatibility impact of this. Is there really no way to cleanly maintain the old behavior, for chrome code?

Assuming there isn't, what can we do to be sure we find as much breakage ahead of time (including in add-ons)?

Comment 77

[Doug Turner (:dougt)]

>Assuming there isn't, what can we do to be sure we find as much breakage ahead of time (including in add-ons)?

Like landing on nightly early in the cycle!

Comment 78

[William Chen [:wchen]]

Attachment: Push <xbl:children> in ancestor filter.

Comment 79

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Addressing bz's comments

Attachment 758821 [details] [diff] should apply on top of this.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759435 [details] [diff] [review]
Addressing bz's comments

>+  // Removes all inserted node from <xbl:children> insertion points under us.

s/node/nodes/

The rest looks lovely.

Comment 81

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixes for accessiblity comments (comment 26)

I am totally in the dark on what these changes do or how to test them. I don't know how to test them or even find the cases that are likely to break. This mostly addresses the comments in comment 26 I'll push it to try shortly.

Comment 82

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #76)
> Given the amount of bugs filed on fixing our internal users, and the fact
> that we haven't even been able to identify all of those problem cases (bug
> 875165 comment 3), I'm really worried about the compatibility impact of
> this. Is there really no way to cleanly maintain the old behavior, for
> chrome code?

I'm worried that there will be a few regressions, but given that in all of the Firefox XUL, we've only found 5 regressions I'm hopeful that there won't be a deluge of regressions.

As for maintaining compatibility... we could, with some work, replicate the "ignore <content> if there's a child that doesn't match any of the <children> in the binding" behavior. However, I'd really prefer not to do that as the fixes (at least thus far) have been trivial and that behavior isn't in web components. In terms of the old "remove <children> from the binding after processing" behavior, we can't replicate that at all. It would reintroduce a bunch of code that we really want to rip out, makes dealing with dynamic changes nigh-on impossible, and moves us in the wrong direction.

> Assuming there isn't, what can we do to be sure we find as much breakage
> ahead of time (including in add-ons)?

Landing early in the cycle (hopefully the day after we branch) should help. I have a couple of ideas about how to get some console spew when something related to this patch breaks. More news on that in a bit.

Comment 83

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 758821 [details] [diff] [review]
Push <xbl:children> in ancestor filter.

Review of attachment 758821 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, let's try this. Boris, are you OK with this?

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758821 [details] [diff] [review]
Push <xbl:children> in ancestor filter.

I need to think about this a bit on Monday...  There has got to be a way to make this less boilerplatey.  Especially since we will simply have this problem again with display:transparent...

Comment 85

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 758821 [details] [diff] [review]
Push <xbl:children> in ancestor filter.

OK, so I've convinced myself this is a different problem from display:transparent, at least for purposes of the frame constructor changes.  That said, the change to ConstructFrameFromItemInternal() doesn't make sense to me (as in, it looks wrong).  What is it trying to do?

For ProcessChildren/BuildInlineChildItems, can we claim that parent->NodeInfo()->Equals(nsGkAtoms::children, kNameSpaceID_XBL) if and only if parent != aContent/parentContent?  Seems like that would be a faster check...  In either case, parent->IsElement() should be true in those, so that part can be simplified.

>+            nsIContent* parent = child->GetContent() ? child->GetContent()->GetParent() : nullptr;

The only frame with a null GetContent() should be the viewport, and that will never be a child.

Comment 86

[William Chen [:wchen]]

Attachment: Push <xbl:children> in ancestor filter. v2

(In reply to Boris Zbarsky (:bz) from comment #85)
> Comment on attachment 758821 [details] [diff] [review]
> Push <xbl:children> in ancestor filter.
> 
> That said, the change to ConstructFrameFromItemInternal() doesn't make sense
> to me (as in, it looks wrong).  What is it trying to do?
> 
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9973
When ProcessChildren calls ConstructFramesFromItemList(aState, itemsToConstruct, aFrame, aFrameItems);
itemsToConstruct may contain a content that is a child of <xbl:children> and the ancestor filter does not contain the <xbl:children> element.
We need to push it some time before the FrameConstructor tries to resolve styles.

The stack I was debugging looks like this:
nsStyleSet::ResolveStyleFor
...
nsCSSFrameConstructor::ConstructFrameFromItemInternal
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#3646
nsCSSFrameConstructor::ConstructFramesFromItem
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#5483
nsCSSFrameConstructor::ConstructFramesFromItemList
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9814
nsCSSFrameConstructor::ProcessChildren
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9973
...

I not sure about the most appropriate function to push the <xbl:children>, but it seems to make sense to me to do it in ConstructFrameFromItemInternal right before we push child of the <xbl:children>. In the updated patch, I've pushed earlier in ConstructFramesFromItemList in case that's any better.

> For ProcessChildren/BuildInlineChildItems, can we claim that
> parent->NodeInfo()->Equals(nsGkAtoms::children, kNameSpaceID_XBL) if and
> only if parent != aContent/parentContent?
That should be true (as long as we don't have any child/parent mismatches outside of XBL anonymous subtree root nodes).

> The only frame with a null GetContent() should be the viewport, and that will
> never be a child.
Perhaps we go even further and assert that GetContent()->GetParent() is non-null because we are iterating over child frames? It's not clear to me whether that would be true.

I've addressed the other comments. Thanks Boris.

Comment 87

[William Chen [:wchen]]

Attachment: Push <xbl:children> in ancestor filter v1 diff v2

Comment 88

[Boris Zbarsky [:bzbarsky]]

Ah, and the key part is that we did push it when we were creating the item itself but now we might create kids for it, right?

So if this were not xbl:children we'd just push it once when we process its kids.  But the problem is that we're flattening the lists together....

I need to think about it when it's not 4am.  That means tomorrow.  :(

Comment 89

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 761808 [details] [diff] [review]
Push <xbl:children> in ancestor filter. v2

Alright, given the setup here, I think the original pushing in ConstructFrameFromItemInternal is better, with a comment explaining that this is needed to handle style resolution on descendants.  I considered handling this entirely inside the filter code, but this is probably actually faster....

r=me with that

Comment 90

[William Chen [:wchen]]

Attachment: Push <xbl:children> in ancestor filter. v3

I missed pushing a children element in the case of undisplayed content.

Comment 91

[William Chen [:wchen]]

Attachment: Push <xbl:children> in ancestor filter v2 diff v3

Comment 92

[William Chen [:wchen]]

Attachment: Normalize the undisplayed map entry for <xbl:children>

Comment 93

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 766567 [details] [diff] [review]
Normalize the undisplayed map entry for <xbl:children>

>+  // In the case of XBL default content, there is an asymmetry between tree
>+  // traversal from leaves towards the root and from the root towards the leaves
>+  // due to the lack of a frame for <xbl:children> elements. In the case of
>+  // traversal from leaves towards the root, we might encounter a <xbl:children>
>+  // element that is not present in traversal from the root towards the leaves
>+  // (instead we only see the insertion parent). Here we normalize children
>+  // elements to the insertion parent (the parent of the children element).

Is this actually an asymmetry (which seems really weird), or just a difference between content tree and frame tree?  I think this could be explained better either way.

>+  if (parentContent &&
>+      parentContent->NodeInfo()->Equals(nsGkAtoms::children, kNameSpaceID_XBL)) {
>+    parentContent = parentContent->GetParent();
>+    // Change the callers pointer for the parent content to be the insertion parent.

callers -> caller's


r=dbaron with that fixed

Comment 94

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 766565 [details] [diff] [review]
Push <xbl:children> in ancestor filter. v3

Could you explain what's changed since bz granted review in comment 89?

I'm also confused as to the changes from the previous patch in no longer asserting parent->IsElement() (and instead checking it).

Comment 95

[William Chen [:wchen]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #93)
> Comment on attachment 766567 [details] [diff] [review]
> Normalize the undisplayed map entry for <xbl:children>
>
> Is this actually an asymmetry (which seems really weird), or just a
> difference between content tree and frame tree?  I think this could be
> explained better either way.
> 
Yes, this is really a difference between the content tree and the frame tree. How about this as a comment:

In the case of XBL default content, <xbl:children> elements do not get a frame causing a mismatch between the content tree and the frame tree. |GetEntryFor| is sometimes called with the content tree parent (which may be a <xbl:children> element) but the parent in the frame tree would be the insertion parent (parent of the <xbl:children> element). Here the children elements are normalized to the insertion parent to correct for the mismatch.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #94)
> Comment on attachment 766565 [details] [diff] [review]
> Push <xbl:children> in ancestor filter. v3
> 
> Could you explain what's changed since bz granted review in comment 89?
> 
The changes made from from comment 89 are moving the ancestor push from ConstructFramesFromItemList to ConstructFrameFromItemInternal (to address review comment) and the change in attachment #766566 [details] [diff] [review]
> I'm also confused as to the changes from the previous patch in no longer
> asserting parent->IsElement() (and instead checking it).
I'm not sure which change you are talking about here.

Thanks David.

Comment 96

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to William Chen [:wchen] from comment #95)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #93)
> > Comment on attachment 766567 [details] [diff] [review]
> > Normalize the undisplayed map entry for <xbl:children>
> >
> > Is this actually an asymmetry (which seems really weird), or just a
> > difference between content tree and frame tree?  I think this could be
> > explained better either way.
> > 
> Yes, this is really a difference between the content tree and the frame
> tree. How about this as a comment:
> 
> In the case of XBL default content, <xbl:children> elements do not get a
> frame causing a mismatch between the content tree and the frame tree.
> |GetEntryFor| is sometimes called with the content tree parent (which may be
> a <xbl:children> element) but the parent in the frame tree would be the
> insertion parent (parent of the <xbl:children> element). Here the children
> elements are normalized to the insertion parent to correct for the mismatch.

Sounds good, except I'd change the "children elements" in the last sentence to
"xbl:children elements" to avoid confusion.

> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #94)
> > Comment on attachment 766565 [details] [diff] [review]
> > Push <xbl:children> in ancestor filter. v3
> > 
> > Could you explain what's changed since bz granted review in comment 89?
> > 
> The changes made from from comment 89 are moving the ancestor push from
> ConstructFramesFromItemList to ConstructFrameFromItemInternal (to address
> review comment) and the change in attachment #766566 [details] [diff] [review] [diff]
> [review]
> > I'm also confused as to the changes from the previous patch in no longer
> > asserting parent->IsElement() (and instead checking it).
> I'm not sure which change you are talking about here.

Sorry, I think it looks like I ran the diff between v1 and v3 the wrong way around.

Comment 97

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 766565 [details] [diff] [review]
Push <xbl:children> in ancestor filter. v3

ok, r=dbaron, although I think it's more of an r=bz since I'm just looking at the diffs from v2.

Comment 98

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Compatibility hack for the child selector in CSS

I was hoping to avoid having to do this, but as Gavin pointed out in bug 887950 comment 2, there are simply too many places that rely on being able to address default comment via the child combinator (note also that the bug only points out a single instance, systematically finding all such cases is hard).

This patch makes selecting the children element optional via the child combinator. That means that given a binding with |<content><a><children><b /></children></a></content>| the <b> is selectable either with |a > b { ... }| *or* |a > children > b { ... }|.

With this patch, I'm not nearly as worried about being deluged with bugs from the (other) incompatibilities introduced here.

We don't plan to do this for Web Component's <content> element.

Comment 99

[Blake Kaplan (:mrbkap) (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=317e57ff7c41

Comment 100

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 768666 [details] [diff] [review]
Compatibility hack for the child selector in CSS

So what this doesn't get right is the case where the content tree looks like:

  <body>
    <div>
      <xbl:children>
        <p></p>
      </xbl:children>
    </div>
  </body>

and the selector is "body > * > p".  This used to match, but it won't match even with this patch because the xbl:children does match the *.

I think it's probably more correct to do a recursive call (like we do for the case where SelectorMatches fails and we have a certain pairing of operators) when element is <xbl:children>, whether or not the match fails or succeeds.

It would also be really good to have a test for this.

Comment 101

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Compatibility hack for the child selector in CSS v2

Comment 102

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 769293 [details] [diff] [review]
Compatibility hack for the child selector in CSS v2

r=dbaron

(Though it might also not be a bad idea to test the case with another div outside .a, and the selector .aparent > * > .b .

Comment 103

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/731a5fcf1328
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7bde7c1ec6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5516ee85d26d
https://hg.mozilla.org/integration/mozilla-inbound/rev/53af025b0036
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8207252e9c

Comment 104

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/731a5fcf1328
https://hg.mozilla.org/mozilla-central/rev/ee7bde7c1ec6
https://hg.mozilla.org/mozilla-central/rev/5516ee85d26d
https://hg.mozilla.org/mozilla-central/rev/53af025b0036
https://hg.mozilla.org/mozilla-central/rev/1b8207252e9c

Comment 105

[[e-mail]]

Comment on attachment 703989 [details] [diff] [review]
Part 1 updated to tip

>-      // There are children being placed underneath us, but we have no specified
>-      // insertion points, and therefore no place to put the kids.  Don't generate
>-      // anonymous content.
>-      // Special case template and observes.
[This got lost but we have markup depending on it... filing new bug.]