| Summary: | Factor out common post-insertion logic in ContainerNode | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Klein <adamk> | ||||||
| Component: | New Bugs | Assignee: | Adam Klein <adamk> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, mjs, ojan, rniwa, sam, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Adam Klein
2012-03-28 15:52:06 PDT
Created attachment 134431 [details]
Patch
Comment on attachment 134431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134431&action=review > Source/WebCore/dom/ContainerNode.cpp:1093 > + ASSERT(parent->refCount()); > + ASSERT(child->refCount()); great asserts! > Source/WebCore/dom/ContainerNode.cpp:1095 > + // Send notification about the children change. Nit: I realize some of the old instances of this had this comment, but it's pretty useless. Delete? > Source/WebCore/dom/ContainerNode.cpp:1099 > + // Add child to the rendering tree. Ditto. Useless comment. > Source/WebCore/dom/ContainerNode.cpp:1108 > + // Now that the child is attached to the render tree, dispatch > + // the relevant mutation events. Ditto. Comment on attachment 134431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134431&action=review > Source/WebCore/dom/ContainerNode.cpp:1095 > + // Send notification about the children change. It seems like this comment is a pure noise. > Source/WebCore/dom/ContainerNode.cpp:1096 > + parent->childrenChanged(false, child->previousSibling(), child->nextSibling(), 1); It seems like we'll be calling with a different prev if DOM had been mutated in the last call to dispatchChildInsertionEvents but new code seems more correct. Let's hope no website depends on the old behavior (very unlikely). Is there anyway to test this behavior change? > Source/WebCore/dom/ContainerNode.cpp:1099 > + // Add child to the rendering tree. Ditto. > Source/WebCore/dom/ContainerNode.cpp:1108 > + // Now that the child is attached to the render tree, dispatch > + // the relevant mutation events. Ditto. Can we explain why we need to do things in this order instead? Comment on attachment 134431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134431&action=review >>> Source/WebCore/dom/ContainerNode.cpp:1095 >>> + // Send notification about the children change. >> >> Nit: I realize some of the old instances of this had this comment, but it's pretty useless. Delete? > > It seems like this comment is a pure noise. Will remove all comments. >> Source/WebCore/dom/ContainerNode.cpp:1096 >> + parent->childrenChanged(false, child->previousSibling(), child->nextSibling(), 1); > > It seems like we'll be calling with a different prev if DOM had been mutated in the last call to dispatchChildInsertionEvents but new code seems more correct. > Let's hope no website depends on the old behavior (very unlikely). > > Is there anyway to test this behavior change? Note that this is only a change in logic for insertBefore: replaceChild and appendChild already updated prev every time through the loop, after dispatching events. The only two uses of the "beforeChange" argument to childrenChanged are in Element and HTMLElement. The former is to handle updating the :last-child selector, and there's no way I can see to get the old code to generate incorrect behavior: all it's doing is making sure the old lastChild gets its style recalculated, and that'll happen the first time childrenChanged is called (which is before any events fire). The HTMLElement case involves directionality, and looks like it might be testable, but I don't know enough about directionality to easily put one together. See HTMLElement::adjustDirectionalityIfNeededAfterChildrenChanged if you'd like to help me find some way to test this (looks like the new behavior would be strictly better than the old behavior). >>> Source/WebCore/dom/ContainerNode.cpp:1108 >>> + // the relevant mutation events. >> >> Ditto. > > Ditto. Can we explain why we need to do things in this order instead? I'm worried that there isn't a good explanation, other than that this is the order we do them in. Do you have a suggestion for why it's important that we call attach() or lazyAttach() before dispatchChildInsertionEvents()? Created attachment 134464 [details]
Added FIXME
Comment on attachment 134464 [details]
Added FIXME
I think Ojan's r+ withstands :)
Comment on attachment 134464 [details] Added FIXME Clearing flags on attachment: 134464 Committed r112546: <http://trac.webkit.org/changeset/112546> All reviewed patches have been landed. Closing bug. |