501847 - Handle appends to the trailing inline of an {ib} split better

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing the problem

The attached page (download and unzip) loads in about 48 seconds from local disk in firefox.  It loads in 2-6 seconds in Opera and Safari.  A large part of the problem are all the bogus tags in the page which leads to lots of {ib} splits.  One in particular contains most of the page.  Any time a block is appended to such an {ib} split, we reframe the ancestor, which is expensive.

If we didn't have to worry about floats, we could just steal all kids up to our prevSibling of the last inline part of the split back into the block, then append to the block.  We could try checking for floating descendants and doing that if there are none, or we could try to move the floats too.  Or just reconstruct the kids of the trailing inline.  Or something....

Comment 1

[Boris Zbarsky [:bzbarsky]]

I was rereading the CSS spec for block-inside-inline and discovered that our implementation was completely bogus.  In particular, CSS actually specifies breaking around the block kids, not wrapping all kids from first block to last in a single block.

I've gone ahead and implemented this; instead of creating just three {ib} frames we'll create 2N+1, where N is the number of runs of consecutive blocks.  We could break around each block individually if desired, but I think coalescing runs of blocks is better (less memory, if nothing else).  One drawback is that it does mean that inserting an inline between two blocks involves reframing.  I can probably optimize this further, but it would take some work.  There is always a leading inline and trailing inline.

I do allow empty inlines between runs of blocks in dynamic append situations, which is what lets me do fast appends: when appending we always append to/after the trailing inline, and never require a reframe (unless we have a block-level ::after on an inline).  Time to load the attached testcase drops from 60s to 10s or so (6s with the HTML5 parser, which seems to notify less often).  There are still some reframes on inserts of inlines before blocks when shipping out misplaced content from tables, unfortunately.

I can try to write a crashtest for the O(N^2) behavior we used to have here if people feel it would be worth it; I'm not sure how reliable we can get that sort of thing.

Patches coming up.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Some preliminary cleanup

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Fix assumptions consumers make about the number of ib splits

I welcome suggestions for better names for the nsLayoutUtils functions.

Comment 4

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

(In reply to comment #1)
> I was rereading the CSS spec for block-inside-inline and discovered that our
> implementation was completely bogus.  In particular, CSS actually specifies
> breaking around the block kids, not wrapping all kids from first block to last
> in a single block.

Can you give an example in which the behavior difference is detectable?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. Change nsInlineFrame::IsSelfEmpty to play nicer with the inlines in {ib} splits

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Can you give an example in which the behavior difference is detectable?

Sure thing:

  <span style="border: 1px solid green">
    <span style="display: block"></span>
    Text
    <span style="display: block"></span>
  </span>

Gecko's current implementation (before my patches) doesn't draw a green border above/below "Text".  IE8, Opera 10, Safari 4, and Gecko with my patch do.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4. The main frame construction and test changes

These are as described in comment 1.  I actually have this as three separate patches locally (one change to ConstructInline, one change to dynamic handling, and the test changes/additions) if someone wants to see them individually for some reason.  I'll qfold them before pushing, though, so as not to push broken states.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5. Get rid of MoveFrames, since there's only one caller now.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6. Rip the float-munging code out of MoveChildrenTo, since we no longer stick inlines into the "wrong" block

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Can you give an example in which the behavior difference is detectable?

Another example is actually effectively one of our reftests, assuming our interpretation of what to do with relative positioning is correct:

  <span style="position: relative; left: 100px">
    <span style="display: block"></span>
    <span style="float:left">Left</span>
    <span style="display: block"></span>
  </span>

The left edge of the float ends up 100px from the left edge of the outer span's parent block in current Gecko, and flush against the left edge of the outer span's parent block with my changes.  Behavior of other UAs is somewhat inconsistent; I posted to www-style about this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  I just realized that this patch makes some of the tests for bug 424236 fail because there are now multiple anonymous blocks.  I can adjust the references pretty easily if the new behavior is actually what we want.  Is it?  Seems to me like it ought to be, at least as much as the behavior we used to have.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree.

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 399272 [details] [diff] [review]
Part 2.  Fix assumptions consumers make about the number of ib splits

>   /**
>-   * Return whether aFrame is an inline frame in the first part of an {ib}
>-   * split.
>+   * Return whether aFrame is an inline frame that is in an {ib} split
>+   * and is NOT the first inline in it.
>    */

The "is an inline frame" part is no longer true. Maybe "Return whether aFrame is in an {ib} split and is NOT the first (inline-)frame in it." Same thing for both FrameIsNonFirstInIBSplit and FrameIsNonLastInIBSplit.

>-     * 3) The frame is in the first part of an {ib} split.
>+     * 3) The frame is in and {ib} split and is not the last part.

