[Shadow] ASSERT triggered when we try reprojecting fallback elements

[Shinya Kawanaka]

Attaching fallback contents twice!

[Shinya Kawanaka]

Created attachment 169561 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 169561 [details]
Patch

I see what you're doing here, but as Dominic would say, the solution looks a bit "skeezy" :)

1) Why is fallback content attached in the first place?
2) If it is, wouldn't it be more civil to detach and reattach it?

[Shinya Kawanaka]

+morrita

We were attaching elements twice before, but it made NodeRenderingContext complex.
I and morrita removed such complexity before, so I'm afraid that I bring it again...

[Hajime Morrita]

Comment on attachment 169561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169561&action=review

> Source/WebCore/dom/Element.cpp:1013
> +        if (childrenMightBeAlreadyAttached()) {

Adding a virtual call here will hurt the speed. We need to figure out some better way.

[Shinya Kawanaka]

Created attachment 169827 [details]
Patch

[Hajime Morrita]

Comment on attachment 169827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169827&action=review

Can you take some performance measurement? I think checking dom-modify and html5-full-render will be sufficient.

> Source/WebCore/dom/ContainerNode.h:166
> +    virtual bool childAttachedAllowedWhenAttachingChildren() { return false; }

This doesn't need to be polymorphic. We can just make it a standalone function. 
Then we can get rid of this #ifdef and just call it inside the ASSERT-ed expression.

> Source/WebCore/dom/ContainerNode.h:218
> +#ifndef NDEBUG

You don't need this. Asssertions are on only on debug build.

[Shinya Kawanaka]

Created attachment 169840 [details]
Patch

[Hajime Morrita]

Comment on attachment 169840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169840&action=review

> Source/WebCore/ChangeLog:12
> +        We have confirmed that this patch does not regress the performance so much. The summary of the

Don't need "so much". This obviously has no perf impact. Be confident :-)

> Source/WebCore/dom/ContainerNode.h:NaN
>  inline void ContainerNode::attachChildren()

Do we have attachChildren() anywhere else? I believe we can replace attachChildren() with the "IfNeeed" version.

[Shinya Kawanaka]

(In reply to comment #8)
> (From update of attachment 169840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169840&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        We have confirmed that this patch does not regress the performance so much. The summary of the
> 
> Don't need "so much". This obviously has no perf impact. Be confident :-)
> 
> > Source/WebCore/dom/ContainerNode.h:NaN
> >  inline void ContainerNode::attachChildren()
> 
> Do we have attachChildren() anywhere else? I believe we can replace attachChildren() with the "IfNeeed" version.

I'll replace attachChildren() with the current version.

I'll also do a few refactoring after landing this.

[Shinya Kawanaka]

Created attachment 169848 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169848 [details]
Patch for landing

Clearing flags on attachment: 169848

Committed r132047: <http://trac.webkit.org/changeset/132047>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.