480979 - [FIX]Rework {ib} frame construction to not require destroying newly-created frames

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Several patches coming up.  The basic idea is to introduce a struct called FrameConstructionItem, which encapsulates all the information needed to create a frame except for the parent frame.  This means not just the FrameConstructionData but also the content, style context, tag, namespace, and some booleans that affect the behavior.  Then frame construction becomes a three-pass process.  First build up the list of FrameConstructionItems to construct, then do whatever post-processing is desired on this list, then construct the frames.  To handle {ib} splits, we actually construct not just a list of the FrameConstructionItems but a list of trees, with the only non-leaf nodes being FrameConstructionItems for inlines and positioned inlines.  This allows us to propagate state like "contains a block" up the tree.

For now I left most of ConstructInline alone: it still constructs all its frames and then reparents as needed.  I think we can change this, but it'll take a bit more thought and I was more interested in getting rid of DestroyNewlyCreatedFrames in any case.

The patches for this bug depend on the patch for bug 480323.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1: introduce FrameConstructionItem

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2: Pass FrameConstructionItems to FrameFullConstructors

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3: Move around column/colgroup child suppression so that CreateFramesFromItem always does

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4: Separate items for page-break frames

I considered using a FrameFullConstructor here instead, but this seems to be good enough.  These are no different from various other leaf frames we have, really.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5: Actually create a tree of FrameConstructionItems under inlines

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 (and last): Get rid of DestroyNewlyCreatedFrames

