Bug 79180 – firstRendererOf() returns fallback element in NodeRenderingContext

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

Currently we handle AttachingFallback in some special manner.
However, if firstRendererOf and lastRendererOf returns fallback renderer, we don't need to handle it in special manner.

[Shinya Kawanaka]

Created attachment 128103 [details]
Patch

[Shinya Kawanaka]

Comment on attachment 128103 [details]
Patch

Ah, I should have to remove more code.

[Hajime Morrita]

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 ?

[Shinya Kawanaka]

(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...

[Shinya Kawanaka]

Created attachment 128143 [details]
Patch

[Hajime Morrita]

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.

[Shinya Kawanaka]

> > 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.

[Shinya Kawanaka]

Created attachment 128156 [details]
Patch

[Shinya Kawanaka]

(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.

[WebKit Review Bot]

Comment on attachment 128156 [details]
Patch

Clearing flags on attachment: 128156

Committed r108480: <http://trac.webkit.org/changeset/108480>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.