Closed
Bug 501847
Opened 15 years ago
Closed 15 years ago
Handle appends to the trailing inline of an {ib} split better
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(8 files, 8 obsolete files)
292.27 KB,
application/zip
|
Details | |
13.44 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
12.52 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
5.57 KB,
patch
|
tnikkel
:
review+
roc
:
review+
|
Details | Diff | Splinter Review |
9.92 KB,
patch
|
roc
:
review+
tnikkel
:
review+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
83.18 KB,
patch
|
tnikkel
:
review+
dbaron
:
review+
|
Details | Diff | Splinter Review |
17.72 KB,
patch
|
Details | Diff | Splinter Review |
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....
Assignee | ||
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
Assignee | ||
Comment 3•15 years ago
|
||
I welcome suggestions for better names for the nsLayoutUtils functions.
Attachment #399272 -
Flags: superreview?(roc)
Attachment #399272 -
Flags: review?(tnikkel)
Assignee | ||
Updated•15 years ago
|
Attachment #399272 -
Attachment description: Fix assumptions consumers make about the number of ib splits → Part 2. Fix assumptions consumers make about the number of ib splits
(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?
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #399273 -
Flags: review?(dbaron)
Assignee | ||
Comment 6•15 years ago
|
||
> 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.
Assignee | ||
Comment 7•15 years ago
|
||
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.
Attachment #399276 -
Flags: review?(tnikkel)
Attachment #399276 -
Flags: review?(roc)
Attachment #399276 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•15 years ago
|
||
Attachment #399277 -
Flags: review?(tnikkel)
Attachment #399277 -
Flags: review?(roc)
Assignee | ||
Comment 9•15 years ago
|
||
Attachment #399278 -
Flags: review?(tnikkel)
Attachment #399278 -
Flags: review?(roc)
Assignee | ||
Comment 10•15 years ago
|
||
> 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.
Assignee | ||
Comment 11•15 years ago
|
||
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.
I agree.
Updated•15 years ago
|
Attachment #399272 -
Flags: review?(tnikkel) → review+
Comment 13•15 years ago
|
||
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..."
Assignee | ||
Comment 14•15 years ago
|
||
> 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.
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #399271 -
Attachment is obsolete: true
Attachment #401479 -
Flags: review?(roc)
Attachment #399271 -
Flags: review?(roc)
Assignee | ||
Comment 16•15 years ago
|
||
Attachment #399272 -
Attachment is obsolete: true
Attachment #401480 -
Flags: superreview?(roc)
Attachment #399272 -
Flags: superreview?(roc)
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #399273 -
Attachment is obsolete: true
Attachment #401482 -
Flags: review?(dbaron)
Attachment #399273 -
Flags: review?(dbaron)
Assignee | ||
Comment 18•15 years ago
|
||
Attachment #399276 -
Attachment is obsolete: true
Attachment #401483 -
Flags: review?(tnikkel)
Attachment #401483 -
Flags: review?(roc)
Attachment #401483 -
Flags: review?(dbaron)
Attachment #399276 -
Flags: review?(tnikkel)
Attachment #399276 -
Flags: review?(roc)
Attachment #399276 -
Flags: review?(dbaron)
Assignee | ||
Comment 19•15 years ago
|
||
Attachment #401484 -
Flags: review?(tnikkel)
Attachment #401484 -
Flags: review?(roc)
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #399277 -
Attachment is obsolete: true
Attachment #399278 -
Attachment is obsolete: true
Attachment #401485 -
Flags: review?(tnikkel)
Attachment #401485 -
Flags: review?(roc)
Attachment #399277 -
Flags: review?(tnikkel)
Attachment #399277 -
Flags: review?(roc)
Attachment #399278 -
Flags: review?(tnikkel)
Attachment #399278 -
Flags: review?(roc)
Attachment #401479 -
Flags: review?(roc) → review+
Attachment #401480 -
Flags: superreview?(roc) → superreview+
Attachment #401483 -
Flags: review?(roc) → review+
Attachment #401484 -
Flags: review?(roc) → review+
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
Attachment #401485 -
Flags: review?(roc) → review+
Attachment #401482 -
Flags: review?(dbaron) → review-
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.
Assignee | ||
Comment 23•15 years ago
|
||
> This is pretty obviously wrong for RTL.
Good catch! This patch has that fixed, plus tests that would have caught the problem.
Attachment #401482 -
Attachment is obsolete: true
Attachment #403955 -
Flags: review?(dbaron)
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().
Attachment #403955 -
Flags: review?(dbaron) → review+
Attachment #403955 -
Attachment description: Part updated to comments → Part 3 updated to comments
Assignee | ||
Comment 25•15 years ago
|
||
Eliminated the ltr local.
Comment 26•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #401484 -
Flags: review?(tnikkel) → review+
Updated•15 years ago
|
Attachment #401485 -
Flags: review?(tnikkel) → review+
Comment 27•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
(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?
Assignee | ||
Comment 30•15 years ago
|
||
> 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.
Assignee | ||
Comment 31•15 years ago
|
||
> Maybe make the code the same too?
Oh, fixed.
Assignee | ||
Comment 32•15 years ago
|
||
Attachment #401483 -
Attachment is obsolete: true
Attachment #404666 -
Flags: review?(tnikkel)
Attachment #404666 -
Flags: review?(dbaron)
Attachment #401483 -
Flags: review?(tnikkel)
Attachment #401483 -
Flags: review?(dbaron)
Assignee | ||
Comment 33•15 years ago
|
||
Comment 34•15 years ago
|
||
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.
Attachment #404666 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 35•15 years ago
|
||
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•15 years ago
|
||
(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.
Assignee | ||
Comment 37•15 years ago
|
||
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•15 years ago
|
||
(In reply to comment #37) > // put the first one int *aNewFrame. The whole setup is called an {ib} s/int/into/ Looks good, thanks!
Attachment #404666 -
Flags: review?(dbaron) → review+
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
Assignee | ||
Comment 40•15 years ago
|
||
> 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.
Assignee | ||
Comment 41•15 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 526596
Assignee | ||
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.3a1
You need to log in
before you can comment on or make changes to this bug.
Description
•