Bug 79180 - firstRendererOf() returns fallback element in NodeRenderingContext
Summary: firstRendererOf() returns fallback element in NodeRenderingContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 79220
  Show dependency treegraph
 
Reported: 2012-02-21 18:44 PST by Shinya Kawanaka
Modified: 2012-02-22 04:06 PST (History)
1 user (show)

See Also:


Attachments
Patch (5.43 KB, patch)
2012-02-21 18:51 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (18.75 KB, patch)
2012-02-21 23:32 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (20.06 KB, patch)
2012-02-22 01:35 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-21 18:44:54 PST
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.
Comment 1 Shinya Kawanaka 2012-02-21 18:51:27 PST
Created attachment 128103 [details]
Patch
Comment 2 Shinya Kawanaka 2012-02-21 19:28:27 PST
Comment on attachment 128103 [details]
Patch

Ah, I should have to remove more code.
Comment 3 Hajime Morrita 2012-02-21 19:51:17 PST
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 ?
Comment 4 Shinya Kawanaka 2012-02-21 22:29:26 PST
(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...
Comment 5 Shinya Kawanaka 2012-02-21 23:32:34 PST
Created attachment 128143 [details]
Patch
Comment 6 Hajime Morrita 2012-02-22 00:20:42 PST
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.
Comment 7 Shinya Kawanaka 2012-02-22 00:33:49 PST
> > 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.
Comment 8 Shinya Kawanaka 2012-02-22 01:35:50 PST
Created attachment 128156 [details]
Patch
Comment 9 Shinya Kawanaka 2012-02-22 01:37:13 PST
(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 10 WebKit Review Bot 2012-02-22 04:06:23 PST
Comment on attachment 128156 [details]
Patch

Clearing flags on attachment: 128156

Committed r108480: <http://trac.webkit.org/changeset/108480>
Comment 11 WebKit Review Bot 2012-02-22 04:06:29 PST
All reviewed patches have been landed.  Closing bug.