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

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

49.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
63.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
24.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
42.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
32.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
Created attachment 364937 [details] [diff] [review]
Part 1: introduce FrameConstructionItem
Attachment #364937 - Flags: superreview?(roc)
Attachment #364937 - Flags: review?(roc)
Created attachment 364938 [details] [diff] [review]
Part 2: Pass FrameConstructionItems to FrameFullConstructors
Attachment #364938 - Flags: superreview?(roc)
Attachment #364938 - Flags: review?(roc)
Created attachment 364939 [details] [diff] [review]
Part 3: Move around column/colgroup child suppression so that CreateFramesFromItem always does
Attachment #364939 - Flags: superreview?(roc)
Attachment #364939 - Flags: review?(roc)
Created attachment 364940 [details] [diff] [review]
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.
Attachment #364940 - Flags: superreview?(roc)
Attachment #364940 - Flags: review?(roc)
Created attachment 364941 [details] [diff] [review]
Part 5: Actually create a tree of FrameConstructionItems under inlines
Attachment #364941 - Flags: superreview?(roc)
Attachment #364941 - Flags: review?(roc)
Created attachment 364942 [details] [diff] [review]
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.  :(
Attachment #364942 - Flags: superreview?(roc)
Attachment #364942 - Flags: review?(roc)
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?
>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.
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.
Attachment #364937 - Flags: superreview?(roc)
Attachment #364937 - Flags: superreview+
Attachment #364937 - Flags: review?(roc)
Attachment #364937 - Flags: review+
OK, r+sr with those changes and assuming we'll put the anonymous frames before the regular frames under a separate bug.
Attachment #364938 - Flags: superreview?(roc)
Attachment #364938 - Flags: superreview+
Attachment #364938 - Flags: review?(roc)
Attachment #364938 - Flags: review+
Attachment #364939 - Flags: superreview?(roc)
Attachment #364939 - Flags: superreview+
Attachment #364939 - Flags: review?(roc)
Attachment #364939 - Flags: review+
Attachment #364940 - Flags: superreview?(roc)
Attachment #364940 - Flags: superreview+
Attachment #364940 - Flags: review?(roc)
Attachment #364940 - Flags: review+
Filed bug 481105 on the anonymous frames issue.
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.
Attachment #364941 - Flags: superreview?(roc)
Attachment #364941 - Flags: superreview+
Attachment #364941 - Flags: review?(roc)
Attachment #364941 - Flags: review+
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?
Attachment #364942 - Flags: superreview?(roc)
Attachment #364942 - Flags: superreview+
Attachment #364942 - Flags: review?(roc)
Attachment #364942 - Flags: review+
> 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.
Or rather, I'll just add a part 7 that implements such a list.
Created attachment 365127 [details] [diff] [review]
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.
Attachment #365127 - Flags: review?(roc)
Created attachment 365373 [details] [diff] [review]
Part 7.  FrameConstructionItemList.

I'll file a followup bug on using arena allocation for the items.
Attachment #365373 - Flags: superreview?(roc)
Attachment #365373 - Flags: review?(roc)
Attachment #365373 - Flags: superreview?(roc)
Attachment #365373 - Flags: superreview+
Attachment #365373 - Flags: review?(roc)
Attachment #365373 - Flags: review+

Updated

10 years ago
Depends on: 481769

Updated

10 years ago
No longer depends on: 481769

Updated

10 years ago
Depends on: 482398
You need to log in before you can comment on or make changes to this bug.