Webkit does it; they needed it for the Acid3 perf thing. And it shows up in lots of other benchmarks too... And maybe even in some real-life web pages.

So if I read things correctly, what webkit does is that it does lazy attach() calls if the DOM appendChild/insertBefore/replaceChild calls are made from JS. When this is done, all ancestors are flagged as needing a style recalculation and one is scheduled. It looks like that restyle then does the equivalent of ContentRemoved/ContentInserted on the node. There's no batching that I can see; I'm not quite sure where they handle table anonymous kids here. That somewhat matches what dbaron and I talked about. Basic idea we were thinking of: 1) ContentRemoved should eagerly remove frames. 2) ContentInserted/Appended should flag parent as needing child frames created (and for the latter maybe the starting index). 3) When we go to create frames, don't do it for nodes which already have a frame. A prerequisite here is having a fast way to tell whether a node has a frame (bug 500882). There's still the display:none issue, which webkit solves by just storing the style in nodes...

What exactly is the display:none issue? I could see this being a performance win for real scripts that append a lot of nodes to the same parent.

> What exactly is the display:none issue? The fact that if a node has display:none we'll keep trying to construct frames for it over and over again in the above setup if we just construct for nodes with no frames. I guess we can optimize that away using the undisplayed map... So maybe there isn't an issue here. I agree that there could be a real performance win here from the batching in Gecko. It's just webkit that doesn't do batching; I don't think we should emulate them on that point.

This would be a great step forward for our DHTML performance, as I understand it, and it sounds like we need it for acid3. If that's reason enough to do SMIL, I think it's gotta be reason enough to prioritize something that will affect actual web content too. :)

To what extent can we just extend the approach from http://hg.mozilla.org/mozilla-central/rev/736ada2d64bf , and just do the same thing for ContentInserted, ContentAppended, ContentReplaced when called from JS? Since we won't be adding to the undisplayed map, further mutations to the same node will find that we have neither a frame nor an undisplayed map entry and determine that no additional work needs to be done.

That's the easy-and-quick way to do it, imo. It wouldn't get us the batching thing, but it would certainly get us async behavior. I think roc wanted to see whether we can get batching to work.

What sort of batching is it that you want that that wouldn't get us?

I'm not sure what Boris meant...

If you insert N content nodes all under the same parent and we just use our existing restyle stuff to post reconstruct (or better yet "create frame if not there") hints for all of them, which is what I assume comment 5 suggests, then we'll end up processing the frame creations for them one at a time. I think this is in fact not that difficult to do; I offered to do it back in September or so. It would be much faster, given how frame construction is set up, to create a single frame construction item list for the N nodes if we can and then process it. That involves more complicated bookkeeping to figure out when we can and can't do that, however. Last I checked, Robert was in favor of trying to do this.

Ah, that's what you meant. Yeah.

I needed to change test_bug398135.xhtml to xul because bindings get attached at frame construction time and I used items 1 and 2 of bug 514120, comment 1 to make frame construction non-lazy for XUL and in chrome. browser_463205.js was depending on the order in which load (or domcontentedloaded) events get dispatched.

NODE_SCRIPT_TYPE_OFFSET didn't get updated when the primary frame map got removed, so that is why it is only incremented by 1.

I left the existing reconstruct frame stuff in the restyle path unchanged due to the proposal to change the restyle mechanism in bug 479655.

I'm open to a better name for ContentRangeInserted (and a few other things could use better names). This patch was a bit bigger than I wanted, but I couldn't think of a good way to split it.

This is a very minimal amount of tests, but I wasn't sure what else to write.