This adds some duplicated information about which frames are line participants...  Not sure what I can do about that so far.  :(

Comment 7

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

Comment on attachment 364937 [details] [diff] [review]
Part 1: introduce FrameConstructionItem

   PRBool                    mFixedPosIsAbsPos;
 
+  // A boolean to indicate whether we have a "pending" popupgroup.  That is, we
+  // have already created the FrameConstructionItem for the root popupgroup but
+  // we have not yet created the relevant frame.
+  PRBool                    mHavePendingPopupgroup;

PRPackedBools?

+  AddFrameConstructionItemInternal(aState, container, aParentFrame, elemName,
+                                   kNameSpaceID_None, pseudoStyleContext,
+                                   PR_FALSE, PR_FALSE, PR_TRUE, aItems);

Can we use a flags word instead of three mysterious boolean parameters?

Seems like you could simplify nsCSSFrameConstructor::ProcessChildren and make the creation order of the anonymous frames consistent for root frame vs non-root frame by always putting the anonymous frames before the regular frames. That should work for nsGfxScrollFrame and obviously nsDocElementBoxFrame ... are there any other frames that have both regular kids and anonymous kids?

Comment 8

[Boris Zbarsky [:bzbarsky]]

>PRPackedBools?
>Can we use a flags word instead of three mysterious boolean parameters?

Sure on both counts.

> always putting the anonymous frames before the regular frames.

Hmm... I had semi-assumed there was a reason for the behavior.  Bad call on my part.  I can do it either as a prerequisite to this bug or as a followup, but I'd like to do it as a separate change so I can track regressions from it separately.

> are there any other frames that have both regular kids and anonymous kids?

Combobox, sort of, but it's not going through this codepath anyway.  Does <svg:use> show DOM kids in any way?  It doesn't seem to be IsLeaf(), but maybe that's just a bug?  That seems to be it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, <use> is just bug 481045.  So yeah, as long as scrollframe doesn't care about the order of its kids, this should be fine.

Comment 10

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

OK, r+sr with those changes and assuming we'll put the anonymous frames before the regular frames under a separate bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Filed bug 481105 on the anonymous frames issue.

Comment 12

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

Comment on attachment 364941 [details] [diff] [review]
Part 5: Actually create a tree of FrameConstructionItems under inlines

+  // aParentFrame may be null; this method doesn't use it directly in any case.
   void CreateGeneratedContentItem(nsFrameConstructorState& aState,
                                   nsIFrame*                aFrame,
                                   nsIContent*              aContent,
                                   nsStyleContext*          aStyleContext,
                                   nsIAtom*                 aPseudoElement,
                                   nsTArray<FrameConstructionItem>& aItems);

aParentFrame isn't a parameter here

+  void GetInlineChildItems(nsFrameConstructorState& aState,
+                           FrameConstructionItem& aParentItem);

Maybe "BuildInlineChildItems" is a better name here?

+  /**
+   * Process the indicated range of aItems, and put the resulting frames in
+   * aFrameItems.  This function will save pseudoframes on entry and restore on
+   * exit.
+   */
+  nsresult ProcessItems(nsFrameConstructorState& aState,
+                        nsTArray<FrameConstructionItem>& aItems,
+                        PRUint32 aStart,
+                        PRUint32 aEnd,
+                        nsIFrame* aParentFrame,
+                        nsFrameItems& aFrameItems);

'Process' isn't very descriptive. Maybe "ConstructFramesFromItemSublist" or something?

+        (!aParentFrame || // Parent is inline

I guess you should document somewhere that nsCSSFrameConstructor::AddFrameConstructionItems aParent==nsnull means that the context is inline.

+      // Things that we're guaranteed will end up out-of-flow are inline.  This
+      // is not a precise test, since one of our ancestor inlines might add an
+      // absolute containing block (if it's relatively positioned) or float
+      // containing block (the latter if it gets split by child blocks on both
+      // sides of us).  But it's conservative in the sense that anything that
+      // will really end up as an in-flow non-inline will have false
+      // mIsAllInline.  It just might be that even an inline that has
+      // mIsAllInline false doesn't need an {ib} split.  So this is just an
+      // optimization to keep from doint to much work when that happens.

I think you're referring to situations where there is no abs-pos containing block or no float containing block and an ancestor inline introduces one. You might want to mention that explicitly.

+  // XXXbz should we preallocate aParentItem.mChildItems to some sane
+  // length?  Maybe even to parentContent->GetChildCount()?

If we used a linked list of items and a good arena allocator, that would take care of things.

+    // Manually resolve the style context too.  Would be nice to share this
+    // code with ResolveStyleContext.
+    nsRefPtr<nsStyleContext> childContext =
+      ResolveStyleContext(parentStyleContext, content);

I'm confused, what is the comment referring to?

+  for (PRUint32 i = 0, count = aParentItem.mChildItems.Length();
+       i < count; ++i) {
+    if (!aParentItem.mChildItems[i].mIsAllInline) {
+      aParentItem.mIsAllInline = PR_FALSE;
+      break;
+    }
+  }

I wonder if it's worth avoiding this loop by letting CreateGeneratedContentItem and AddFrameConstructionItemsInternal take an out-parameter to set mIsAllInline directly.

Comment 13

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

Comment on attachment 364942 [details] [diff] [review]
Part 6 (and last): Get rid of DestroyNewlyCreatedFrames

+PRBool
+nsCSSFrameConstructor::AnyItemsNeedBlockParent(const nsTArray<FrameConstructionItem>& aItems)
+{
+  for (PRUint32 i = 0, count = aItems.Length(); i < count; ++i) {
+    if (aItems[i].mFCData->mBits & FCDATA_IS_LINE_PARTICIPANT) {
+      return PR_TRUE;
+    }
+  }
+
+  return PR_FALSE;
+}

Hmm, so if we had a FrameConstructionItemList, it could track "all children have mIsAllInline" and "all children have IS_LINE_PARTICIPANT" itself and avoid this loop, the loop I just mentioned in comment #12, and AreAllItemsInline. If it was "number of children with mIsAllInline" and "number of children with IS_LINE_PARTICIPANT", we could even handle deletes from the list efficiently.

Can we assert somewhere that FCDATA_IS_LINE_PARTICIPANT matches IsFrameOfType(eLineParticipant) for the frame we actually construct?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> aParentFrame isn't a parameter here

Indeed. Should have been aFrame.

> Maybe "BuildInlineChildItems" is a better name here?

Yep. Renamed.

> Maybe "ConstructFramesFromItemSublist" or something?

Yeah, makes sense.

> I guess you should document somewhere that
> nsCSSFrameConstructor::AddFrameConstructionItems aParent==nsnull means that
> the context is inline.

It should never be null in AddFrameConstructionItems.  I did document it for AddFrameConstructionItemsInternal, which is what BuildInlineChildItems calls directly.

> I think you're referring to situations where there is no abs-pos containing
> block or no float containing block and an ancestor inline introduces one.

Yeah.  Will make that clear.

> I'm confused, what is the comment referring to?

Code that's no longer there in this version of the patch.  ;)  Removed it.

I'll work on a FrameConstructionItemList class with an iterator to address the other comments, and will post an interdiff of that vs the state with the patches above applied plus all the comments mentioned above resolved.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Or rather, I'll just add a part 7 that implements such a list.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch to flag inline mathml as line participants

> Can we assert somewhere that FCDATA_IS_LINE_PARTICIPANT matches
> IsFrameOfType(eLineParticipant) for the frame we actually construct?

Oh, almost forgot.  Yes, we can.  I've added such an assert to ConstructFrameFromItemInternal right before we do the primary frame map stuff.  All frames created via FrameConstructionData go through here.  It caught the fact that <mathml:math style="display: inline"> gets a frame that inherits from nsInlineFrame.  This patch fixes the fcdata bits for that case.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7. FrameConstructionItemList.

I'll file a followup bug on using arena allocation for the items.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed this morning:

http://hg.mozilla.org/mozilla-central/rev/623d7d25bb2a
http://hg.mozilla.org/mozilla-central/rev/026d36aa792c
http://hg.mozilla.org/mozilla-central/rev/5610ee9433dd
http://hg.mozilla.org/mozilla-central/rev/8abc0e906ed8
http://hg.mozilla.org/mozilla-central/rev/e6fb3c024356
http://hg.mozilla.org/mozilla-central/rev/159168c61b02
http://hg.mozilla.org/mozilla-central/rev/fb254bab5a82

No unit test or talos issues that I can see.  Filed bug 481788 on arena allocation.