"The frame is in *an* {ib} split..."

Comment 14

[Boris Zbarsky [:bzbarsky]]

> The "is an inline frame" part is no longer true.

Indeed.  The comment is now:

   * Return true if aFrame is in an {ib} split and is NOT one of the
   * continuations of the first inline in it.

and similar for the other function with s/first/last/.

> "The frame is in *an* {ib} split..."

Fixed.  Good catch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged to bug 233463.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 merged to bug 233463

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 merged to bug 233463

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 merged to bug 233463

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 merged to bug 233463

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 merged to bug 233463

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 401485 [details] [diff] [review]
Part 6 merged to bug 233463

"get rid of views" is the answer to your comment, I think

Comment 22

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

Comment on attachment 401482 [details] [diff] [review]
Part 3 merged to bug 233463

This is pretty obviously wrong for RTL.  I'd suggest fixing by assigning to haveStart and haveEnd variables inside the NS_FRAME_IS_SPECIAL test.

Beyond that, I really can't claim to understand this code anymore; there might be reasons to also change the zeroEffectiveSpanBox code, but I think that code matters less than it used to, and it's already substantially different.  So I think this is ok with the RTL business fixed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to comments

> This is pretty obviously wrong for RTL.

Good catch!  This patch has that fixed, plus tests that would have caught the problem.

Comment 24

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

Comment on attachment 403955 [details] [diff] [review]
Part 3 updated to comments

r=dbaron, although you don't need the ltr variable; you can put it straight into the if().

Comment 25

[Boris Zbarsky [:bzbarsky]]

Eliminated the ltr local.

Comment 26

[Timothy Nikkel (:tnikkel)]

Comment on attachment 401483 [details] [diff] [review]
Part 4 merged to bug 233463

> static nsIFrame*
> GetLastSpecialSibling(nsIFrame* aFrame, PRBool aIgnoreEmpty)

aIgnoreEmpty doesn't do the "obvious thing" based just on the name, so there should be a comment here explaining that aIgnoreEmpty means "returns the preceding anonymous block if the trailing inline is empty". Or maybe just give aIgnoreEmpty a better name, perhaps something like "aReturnEmptyTrailingInline" and flip the meaning of the bit.

>+      // Figure out whether we're guaranteed this item will be out of flow.
>+      // This is not a precise test, since one of our ancestor inlines might
>+      // add an absolute containing block (if it's relatively positioned) when
>+      // there wasn't such a containining block before.  But it's conservative
>+      // in the sense that anything that will really end up as an in-flow
>+      // non-inline will test false here.  In other words, if this test is true
>+      // is true we're guaranteed to be inline; if its false we don't know what
>+      // we'll end up as.
>+      //
>+      // If we make this test precise, we can remove some of the code dealing
>+      // with the imprecision in ConstructInline and adjust the comments on
>+      // mIsAllInline and mIsBlock in the header.  And probably remove mIsBlock
>+      // ltogether, since then it will alwas be equal to !mHasInlineEnds.

s/containining block/containing block/
s/if this test is true is true/if this test is true/
s/if its false/if it's false/
s/remove mIsBlock ltogether/remove mIsBlock altogether/
s/alwas be equal/always be equal/

>+  if (IsFrameSpecial(aParentFrame)) {
>+    // We might be in a situation where the last part of the {ib} split was
>+    // empty.  Since we have no ::after pseudo-element, we do in fact want
>+    // to be appending to that last part, so advance to it if needed.
>+    nsIFrame* trailingInline = GetSpecialSibling(aParentFrame);
>+    if (trailingInline) {
>+      aParentFrame = trailingInline->GetLastContinuation();
>+    }
>+  }

It's a little bit opaque why GetSpecialSibling will get you the last special sibling. At least put a comment here that aParentFrame is the result of a GetLastSpecialSibling call (or similar), so its either the last or second last part. Maybe an assert if you think its worth it.