Comment on attachment 420514 [details] [diff] [review] Part 3. Implement lazy frame construction. >+PRBool >+nsCSSFrameConstructor::MaybeContructLazily(Operation aOperation, s/Contruct/Construct/ (there's about four or so places that need that fixed)

Comment on attachment 420510 [details] [diff] [review] Part 1. Fix up some tests. Please get one of the sessionstore folks to review that test? Why are the 290018 test changes needed?

As far as part 2 + 3 goes, is there a writeup of the overall idea somewhere? Similar for part 4. Also, why bother removing the flags on node removal?

(In reply to comment #17) > Why are the 290018 test changes needed? I'm not totally sure, I just added stuff to make it wait until the test was finished so it would pass.

(In reply to comment #18) > As far as part 2 + 3 goes, is there a writeup of the overall idea somewhere? > Similar for part 4. I will write something up, an oversight on my part. > Also, why bother removing the flags on node removal? That is a good question, I was thinking yesterday about even leaving the bits on nodes still in the tree that are only set because of the previous insert for the removed node and let those bits get cleared when we next process frame creation. I will think more on this as it is a potential slow spot.

> I just added stuff to make it wait until the test was finished so it would pass That's what confuses me. Why is any waiting needed at all? The onload event for that outer document should not be firing until everything is fully loaded and ready. If it _is_ that sounds like a bug to me.

(In reply to comment #21) > That's what confuses me. Why is any waiting needed at all? The onload event > for that outer document should not be firing until everything is fully loaded > and ready. If it _is_ that sounds like a bug to me. Ok, I can look into it more closely.

Comment on attachment 420513 [details] [diff] [review] Part 2. Add bits to nsINode.h. sr=me, But definitely want someone from layout to review that this is the right approach, since I have no opinion on that.

This stack explains why the 290018 test fails. This stack happens when the iframe has be notified on and marked for lazy construction, then we start running the script and call document.open(). nsHTMLDocument::OpenCommon calls Stop on the docshell. This in turn calls Cancel on the loadgroup, and when the last request is removed from loadgroup in Cancel nsDocLoader::DocLoaderIsEmpty sees that it has no more requests so it flushes layout to see if it gets any more requests. This flush creates the frame for the iframe and it adds some XBL constructors for scrollbar stuff to the attached queue. This calls BlockOnload on the document so the constructors get run before onload. BlockOnload tries to add the onload blocker but the loadgroup is still canceling so it rejects the add. Then later on when we try to block onload the mOnloadBlockCount is nonzero so we don't add the onload blocker to the loadgroup, and onload fires too early. Adding a bool to nsDocument that keeps track of the onload blocker request being successfully added to the loadgroup instead of using mOnloadBlockCount for that purpose fixes the problem.

(In reply to comment #16) > s/Contruct/Construct/ Thanks, fixed in my tree.

I think that the condition of comment 18, where we call Cancel on the document's loadgroup but still want to do a load in that same document is unique to document.open(). So just ensure that the onload blocker is in the loadgroup if we have a non-zero onload block count after we call Stop. Are there any other places where something like this might happen?

Part 0 fixes the 290018 test. The sessionstore test now passes, I don't know what changed on trunk to have fixed this.

Just changed the comment about lazy bits only being set on nodes in documents because we no longer unset the bits on removal.

Update so that it no longer unsets lazy bits on removal. Added a large comment outlining how lazy frame construction works. Clear lazy bits in BindToTree (thanks to bz for suggesting this). Walks the XBL child list in CreateNeededFrames.

Added a comment explaining how ContentRangeInserted works.

Comment on attachment 422180 [details] [diff] [review] Part 0. Make sure onload blocker is in place with document.open(). To be safe, you should probably enumerate the loadgroup to make sure that mOnloadBlocker isn't in it already. r=bzbarsky with that.

Comment on attachment 422181 [details] [diff] [review] Part 1 v2. Fix up a test. r=bzbarsky

Comment on attachment 422182 [details] [diff] [review] Part 2 v2. Add bits to nsINode.h. This looks ok, but the comments should probably make it clear that NODE_DESCANDANTS_NEED_FRAMES means that flattened tree descendants need frames, and that in particular the flag needs to be set up the flattened tree parent chain, not just the DOM parent chain.

> set up the flattened tree parent chain Note that we'll need to traverse that chain in bug 479655 too. So I think we should add a method on nsIContent (slid as a patch in under part 3 somewhere, or just a separate bug blocking this one) that returns the flattened tree parent. Basically GetParent() unless the return value of GetParent() has NODE_MAY_BE_IN_BINDING_MNGR set, in which case we should do the look-for-insertion-points dance.

Comment on attachment 422185 [details] [diff] [review] Part 3 v2. Implement lazy frame construction. >+++ b/content/base/src/nsGenericElement.cpp >@@ -2550,17 +2550,19 @@ nsGenericElement::BindToTree(nsIDocument >+ UnsetFlags(NODE_FORCE_XBL_BINDINGS | >+ // And clear the lazy frame construction bits. >+ NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES); I'd prefer that comment indented to line up with the NODE_NEEDS_FRAME. >+++ b/layout/base/nsCSSFrameConstructor.cpp >+nsCSSFrameConstructor::MaybeConstructLazily(Operation aOperation, > + if (mPresShell->GetPresContext()->IsChrome() || !aContainer || Hmm. IsChrome() is not that cheap an operation, actually. Can you file a followup bug to cache the result in the prescontext? >+ if (aOperation == CONTENTINSERT) { >+ if (!aChild || aIndex < 0 || aContainer->GetChildAt(aIndex) != aChild || How could !aChild be true here? >+ } else { // CONTENTAPPEND Assert that? >+ // We can construct lazily, just need to set suitable bits in the content >+ // tree. Semicolon needed there, not a comma. >+ // Walk up the tree setting the NODE_DESCENDANTS_NEED_FRAMES bit as we go. If >+ // we hit a node with NODE_NEEDS_FRAME set then we don't need to bother >+ // setting the bits on the new nodes or posting a restyle event. Can we not also stop if we hit a node with no frame? Either it'll get a frame created at some point (and then create them for all its descendants) or it won't (and then we don't need a frame either). In particular, any node with NODE_NEEDS_FRAME would also have no frame, so that test would subsume this one. That said, it looks to me like the callsites already check whether the parent has a frame. So I would expect us to never hit a NODE_NEEDS_FRAME or frameless node during this traversal. Do we hit such nodes? >+ content = content->GetParent(); This should be getting the flattened tree parent, per our earlier discussion. >+static void >+ClearLazyBitsInChildren(nsIContent* aContent, >+ PRUint32 aStartIndex = 1, >+ PRUint32 aEndIndex = 0) >+{ >+ PRBool allChildren = aEndIndex < aStartIndex; Why do we have the default args and the whole allChildren thing? The only caller passes both args and passes aStartIndex <= aEndIndex. >+ for (PRUint32 i = startIndex; i < endIndex; i++) { >+ aContent->GetChildAt(i)-> >+ UnsetFlags(NODE_DESCENDANTS_NEED_FRAMES | NODE_NEEDS_FRAME); >+ } It's almost tempting to say that this should use GetChildArray. Not sure it matters much in practice, though... >+nsCSSFrameConstructor::CreateNeededFrames(nsIContent* aContent) >+ aContent->UnsetFlags(NODE_DESCENDANTS_NEED_FRAMES); Assert that the bit is set before that? >+ //xxx should we descend first (on nodes that don't have NODE_NEEDS_FRAME set) >+ // or issue content notifications for our kids first? I think the latter is better in terms of triggering WipeContainingBlock badness in a not-as-bad way usually, but I'm not 100% sure. One can probably write testcases that favor one or the other.... >+ //xxx should we use GetChildArray and scan it completely first and then issue >+ // all notifications? Another question. Why would you need to scan it completely? Can these notifications change the DOM tree? >+ nsIContent* firstChildInRun = nsnull; This is write-only. Ditch it? >+//xxx should this use a non-recursive method? >+void nsCSSFrameConstructor::CreateNeededFrames() That seems like it would be difficult without some heap-allocation, right? So much of frame construction is already recursive that I wouldn't worry about it. > nsCSSFrameConstructor::ContentAppended(nsIContent* aContainer, >- PRInt32 aNewIndexInContainer) >+ PRInt32 aNewIndexInContainer, >+ PRBool aLazyConstruction) That new arg should probably be called aAllowLazyConstruction or something. It just allows it; it doesn't force it. >@@ -6296,18 +6466,19 @@ nsCSSFrameConstructor::ContentAppended(n >+ child->UnsetFlags(NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES); I guess |child| could have NODE_DESCENDANTS_NEED_FRAMES if restyle frame reconstructs are happening on ancestors of things that had NODE_NEEDS_FRAME, right? Would it be better to just push this line of code down into the very beginning of AddFrameConstructionItems instead? We'd still need the explicit call in BuildInlineChildItems, but that should cover all the other callsites. The extra cost of making those calls on native anon content is not that big a deal compared to the more maintainable code, I think. >+nsCSSFrameConstructor::PostRestyleEventInternal(PRBool aForLazyConstruction) >+ if (!mObservingRefreshDriver && (!mInStyleRefresh || >+ (aForLazyConstruction && !mInLazyFCRefresh))) { Document that the correctness of this test (or at least its correct implementation of the optimization I think it's trying to implement) depends on the order in which CreateNeededFrames and ProcessPendingRestyles get called, please? And document in the header that CreateNeededFrames must be called before ProcessPendingRestyles? >+++ b/layout/base/nsCSSFrameConstructor.h >+ * Lazy frame construction is controlled by the aLazyConstruction bool aAllowLazyConstruction. Or aMayBeLazy, maybe? >+ * If MaybeConstructLazily returns false we construct as usual, but if it >+ * returns true then it adds NODE_NEEDS_FRAME bits to the newly >+ * inserted/appended nodes and adds NODE_DESCENDANTS_NEED_FRAMES bits to the >+ * container and up along the parent chain until it hits the root or another >+ * node with one of those bits set. This comment might need changing depending on whether we leave the NODE_NEEDS_FRAME stuff in MaybeConstructLazily. >+ (We do clear the bits nsGenericElement::BindToTree though.) "bits in nsGenericElement::BindToTree" r=bzbarsky with the above nits fixed.

Comment on attachment 422186 [details] [diff] [review] Part 4 v2. Implement ContentRangeInserted. >+++ b/layout/base/nsCSSFrameConstructor.cpp > nsCSSFrameConstructor::GetInsertionPrevSibling(nsIFrame*& aParentFrame, >+ PRInt32 aStartSkipIndexInContainer, >+ nsIContent* aStartSkipChild, >+ PRInt32 aEndSkipIndexInContainer, >+ nsIContent *aEndSkipChild) So aStartSkipChild is non-null if and only if aStartSkipIndexInContainer >= 0, and likewise for aEndSkipChild, right? Can we assert whatever has to be true here? Oh and aStartSkipIndexInContainer and aEndSkipIndexInContainer must either both be -1 or both be >= 0. And in the latter case, aEndSkipIndexInContainer > aStartSkipIndexInContainer is also required, right? Please add some NS_PRECONDITIONs here. >+ if (aIsRangeInsertSafe) { >+ *aIsRangeInsertSafe = PR_FALSE; I think we only have one caller that doesn't want to know this information. I'd prefer that we just make aIsRangeInsertSafe a required-and-nonnull argument. Also, do we need this? There are no early returns in this function... >+IsListBoxParent(nsIContent* aContainer) IsXULListBox, perhaps? >+nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent* aContainer, >+ // If we have an insertion point (or multiple insertion points) and the >+ // operation is not a true append then we must fallback. >+ if (multiple || aEndIndexInContainer != -1 || childCount > 0) { It's more like: If we have multiple insertion points or if we have an insertion point and the operation is not a true append or if the insertion point already has explicit children, then we must fall back. I think. >+nsCSSFrameConstructor::MaybeRecreateForFrameset(nsIContent* aContainer, >+ nsIFrame* aParentFrame, >+ PRInt32 aStartIndexInContainer, >+ PRInt32 aEndIndexInContainer) Since aStartIndexInContainer and aEndIndexInContainer just get cast to PRUint32 anyway, why not make them PRUint32 to start with? >+// ContentRangeInserted handles creating frames for a range of nodes that >+// aren't at the end of their childlist. ContentRangeInserted only gets called >+// to lazily construct frames from CreateNeededFrames And from ContentInserted, right? >+nsCSSFrameConstructor::ContentRangeInserted(nsIContent* aContainer, ... >+ PRBool aSingle, This is true if and only if aEndIndexInContainer == aIndexInContainer + 1, right? Why is it needed at all? Can we not just computed a local |isSingleInsert| at the top of ContentRangeInserted? >+ } else if (aChild) { When is aChild null here? >+ NS_ASSERTION(!aSingle || aChild, "single insertions should pass a child"); But don't non-single ones pass one as well? They sure seem to. >+ // ContertInserted's for each node inserted. "ContentInserted calls", not "ContentInserted's". >+ NS_ASSERTION(aSingle || !aLazyConstruction, >+ "range insert shouldn't be lazy"); This seems like it could/should come much earlier in ContentInserted. At the very top of the function, in fact. >+ // If we don't have aChild, then it's a range insert, so the first child is >+ // in the principle child list at the start index. >+ if (!aChild) { >+ aChild = aContainer->GetChildAt(aIndexInContainer); >+ } s/principle/principal/, but this whole block should go away, as far as I can tell, since aChild is never null. >+ if (!aSingle && !isRangeInsertSafe) { >+ // must fallback to a single ContertInserted for each child in the range "fall back". >+ // Need check if a range insert is still safe. Need to check whether a range .... >+ RecoverLetterFrames(state.mFloatedItems.containingBlock); >+ >+ // must fallback to a single ContertInserted for each child in the range "fall back" >+ if (nsGkAtoms::tableFrame == frameType || >+ nsGkAtoms::tableOuterFrame == frameType) { >+ PullOutCaptionFrames(frameItems, captionItems); Can we really end up here when frameType == nsGkAtoms::tableOuterFrame while still doing a range insert? If so, how? I think the code above is safe, but I _think_ out current "is it safe to do a range insert?" checks would also make it safe to only check for nsGkAtoms::tableFrame. >+ // We might have captions, put them into the caption list of the s/,/;/ >+ // We need to determine where to put the caption items, start with the s/,/;/ >+ // It is very important here that we skip the children in >+ // [aIndexInContainer,aEndIndexInContainer) when looking for a >+ // prevsibling. >+ captionPrevSibling = >+ GetInsertionPrevSibling(captionParent, aContainer, firstCaption, >+ aContainer->IndexOf(firstCaption), &captionIsAppend, &ignored, >+ aIndexInContainer, aChild, >+ aEndIndexInContainer, aContainer->GetChildAt(aEndIndexInContainer)); So here we're relying on aEndIndexInContainer < aContainer->GetChildCount() if !aSingle, right? I guess that's more or less enforced by the fact that callers call ContentAppended in the other cases.... but might be worth asserting it on entry into this method. >+ // If the parent is not a outer table frame we will try to add frames "an outer" > + "Pseudo frame construction failure, " s/,/;/ >+ "a caption can be only a child of a outer table frame"); "an outer" Yes, I know you just moved this code... >+++ b/layout/base/nsCSSFrameConstructor.h >+ // not, returns null and issues single ContentInserted's for each child. "single ContentInserted calls" >- // If aLazyConstruction is true then frame construction of the new child >+ // If aLazyConstruction is true then frame construction of the new children > // is done lazily. > nsresult ContentInserted(nsIContent* aContainer, That should still be "child" in the comment, no? >+ //xxx the name 'range' overlaps with another concept True... I suppose we could call it ChildListIntervalInserted or something, but I think ContentRangeInserted is fine. >+ // aChild must be non-null when inserting a single node, otherwise if it's >+ // non-null is should be the first child being inserted. Never null in practice, right? Can we just require that here? >+ // If aLazyConstruction is true then frame construction of the new children >+ // is done lazily. "can be done" lazily, not "is done". And this should document that aLazyConstruction must be false when !aSingle, right? >+ // If aIsRangeInsertSafe is non-null then it returns whether it is safe to do >+ // a range insert with aChild being the first child in the range. Again, let's just require this to be non-null. >+ It is the >+ // callers responsibility to check if a range insert is safe with regards to >+ // fieldsets s/callers/callers'/ and s/if/whether/ >+ // The skip parameters are used to skip a range of children when looking for >+ // a sibling starting with aStartSkipChild (which is in aContainer's regular >+ // child list at aStartSkipIndexInContainer) and up to but not including >+ // aEndSkipChild (at aEndSkipIndexInContainer in aContainer). Maybe: The skip parameters are used to ignore a range of children when looking for a sibling. All nodes starting from aStartSkipChild (which is ....) and up to but not including aEndSkipChild (which is at ...) will be skipped over when looking for sibling frames. r=bzbarsky with the above nits. Let's get this landed!

Are we waiting on anything in particular here, or can we get this checked in?

Since I last pushed these patches to try server in mid January some changes on trunk have caused them to fail some tests. I'm currently debugging and fixing these failures.

(In reply to comment #31) > (From update of attachment 422180 [details] [diff] [review]) > To be safe, you should probably enumerate the loadgroup to make sure that > mOnloadBlocker isn't in it already. r=bzbarsky with that. Ah, you're right. I mistakenly thought adding the same request was idempotent.

docshell/test/navigation/test_reserved.html inserts and iframe and the iframe uses an inline script to insert a link and then uses enhanced privileges to dispatch a click to that link. However, for a click on a link to actually navigate the document it needs to have a prescontext (nsGenericHTMLElement::CheckHandleEventForAnchorsPreconditions), but with lazy frame construction the prescontext only gets constructed when the frame for the iframe gets constructed, so there may not be a prescontext. I don't think there is anyway to send a click to a link without enhanced privileges, so I don't think this is an issue for the web. To fix the test all we need to do is run the script from onload instead of using an inline script element.

Referencing an external document for eg an SVG filter doesn't cause that document load to block onload. So instead add an SVG <use> that references that external document to SVG tests that use external references so that document blocks onload.

(In reply to comment #33) > (From update of attachment 422182 [details] [diff] [review]) > This looks ok, but the comments should probably make it clear that > NODE_DESCANDANTS_NEED_FRAMES means that flattened tree descendants need frames, > and that in particular the flag needs to be set up the flattened tree parent > chain, not just the DOM parent chain. Done.

(In reply to comment #34) > Note that we'll need to traverse that chain in bug 479655 too. So I think we > should add a method on nsIContent (slid as a patch in under part 3 somewhere, > or just a separate bug blocking this one) that returns the flattened tree > parent. Basically GetParent() unless the return value of GetParent() has > NODE_MAY_BE_IN_BINDING_MNGR set, in which case we should do the > look-for-insertion-points dance. Here it is.

This in the v2 to v3 interdiff (really its a separate mq in my tree). Let me know if you want something else. (In reply to comment #35) > >+ UnsetFlags(NODE_FORCE_XBL_BINDINGS | > >+ // And clear the lazy frame construction bits. > >+ NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES); > > I'd prefer that comment indented to line up with the NODE_NEEDS_FRAME. Done. > Hmm. IsChrome() is not that cheap an operation, actually. Can you file a > followup bug to cache the result in the prescontext? Filed bug 553359 for that. > >+ if (aOperation == CONTENTINSERT) { > >+ if (!aChild || aIndex < 0 || aContainer->GetChildAt(aIndex) != aChild || > > How could !aChild be true here? It shouldn't, just an abundance of caution on my part. I removed that bit. > >+ } else { // CONTENTAPPEND > > Assert that? Done. > >+ // We can construct lazily, just need to set suitable bits in the content > >+ // tree. > > Semicolon needed there, not a comma. Done. > >+ // Walk up the tree setting the NODE_DESCENDANTS_NEED_FRAMES bit as we go. If > >+ // we hit a node with NODE_NEEDS_FRAME set then we don't need to bother > >+ // setting the bits on the new nodes or posting a restyle event. > > Can we not also stop if we hit a node with no frame? Either it'll get a frame > created at some point (and then create them for all its descendants) or it > won't (and then we don't need a frame either). > > In particular, any node with NODE_NEEDS_FRAME would also have no frame, so that > test would subsume this one. > > That said, it looks to me like the callsites already check whether the parent > has a frame. So I would expect us to never hit a NODE_NEEDS_FRAME or frameless > node during this traversal. Do we hit such nodes? Nice catch. I removed that bit. > >+ content = content->GetParent(); > > This should be getting the flattened tree parent, per our earlier discussion. Done. > >+static void > >+ClearLazyBitsInChildren(nsIContent* aContent, > >+ PRUint32 aStartIndex = 1, > >+ PRUint32 aEndIndex = 0) > >+{ > >+ PRBool allChildren = aEndIndex < aStartIndex; > > Why do we have the default args and the whole allChildren thing? The only > caller passes both args and passes aStartIndex <= aEndIndex. That is a mistake on my part, ClearLazyBitsInChildren should be called from a few other sites that use those features. I added the extra call sites. Thanks for catching this. > >+ for (PRUint32 i = startIndex; i < endIndex; i++) { > >+ aContent->GetChildAt(i)-> > >+ UnsetFlags(NODE_DESCENDANTS_NEED_FRAMES | NODE_NEEDS_FRAME); > >+ } > > It's almost tempting to say that this should use GetChildArray. Not sure it > matters much in practice, though... Yeah, we could. ClearLazyBitsInChildren isn't necessary, I debated about including it or not because either way could have negative effects. It shouldn't get called much in practice. > >+nsCSSFrameConstructor::CreateNeededFrames(nsIContent* aContent) > >+ aContent->UnsetFlags(NODE_DESCENDANTS_NEED_FRAMES); > > Assert that the bit is set before that? Done. > >+ //xxx should we use GetChildArray and scan it completely first and then issue > >+ // all notifications? > > Another question. Why would you need to scan it completely? Can these > notifications change the DOM tree? I don't think they can change the DOM tree but they can change the storage of childlists via attribute setting. For example this piece of stack from layout/reftests/bugs/414123-ref.xhtml #0 nsGenericElement::SetAttrAndNotify (this=0x7f2bde193980, aNamespaceID=0, aName=0x7f2be2503110, aPrefix=0x0, aOldValue=..., aParsedValue=..., aModification=0, aFireMutation=0, aNotify=0, aValueForAfterSetAttr=0x7fff665cefc0) at ../../../../content/base/src/nsGenericElement.cpp:4377 #1 0x00007f2bf5a73551 in nsGenericElement::SetAttr (this=0x7f2bde193980, aNamespaceID=<value optimized out>, aName=0x7f2be2503110, aPrefix=<value optimized out>, aValue=..., aNotify=0) at ../../../../content/base/src/nsGenericElement.cpp:4364 #2 0x00007f2bf5dea7dc in nsMathMLTokenFrame::SetTextStyle (this=0x7f2bd97cc400) at ../../../layout/mathml/nsMathMLTokenFrame.cpp:364 #3 0x00007f2bf5dea844 in nsMathMLTokenFrame::ProcessTextData (this=0x7f2bde193980) at ../../../layout/mathml/nsMathMLTokenFrame.cpp:273 #4 0x00007f2bf5deacb9 in nsMathMLTokenFrame::SetInitialChildList (this=0x7f2bd97cc400, aListName=<value optimized out>, aChildList=<value optimized out>) at ../../../layout/mathml/nsMathMLTokenFrame.cpp:146 #5 0x00007f2bf58382d4 in nsCSSFrameConstructor::ConstructFrameFromItemInternal ( this=<value optimized out>, aItem=..., aState=..., aParentFrame=<value optimized out>, aFrameItems=<value optimized out>) at ../../../layout/base/nsCSSFrameConstructor.cpp:3841 > >+ nsIContent* firstChildInRun = nsnull; > > This is write-only. Ditch it? Sorry, that is a bit that belongs in part 4. > >+//xxx should this use a non-recursive method? > >+void nsCSSFrameConstructor::CreateNeededFrames() > > That seems like it would be difficult without some heap-allocation, right? So > much of frame construction is already recursive that I wouldn't worry about it. Make sense, I'll ditch the comment. > > nsCSSFrameConstructor::ContentAppended(nsIContent* aContainer, > >- PRInt32 aNewIndexInContainer) > >+ PRInt32 aNewIndexInContainer, > >+ PRBool aLazyConstruction) > > That new arg should probably be called aAllowLazyConstruction or something. It > just allows it; it doesn't force it. Done. > >@@ -6296,18 +6466,19 @@ nsCSSFrameConstructor::ContentAppended(n > >+ child->UnsetFlags(NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES); > > I guess |child| could have NODE_DESCENDANTS_NEED_FRAMES if restyle frame > reconstructs are happening on ancestors of things that had NODE_NEEDS_FRAME, > right? Would it be better to just push this line of code down into the very > beginning of AddFrameConstructionItems instead? We'd still need the explicit > call in BuildInlineChildItems, but that should cover all the other callsites. > The extra cost of making those calls on native anon content is not that big a > deal compared to the more maintainable code, I think. Good idea, this makes the code nicer. > >+nsCSSFrameConstructor::PostRestyleEventInternal(PRBool aForLazyConstruction) > >+ if (!mObservingRefreshDriver && (!mInStyleRefresh || > >+ (aForLazyConstruction && !mInLazyFCRefresh))) { > > Document that the correctness of this test (or at least its correct > implementation of the optimization I think it's trying to implement) depends on > the order in which CreateNeededFrames and ProcessPendingRestyles get called, > please? Actually I think I'll just get rid of the micro-optimization and go for the clearer code. > And document in the header that CreateNeededFrames must be called before > ProcessPendingRestyles? Done. > >+++ b/layout/base/nsCSSFrameConstructor.h > > >+ * Lazy frame construction is controlled by the aLazyConstruction bool > > aAllowLazyConstruction. Or aMayBeLazy, maybe? aAllowLazyConstruction sounds good to me. > >+ * If MaybeConstructLazily returns false we construct as usual, but if it > >+ * returns true then it adds NODE_NEEDS_FRAME bits to the newly > >+ * inserted/appended nodes and adds NODE_DESCENDANTS_NEED_FRAMES bits to the > >+ * container and up along the parent chain until it hits the root or another > >+ * node with one of those bits set. > > This comment might need changing depending on whether we leave the > NODE_NEEDS_FRAME stuff in MaybeConstructLazily. Updated. > >+ (We do clear the bits nsGenericElement::BindToTree though.) > > "bits in nsGenericElement::BindToTree" Done. > r=bzbarsky with the above nits fixed. Thanks!

This in the v2 to v3 interdiff (really its a separate mq in my tree). Let me know if you want something else. When fixing the rejects for this part I got confused and converted all occurrences of aLazyConstruction to aAllowLazyConstruction, including those that only appeared in part 4. So you won't see those changes in this interdiff. I hope that is not a problem. (In reply to comment #36) > >+++ b/layout/base/nsCSSFrameConstructor.cpp > > nsCSSFrameConstructor::GetInsertionPrevSibling(nsIFrame*& aParentFrame, > >+ PRInt32 aStartSkipIndexInContainer, > >+ nsIContent* aStartSkipChild, > >+ PRInt32 aEndSkipIndexInContainer, > >+ nsIContent *aEndSkipChild) > > So aStartSkipChild is non-null if and only if aStartSkipIndexInContainer >= 0, > and likewise for aEndSkipChild, right? Can we assert whatever has to be true > here? Oh and aStartSkipIndexInContainer and aEndSkipIndexInContainer must > either both be -1 or both be >= 0. And in the latter case, > aEndSkipIndexInContainer > aStartSkipIndexInContainer is also required, right? > > Please add some NS_PRECONDITIONs here. Added preconditions. > >+ if (aIsRangeInsertSafe) { > >+ *aIsRangeInsertSafe = PR_FALSE; > > I think we only have one caller that doesn't want to know this information. > I'd prefer that we just make aIsRangeInsertSafe a required-and-nonnull > argument. > > Also, do we need this? There are no early returns in this function... Done. > >+IsListBoxParent(nsIContent* aContainer) > > IsXULListBox, perhaps? Done. > >+nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent* aContainer, > >+ // If we have an insertion point (or multiple insertion points) and the > >+ // operation is not a true append then we must fallback. > >+ if (multiple || aEndIndexInContainer != -1 || childCount > 0) { > > It's more like: > > If we have multiple insertion points or if we have an insertion point and > the operation is not a true append or if the insertion point already has > explicit children, then we must fall back. > > I think. Changed the comment based on that. > >+nsCSSFrameConstructor::MaybeRecreateForFrameset(nsIContent* aContainer, > >+ nsIFrame* aParentFrame, > >+ PRInt32 aStartIndexInContainer, > >+ PRInt32 aEndIndexInContainer) > > Since aStartIndexInContainer and aEndIndexInContainer just get cast to PRUint32 > anyway, why not make them PRUint32 to start with? Done. > >+// ContentRangeInserted handles creating frames for a range of nodes that > >+// aren't at the end of their childlist. ContentRangeInserted only gets called > >+// to lazily construct frames from CreateNeededFrames > > And from ContentInserted, right? Yeah. The point I was trying to get across was that ContentRangeInserted only gets called to insert more than one node from CreateNeededFrames. I updated the comment to hopefully make this clear. > >+nsCSSFrameConstructor::ContentRangeInserted(nsIContent* aContainer, > ... > >+ PRBool aSingle, > > This is true if and only if aEndIndexInContainer == aIndexInContainer + 1, > right? Why is it needed at all? Can we not just computed a local > |isSingleInsert| at the top of ContentRangeInserted? Yes we can, it was vestigial from an early incarnation. > >+ } else if (aChild) { > > When is aChild null here? It isn't. Removed that else if condition. > >+ NS_ASSERTION(!aSingle || aChild, "single insertions should pass a child"); > > But don't non-single ones pass one as well? They sure seem to. Yes. The three things above were left overs. > >+ // ContertInserted's for each node inserted. > > "ContentInserted calls", not "ContentInserted's". Done. > >+ NS_ASSERTION(aSingle || !aLazyConstruction, > >+ "range insert shouldn't be lazy"); > > This seems like it could/should come much earlier in ContentInserted. At the > very top of the function, in fact. Done. > >+ // If we don't have aChild, then it's a range insert, so the first child is > >+ // in the principle child list at the start index. > >+ if (!aChild) { > >+ aChild = aContainer->GetChildAt(aIndexInContainer); > >+ } > > s/principle/principal/, but this whole block should go away, as far as I can > tell, since aChild is never null. Done. Another left over. > >+ if (!aSingle && !isRangeInsertSafe) { > >+ // must fallback to a single ContertInserted for each child in the range > > "fall back". Done. > >+ // Need check if a range insert is still safe. > > Need to check whether a range .... Done. > >+ RecoverLetterFrames(state.mFloatedItems.containingBlock); > >+ > >+ // must fallback to a single ContertInserted for each child in the range > > "fall back" Done. > >+ if (nsGkAtoms::tableFrame == frameType || > >+ nsGkAtoms::tableOuterFrame == frameType) { > >+ PullOutCaptionFrames(frameItems, captionItems); > > Can we really end up here when frameType == nsGkAtoms::tableOuterFrame while > still doing a range insert? If so, how? I think the code above is safe, but I > _think_ out current "is it safe to do a range insert?" checks would also make > it safe to only check for nsGkAtoms::tableFrame. This same code handles both a range insert (with > 1 nodes) and inserting a single node, so I think we can get here with a table outer frame. Unless I'm missing something. > >+ // We might have captions, put them into the caption list of the > > s/,/;/ > > >+ // We need to determine where to put the caption items, start with the > > s/,/;/ Done and done. > >+ // It is very important here that we skip the children in > >+ // [aIndexInContainer,aEndIndexInContainer) when looking for a > >+ // prevsibling. > >+ captionPrevSibling = > >+ GetInsertionPrevSibling(captionParent, aContainer, firstCaption, > >+ aContainer->IndexOf(firstCaption), &captionIsAppend, &ignored, > >+ aIndexInContainer, aChild, > >+ aEndIndexInContainer, aContainer->GetChildAt(aEndIndexInContainer)); > > So here we're relying on aEndIndexInContainer < aContainer->GetChildCount() if > !aSingle, right? I guess that's more or less enforced by the fact that callers > call ContentAppended in the other cases.... but might be worth asserting it on > entry into this method. Done. > >+ // If the parent is not a outer table frame we will try to add frames > > "an outer" > > > + "Pseudo frame construction failure, " > > s/,/;/ > > >+ "a caption can be only a child of a outer table frame"); > > "an outer" > > Yes, I know you just moved this code... Done x 3. > >+++ b/layout/base/nsCSSFrameConstructor.h > > >+ // not, returns null and issues single ContentInserted's for each child. > > "single ContentInserted calls" Done. > >- // If aLazyConstruction is true then frame construction of the new child > >+ // If aLazyConstruction is true then frame construction of the new children > > // is done lazily. > > nsresult ContentInserted(nsIContent* aContainer, > > That should still be "child" in the comment, no? You are correct. > >+ // aChild must be non-null when inserting a single node, otherwise if it's > >+ // non-null is should be the first child being inserted. > > Never null in practice, right? Can we just require that here? Yes, let's do that. > >+ // If aLazyConstruction is true then frame construction of the new children > >+ // is done lazily. > > "can be done" lazily, not "is done". And this should document that > aLazyConstruction must be false when !aSingle, right? Done. > >+ // If aIsRangeInsertSafe is non-null then it returns whether it is safe to do > >+ // a range insert with aChild being the first child in the range. > > Again, let's just require this to be non-null. Done. > >+ It is the > >+ // callers responsibility to check if a range insert is safe with regards to > >+ // fieldsets > > s/callers/callers'/ and s/if/whether/ Done. > >+ // The skip parameters are used to skip a range of children when looking for > >+ // a sibling starting with aStartSkipChild (which is in aContainer's regular > >+ // child list at aStartSkipIndexInContainer) and up to but not including > >+ // aEndSkipChild (at aEndSkipIndexInContainer in aContainer). > > Maybe: > > The skip parameters are used to ignore a range of children when looking for > a sibling. All nodes starting from aStartSkipChild (which is ....) and up > to but not including aEndSkipChild (which is at ...) will be skipped over > when looking for sibling frames. That's much better. > r=bzbarsky with the above nits. Let's get this landed! If only it was that easy!

How is progress?

The new patches are waiting to be reviewed.

Comment on attachment 433390 [details] [diff] [review] Part 1.1. Fix up another test. r=bzbarsky

Comment on attachment 433389 [details] [diff] [review] Part 0 v3. Man is the simple enumerator stuff ugly. :(

Comment on attachment 433392 [details] [diff] [review] Part 2 v3. Add bits to nsINode.h. r=bzbarsky

Comment on attachment 433404 [details] [diff] [review] Part 2.5. Provide an API to get the flattened tree parent of an nsIContent. r=bzbarsky

Comments on the V3 interdiff: >> That said, it looks to me like the callsites already check whether the parent >> has a frame. So I would expect us to never hit a NODE_NEEDS_FRAME or >> frameless node during this traversal. Do we hit such nodes? > >Nice catch. I removed that bit. Can we add an assert in that loop that the ancestor has a frame and doesn't have NODE_NEEDS_FRAME? >+ if (!mObservingRefreshDriver && >+ ((!aForLazyConstruction && !mInStyleRefresh) || > (aForLazyConstruction && !mInLazyFCRefresh))) { This might be more readable as: PRBool inRefresh = aForLazyConstruction ? mInLazyFCRefresh : mInStyleRefresh; if (!mObservingRefreshDriver && !inRefresh) {

Comment on attachment 433419 [details] [diff] [review] Part 3 v2 to v3 interdiff. r=me with the nits of comment 52 picked.

Comment on attachment 433420 [details] [diff] [review] Part 4 v2 to v3 interdiff. Comments on the part 4 interdiff: >+ NS_PRECONDITION((aStartSkipIndexInContainer >= 0 && aStartSkipChild) || >+ (aStartSkipIndexInContainer < 0 && !aStartSkipChild), >+ "aStartSkipIndexInContainer >= 0 iff aStartSkipChild"); Could be: NS_PRECONDITION((aStartSkipIndexInContainer >= 0) == !!aStartSkipChild, "aStartSkipIndexInContainer >= 0 iff aStartSkipChild"); Not sure which is easier to read. Similar for aEndSkipIndexInContainer, of course. > >- // If aLazyConstruction is true then frame construction of the new child > >+ // If aLazyConstruction is true then frame construction of the new children > > // is done lazily. > > nsresult ContentInserted(nsIContent* aContainer, > > That should still be "child" in the comment, no? This review comment didn't get addressed as far as I can see. While I'm there, the comment in the header on IssueSingleInsertNofications has "Issue's" instead of "Issues". r=bzbarsky with the nits.

(In reply to comment #54) > > >- // If aLazyConstruction is true then frame construction of the new child > > >+ // If aLazyConstruction is true then frame construction of the new children > > > // is done lazily. > > > nsresult ContentInserted(nsIContent* aContainer, > > > > That should still be "child" in the comment, no? > > This review comment didn't get addressed as far as I can see. It got fixed in my queue, but it must be been fixed in the wrong place. Sorry. Fixed all the other things mentioned in comment 52 and comment 54 in my tree.

Comment on attachment 420517 [details] [diff] [review] Part 5. Create some tests. >+++ b/layout/reftests/reftest-sanity/reftest.list >+# test that inserting several divs works >+== insertdiv == insertdiv-ref.html Nix the second "=="? This looks like it covers a single ContentRangeInserted, right? I'd like to see tests that cover: 1) Multiple ContentRangeInserted notifications from one single flush, including a ContentAppended. 2) Tests that rely on the DISPLAY_NOT_SET thing preventing coalescing (do your table tests do that?) 3) A test that tests a single-insert case. 4) Something that tests a situation in which we ContentAppended some content (which didn't create frames because it decided to be lazy), then appended a node that decided to not be lazy, then processed the lazy stuff.

(In reply to comment #56) > (From update of attachment 420517 [details] [diff] [review]) > >+++ b/layout/reftests/reftest-sanity/reftest.list > >+# test that inserting several divs works > >+== insertdiv == insertdiv-ref.html > > Nix the second "=="? Whoops, I must have uploaded the wrong file, that was fixed quickly in my tree. > 2) Tests that rely on the DISPLAY_NOT_SET thing preventing coalescing (do your > table tests do that?) I added a test with rowgroups and colgroups. > 3) A test that tests a single-insert case. Did you mean just inserting a single node? Either way I added a test for inserting a single node as well as tests that rely on ContentRangeInsert splitting up range inserts into single inserts due to bindings having multiple insertion points. > 4) Something that tests a situation in which we ContentAppended some content > (which didn't create frames because it decided to be lazy), then appended a > node that decided to not be lazy, then processed the lazy stuff. Done. I added tests where the non-lazy node was: XUL, an input textbox (this will hopefully be made lazy in bug 559970), and a contenteditable div.

> Did you mean just inserting a single node? Yes, and then triggering a flush so that ContentRangeInserted ends up inserting a range with only one node in it. The rest sounds good.

Comment on attachment 439660 [details] [diff] [review] Part 5 v2. Create some tests. r=bzbarsky

I pushed this and it seems to have stuck. http://hg.mozilla.org/mozilla-central/rev/862dee8093f9 http://hg.mozilla.org/mozilla-central/rev/cf20715bfc4a http://hg.mozilla.org/mozilla-central/rev/fc55069721b7 http://hg.mozilla.org/mozilla-central/rev/fd014668bb04 http://hg.mozilla.org/mozilla-central/rev/e6ba6ff50841 http://hg.mozilla.org/mozilla-central/rev/aa6171e5b8e7 http://hg.mozilla.org/mozilla-central/rev/f80de1a7c604 http://hg.mozilla.org/mozilla-central/rev/7a19bc84e572 http://hg.mozilla.org/mozilla-central/rev/699e6f9a3660 http://hg.mozilla.org/mozilla-central/rev/935bb616b8a6

Pushed to fix some typos in reftest.list files http://hg.mozilla.org/mozilla-central/rev/9b724246586a