Bug 79180

Created attachment 128103 [details] Patch

Comment on attachment 128103 [details] Patch Ah, I should have to remove more code.

Comment on attachment 128103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128103&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:159 > + return parent->firstChild()->renderer(); I don't think this is enough to replace |NodeRenderingContext(m_node->parentNode()).nextRenderer()|. In the fallback case, nextRenderer() is possible to return non-fallback element. Actually as the expression implies, it return the renderer which is next to the renderer of the parent. not renderer of the parent's first child. The point here is that nextRenderer() doesn't necessarily return a renderer of parent's children. It could return one of its sibling's. This is really tricky part but we don't have enough test coverage... > Source/WebCore/html/shadow/InsertionPoint.h:63 > + ASSERT(!node || isInsertionPoint(node)); Doesn't need !node ?

(In reply to comment #3) > (From update of attachment 128103 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128103&action=review > > > Source/WebCore/dom/NodeRenderingContext.cpp:159 > > + return parent->firstChild()->renderer(); > > I don't think this is enough to replace |NodeRenderingContext(m_node->parentNode()).nextRenderer()|. > In the fallback case, nextRenderer() is possible to return non-fallback element. > Actually as the expression implies, it return the renderer which is next to the renderer of the parent. not renderer of the parent's first child. Yeah, we should add a test case that <content> has <content> as a fallback element...

Created attachment 128143 [details] Patch

Comment on attachment 128143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128143&action=review Basically looks good. Let's iterate once more. > Source/WebCore/dom/NodeRenderingContext.cpp:156 > + } Could you return here and eliminate else clause? We generally prefer early return style. > Source/WebCore/dom/NodeRenderingContext.cpp:169 > + } Ditto for return. > Source/WebCore/dom/NodeRenderingContext.h:-77 > - AttachingFallback, These names are getting more confusing. Could you re-align this in separate change? > Source/WebCore/html/shadow/InsertionPoint.h:63 > + ASSERT(!node || isInsertionPoint(node)); it looks we don't need |!node|. > LayoutTests/fast/dom/shadow/shadow-contents-fallback-dynamic.html:343 > +function testContentInContent(callIfDone) { covering these extra case is great! Could you have some case where some host and fallback nodes don't have renderer? we can do it by giving display:none.

> > LayoutTests/fast/dom/shadow/shadow-contents-fallback-dynamic.html:343 > > +function testContentInContent(callIfDone) { > > covering these extra case is great! > Could you have some case where some host and fallback nodes don't have renderer? we can do it by giving display:none. Let me file another bug to cover these cases.

Created attachment 128156 [details] Patch

(In reply to comment #6) > (From update of attachment 128143 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128143&action=review > > Basically looks good. Let's iterate once more. > > > Source/WebCore/dom/NodeRenderingContext.cpp:156 > > + } > > Could you return here and eliminate else clause? > We generally prefer early return style. Done. > > > Source/WebCore/dom/NodeRenderingContext.cpp:169 > > + } > > Ditto for return. Done. > > Source/WebCore/dom/NodeRenderingContext.h:-77 > > - AttachingFallback, > > These names are getting more confusing. Could you re-align this in separate change? Let me make another bug. > > Source/WebCore/html/shadow/InsertionPoint.h:63 > > + ASSERT(!node || isInsertionPoint(node)); > > it looks we don't need |!node|. Done. > > LayoutTests/fast/dom/shadow/shadow-contents-fallback-dynamic.html:343 > > +function testContentInContent(callIfDone) { > > covering these extra case is great! > Could you have some case where some host and fallback nodes don't have renderer? we can do it by giving display:none. I've added a new case, but we should have exhaustive tests. Let me make another bug for this.

Comment on attachment 128156 [details] Patch Clearing flags on attachment: 128156 Committed r108480: <http://trac.webkit.org/changeset/108480>

All reviewed patches have been landed. Closing bug.