>@@ -5720,45 +5743,56 @@ nsCSSFrameConstructor::AppendFrames(nsFr

The comment above AppendFrames in both the .cpp and the .h needs to be updated with the changes to AppendFrames.

>+  // If we we're inserting a list of frames at the end of the trailing inline
>+  // of an {ib} split, we may need to create additional {ib} siblings to parent
>+  // them.
>+  if (!nextSibling && IsFrameSpecial(aParentFrame)) {
>+    // We want to put some of the frames into this inline frame.
>+    nsFrameList::FrameLinkEnumerator firstBlockEnumerator(aFrameList);
>+    FindFirstBlock(firstBlockEnumerator);
>+
>+    nsFrameList inlineKids = aFrameList.ExtractHead(firstBlockEnumerator);
>+    if (!inlineKids.IsEmpty()) {
>+      aState.mFrameManager->AppendFrames(aParentFrame, nsnull, inlineKids);
>+    }
>+
>+    if (!aFrameList.IsEmpty()) {
>+      const nsStyleDisplay* parentDisplay = aParentFrame->GetStyleDisplay();
>+      PRBool positioned =
>+        parentDisplay->mPosition == NS_STYLE_POSITION_RELATIVE ||
>+        parentDisplay->HasTransform();
>+      nsFrameItems ibSiblings;
>+      CreateIBSiblings(aState, aParentFrame, positioned, aFrameList,
>+                       aParentFrame->GetParent(), ibSiblings);
>+

At some point on the path containing AppendFrames -> CreateIBSiblings you need to get or save the first continuation of aParentFrame here, before you pass it to SetFrameIsSpecial. Because aParentFrame can have continuations on this path and if you don't you set the special sibling properties on something other than the first continuation and you lose. Also, the assertions in SetFrameIsSpecial that the frames can't have any continuations shouldn't be asserting in this case.

>+    // We're adding some kids to a block part of an {ib} split.  If all the
>+    // kids are blocks, we don't need to reconstuct.

s/reconstuct/reconstruct/

>--- /dev/null
>+++ b/layout/reftests/ib-split/whitespace-present-1-ref.html
>@@ -0,0 +1,17 @@
>+<!DOCTYPE html>
>+<html>
>+ <head>
>+   <style>
>+     body > span { border: 3px solid blue }
>+     .notstart { border-left: none; }
>+   .notend { border-right: none; }
>+   </style>
>+ </head>

Can you fix the weird indenting here?

There is also a comment at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.cpp#964 that needs to know that there can be more than one anonymous block.

And can you include a general comment about {ib} splits? Just something that says what they are, why they are needed, and how they are implemented. Just a couple paragraphs to give people a clue.

Comment 27

[Timothy Nikkel (:tnikkel)]

Comment on attachment 401485 [details] [diff] [review]
Part 6 merged to bug 233463

>+// XXXbz Since this is only used for {ib} splits, could we just copy the view
>+// bits from aOldParent to aNewParent and then use the ApplySetParent (or
>+// whatever it ends up being called in bug 233463 API to set the parent)?  That
>+// would still leave us doing two passes over the list, of course; if we really
>+// wanted to we could factor out the relevant part of ReparentFrameViewList, I
>+// suppose...  Or just get rid of views, which would make most of this function
>+// go away.

Bug 233463 has landed, so update this comment.

Comment 28

[Timothy Nikkel (:tnikkel)]

Comment on attachment 401479 [details] [diff] [review]
Part 1 merged to bug 233463.

>+static nsIFrame* GetSpecialPrevSibling(nsIFrame* aFrame)
>+{
>+  NS_PRECONDITION(IsFrameSpecial(aFrame), "Shouldn't call this");
>   
>   // We only store the "special sibling" annotation with the first
>   // frame in the continuation chain. Walk back to find that frame now.  
>   return
>     static_cast<nsIFrame*>
>     (aFrame->GetFirstContinuation()->
>        GetProperty(nsGkAtoms::IBSplitSpecialPrevSibling));
> }

GetSpecialPrevSibling does the same thing as GetSpecialSibling (except for the Prev part of course). Maybe make the code the same too?

> static void
> SetFrameIsSpecial(nsIFrame* aFrame, nsIFrame* aSpecialSibling)
> {
>   NS_PRECONDITION(aFrame, "bad args!");
> 
>-  // Mark the frame and all of its siblings as "special".
>-  for (nsIFrame* frame = aFrame; frame != nsnull; frame = frame->GetNextContinuation()) {
>-    frame->AddStateBits(NS_FRAME_IS_SPECIAL);
>-  }
>+  // We should be the only continuation
>+  NS_ASSERTION(!aFrame->GetPrevContinuation(),
>+               "assigning special sibling to other than first continuation!");
>+  NS_ASSERTION(!aFrame->GetNextContinuation(),
>+               "should have no continuations here");
>+
>+  // Mark the frame as "special".
>+  aFrame->AddStateBits(NS_FRAME_IS_SPECIAL);
> 
>   if (aSpecialSibling) {
>-    // We should be the first continuation
>-    NS_ASSERTION(!aFrame->GetPrevContinuation(),
>-                 "assigning special sibling to other than first continuation!");
>+    NS_ASSERTION(!aSpecialSibling->GetPrevContinuation(),
>+                 "assigning something other than the first continuation as the "
>+                 "special sibling");
> 
>     // Store the "special sibling" (if we were given one) with the
>     // first frame in the flow.
>     aFrame->SetProperty(nsGkAtoms::IBSplitSpecialSibling, aSpecialSibling);
>+    aSpecialSibling->SetProperty(nsGkAtoms::IBSplitSpecialPrevSibling, aFrame);
>   }
> }

Keeping in mind my comments about part 4, these asserts will have the net effect of asserting that all parts of the ib split except the last have no continuations, and that the last part passed in is the first continuation. Just for symmetry it seems like you'd want end up asserting the same things about each part. But from my comments about part 4 aFrame will have continuations some times, so figure out something good to assert here? :)

>+ * This function takes a "special" frame and _if_ that frame is an anonymous
>+ * block crated by an ib split it returns the blocks preceding inline.  This is
>+ * needed because the split inline's style context is the parent of the
>+ * anonymous block's style context.
>+ *
>+ * If aFrame is not the anonymous block, null is returned.
>+ */
>+static nsIFrame*
>+GetIBSpecialSiblingForAnonymousBlock(nsIFrame* aFrame)

s/crated/created/
s/returns the blocks preceding inline/returns the block's preceding inline/
The last "the anonymous block" should be "an anonymous block".

Can we call this GetIBSpecialPrevSiblingForAnonymousBlock since we consistently use sibling to mean next sibling everywhere else in the {ib} split code?

>       if (sibling) {
>-        // |parent| was the block in an {ib} split; use the inline as
>+        // |parent| was a block in an {ib} split; use the inline as
>         // |the style parent.
>         parent = sibling;
>       }

Can we s/use the inline as/use the preceding inline as/ for clarity?

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to comment #26)
> At some point on the path containing AppendFrames -> CreateIBSiblings you need
> to get or save the first continuation of aParentFrame here, before you pass it
> to SetFrameIsSpecial. Because aParentFrame can have continuations on this path
> and if you don't you set the special sibling properties on something other than
> the first continuation and you lose. Also, the assertions in SetFrameIsSpecial
> that the frames can't have any continuations shouldn't be asserting in this
> case.

If this wasn't caught by a test, can we get a test for this too?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Or maybe just give aIgnoreEmpty a better name, perhaps something like
> "aReturnEmptyTrailingInline"

Did that.

Fixed the typos in the big comment.

Added a comment about GetLastSpecialSibling in AdjustAppendParentForAfterContent.  

Updated the AppendFrames comments.

Good catch on the SetFrameSpecial assertions.  I've fixed that by changing nsCSSFrameConstructor::CreateIBSibling to initialize lastNewInline to aInitialInline->GetFirstContinuation(), and added a reftest that exercises this (and both asserts and fails without this change).  I've also changed the "!aFrame->GetNextContinuation()" assert in SetFrameIsSpecial to assert that either there is no next continuation or it already has the NS_FRAME_IS_SPECIAL bit set.

> s/reconstuct/reconstruct/

Fixed.

> Can you fix the weird indenting here?

Fixed.

> There is also a comment at
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.cpp#964

Fixed comment (rolled that fix into part 2, actually), and filed bug 520605 on fixing it.  Should be easy.

> And can you include a general comment about {ib} splits? Just something that
> says what they are, why they are needed, and how they are implemented. Just a
> couple paragraphs to give people a clue.

I'm not really sure where to put this sort of comment.  There's an existing comment in ConstructInline that's similar in spirit, though.

> Bug 233463 has landed, so update this comment.

Done.

> figure out something good to assert here?

Done, see above.

> s/crated/created/
> s/returns the blocks preceding inline/returns the block's preceding inline/
> The last "the anonymous block" should be "an anonymous block".

Fixed.

> Can we call this GetIBSpecialPrevSiblingForAnonymousBlock
> Can we s/use the inline as/use the preceding inline as/ for clarity?

I'm going to make this function's behavior match its name in bug 520605.  Will fix comments as needed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Maybe make the code the same too?

Oh, fixed.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 updated to comments

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 interdiff for review ease

Comment 34

[Timothy Nikkel (:tnikkel)]

Comment on attachment 404666 [details] [diff] [review]
Part 4 updated to comments

>+    // We might be in a situation where the last part of the {ib} split was
>+    // empty.  Since we have no ::after pseudo-element, we do in fact want to be
>+    // appending to that last part, so advance to it if needed.  Note that her
>+    // aParentFrame is the result of a GetLastSpecialSibling call, so must be
>+    // either the last or next to last special sibling.

s/her/here/

As for the general {ib} split comment, you can just expand the one in ConstructInline. Make it a bit more general (not just specific to what ConstructInline is doing) and actually use the words "{ib} split", and put a pointer to it from the definition of the NS_FRAME_IS_SPECIAL bit.

Comment 35

[Boris Zbarsky [:bzbarsky]]

How does this look for the general comment:

  // If an inline frame has non-inline kids, then we chop up the child list
  // into runs of blocks and runs of inlines, create wrapper block frames for
  // the runs of blocks, wrapper inline frames for the runs of inlines, and
  // make those wrappers siblings of our original inline.  The whole setup is
  // called an {ib} split; in what follows "frames in the split" refers to the
  // wrapper blocks and inlines (as well as the original inline).
  //
  // {ib} splits maintain the following invariants:
  // 1) All frames in the split have the NS_FRAME_IS_SPECIAL bit set.
  // 2) Each frame in the split has the nsGkAtoms::IBSplitSpecialSibling
  //    property pointing to the next frame in the split (possibly null).
  // 3) Each frame in the split has the nsGkAtoms::IBSplitSpecialPrevSibling
  //    property pointing to the next frame in the split (possibly null).
  // 4) The first and last frame in the split are always inlines.
  //
  // An invariant that is NOT maintained is that the wrappers are actually
  // linked via GetNextSibling linkage.  A simple example is an inline
  // containing an inline that contains a block.  The three parts of the inner
  // inline end up with three different parents.

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to comment #35)
>   // If an inline frame has non-inline kids, then we chop up the child list
>   // into runs of blocks and runs of inlines, create wrapper block frames for
>   // the runs of blocks, wrapper inline frames for the runs of inlines, and
>   // make those wrappers siblings of our original inline.  The whole setup is
>   // called an {ib} split; in what follows "frames in the split" refers to the
>   // wrapper blocks and inlines (as well as the original inline).

I don't really like calling them both wrapper frames, as the inline ones are actually based on content, and the anonymous blocks are created from thin air.

>   // {ib} splits maintain the following invariants:
>   // 1) All frames in the split have the NS_FRAME_IS_SPECIAL bit set.
>   // 2) Each frame in the split has the nsGkAtoms::IBSplitSpecialSibling
>   //    property pointing to the next frame in the split (possibly null).
>   // 3) Each frame in the split has the nsGkAtoms::IBSplitSpecialPrevSibling
>   //    property pointing to the next frame in the split (possibly null).

previous frame

Can we say that the first/last frames don't have one of the properties set instead of falsely saying that the property is null?

Feel free to include this example.

<span>
  text
  <div>block text</div>
  more text
</span>

Simplified frame tree:

Inline(span)   Anonymous block    Inline(span)
   text             div            more text
                 block text


Thanks for putting up with this request.

Comment 37

[Boris Zbarsky [:bzbarsky]]

OK, how about:

  // If an inline frame has non-inline kids, then we chop up the child list
  // into runs of blocks and runs of inlines, create anonymous block frames to
  // contain the runs of blocks, inline frames with our style context for the
  // runs of inlines, and put all these frames, in order, into aFrameItems.  We
  // put the first one int *aNewFrame.  The whole setup is called an {ib}
  // split; in what follows "frames in the split" refers to the anonymous blocks
  // and inlines that contain our children.
  //
  // {ib} splits maintain the following invariants:
  // 1) All frames in the split have the NS_FRAME_IS_SPECIAL bit set.
  // 2) Each frame in the split has the nsGkAtoms::IBSplitSpecialSibling
  //    property pointing to the next frame in the split, except for the last
  //    one, which does not have it set.
  // 3) Each frame in the split has the nsGkAtoms::IBSplitSpecialPrevSibling
  //    property pointing to the previous frame in the split, except for the
  //    first one, which does not have it set.
  // 4) The first and last frame in the split are always inlines.
  //
  // An invariant that is NOT maintained is that the wrappers are actually
  // linked via GetNextSibling linkage.  A simple example is an inline
  // containing an inline that contains a block.  The three parts of the inner
  // inline end up with three different parents.
  //
  // For example, this HTML:
  // <span>
  //   <div>a</div>
  //   <span>
  //     b
  //     <div>c</div>
  //   </span>
  //   d
  //   <div>e</div>
  //   f
  //  </span>
  // Gives the following frame tree:
  //
  // Inline (outer span)
  // Block (anonymous, outer span)
  //   Block (div)
  //     Text("a")
  // Inline (outer span)
  //   Inline (inner span)
  //     Text("b")
  // Block (anonymous, outer span)
  //   Block (anonymous, inner span)
  //     Block (div)
  //       Text("c")
  // Inline (outer span)
  //   Inline (inner span)
  //   Text("d")
  // Block (anonymous, outer span)
  //   Block (div)
  //     Text("e")
  // Inline (outer span)
  //   Text("f")

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to comment #37)
>   // put the first one int *aNewFrame.  The whole setup is called an {ib}

s/int/into/

Looks good, thanks!

Comment 39

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

Comment on attachment 404666 [details] [diff] [review]
Part 4 updated to comments

>+    // appending to that last part, so advance to it if needed.  Note that her

s/her/here/

>+  // If we we're inserting a list of frames at the end of the trailing inline
>+  // of an {ib} split, we may need to create additional {ib} siblings to parent
>+  // them.

Why is this specific to inserting at the end of the trailing inline of
an {ib} split?

Are you assuming ConstructInline handles the other case?  But what about
appending something with blocks to an already existing inline?

Should CreateIBSibling assert something about the relationship between
aInitialInline and aParentFrame.  In particular, the line:
>+    if (blockFrame->HasView() || aInitialInline->HasView()) {
seems to assume that they're child and parent, but I'm not sure that's
the case.  (Applies to MoveFramesToEndOfIBSplit as well.)

r=dbaron, and sorry for the delay

Comment 40

[Boris Zbarsky [:bzbarsky]]

> Why is this specific to inserting at the end of the trailing inline of
> an {ib} split?

Because this is AppendFrames, and we guarantee that there's a trailing inline in {ib} splits and that it's the parent used for the cases that call AppendFrames.

> But what about appending something with blocks to an already existing inline?

That's precisely when we need to create additional {ib} siblings, yes.  That's assuming that the inline didn't get blown away via WipeContainingBlock, of course; that handles all the "hard" cases.  I've been considering making more cases avoid the reconstruct, as a followup...

> Should CreateIBSibling assert something about the relationship between
> aInitialInline and aParentFrame.

aParentFrame must be the parent frame of aInitialInline.  This is in fact guaranteed at the moment.  The only two callers are ConstructInline (which passes newFrame and aParentFrame, where the former has been initialized with the latter as parent) and AppendFrames, which passes aParentFrame and aParentFrame->GetParent().

Given that, I've just removed the aParentFrame argument of CreateIBSiblings and made it get the parent from aInitialInline.

> (Applies to MoveFramesToEndOfIBSplit as well.)

The function this code moves to by the end of this patch series (MoveChildrenTo)
has the following at the beginning as of the last patch in this bug:

+  NS_PRECONDITION(aOldParent->GetParent() == aNewParent->GetParent(),
+                  "Unexpected old and new parents");

which is basically asserting what you ask for.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/35ba5ef18778
http://hg.mozilla.org/mozilla-central/rev/61febc552982
http://hg.mozilla.org/mozilla-central/rev/ce946663a30a
http://hg.mozilla.org/mozilla-central/rev/822ba7ef29bb
http://hg.mozilla.org/mozilla-central/rev/5bc1c85439bf
http://hg.mozilla.org/mozilla-central/rev/0f12f3edad42