[webkit-reviews] review granted: [Bug 79180] firstRendererOf() returns fallback element in NodeRenderingContext : [Attachment 128143] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has granted Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 79180: firstRendererOf() returns fallback element in NodeRenderingContext
https://bugs.webkit.org/show_bug.cgi?id=79180

Attachment 128143: Patch
https://bugs.webkit.org/attachment.cgi?id=128143&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
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.