Bug 78596 – <shadow> should be rendered correctly

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

[Shinya Kawanaka]

<shadow> should render an older shadow subtree.

[Shinya Kawanaka]

This will be easily done if my InsertionPoint refactoring.
In that case, <shadow> should be derived from InsertionPoint.

[Shinya Kawanaka]

Created attachment 128173 [details]
WIP

[Shinya Kawanaka]

Created attachment 128657 [details]
WIP

[WebKit Review Bot]

Comment on attachment 128657 [details]
WIP

Attachment 128657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11603713

[Early Warning System Bot]

Comment on attachment 128657 [details]
WIP

Attachment 128657 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11576739

[Philippe Normand]

Comment on attachment 128657 [details]
WIP

Attachment 128657 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11608723

[WebKit Review Bot]

Comment on attachment 128657 [details]
WIP

Attachment 128657 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11574752

[Gyuyoung Kim]

Comment on attachment 128657 [details]
WIP

Attachment 128657 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11599720

[Shinya Kawanaka]

Created attachment 129394 [details]
Patch

[Hajime Morrita]

Comment on attachment 129394 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +

Please split this up to these three changes if possible.
This patch is simply to big.
You can use this bug as a metabug.

> Source/WebCore/html/shadow/HTMLShadowElement.h:56
> +    ShadowRoot* distributedShadowRoot;

Please follow the naming guideline.

[Shinya Kawanaka]

Let me try to split the patch.

[Shinya Kawanaka]

Created attachment 129654 [details]
WIP

[Shinya Kawanaka]

Created attachment 129658 [details]
WIP

[Shinya Kawanaka]

Created attachment 129661 [details]
Patch

[Hajime Morrita]

Comment on attachment 129661 [details]
Patch

- I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement.
- Pelase split  firstRendererOf() and lastRendererOf() refactoring in separate patch.
   Having it in the same change makes it really hard to see  the actual change.

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> (From update of attachment 129661 [details])
> - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement.

I think we do need this behind flag. Otherwise, there are visible effects of the Shadow DOM implementation even when the flag is enabled. See bug 79935 for details.

> - Pelase split  firstRendererOf() and lastRendererOf() refactoring in separate patch.
>    Having it in the same change makes it really hard to see  the actual change.

[Hajime Morrita]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 129661 [details] [details])
> > - I don't think we need to hide HTMLShadowElement behind the flag it can be like as HTMLContentElement.
> 
> I think we do need this behind flag. Otherwise, there are visible effects of the Shadow DOM implementation even when the flag is enabled. See bug 79935 for details.
> 
> > - Pelase split  firstRendererOf() and lastRendererOf() refactoring in separate patch.
> >    Having it in the same change makes it really hard to see  the actual change.

Let me clarify, I think:
- It's OK to have HTMLShadowElement.cpp/h files for  regardless of ENABLE(SHADOW_DOM).
- It's NOT ok to have HTMLShadowElement.idl compiled in unless we ENABLE(SHADOW_DOM).
- For tag name, I think we need to fix Bug 79935 WITHOUT hiding HTMLContentElement.h/cpp behind ENABLE(SHADOW_DOM).
   It should be hidden by runtime flag, not compilation flag. I guess we need parser support here. Let me investigate this.

If we can pull up most of the code from HTMLContentElement to InsertionPoint, and can implement built-in shadows 
using only InsertionPoint without using HTMLContentElement. That would be best.

[Shinya Kawanaka]

Created attachment 130067 [details]
WIP

[Build Bot]

Comment on attachment 130067 [details]
WIP

Attachment 130067 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11803879

[Build Bot]

Comment on attachment 130067 [details]
WIP

Attachment 130067 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11801839

[Early Warning System Bot]

Comment on attachment 130067 [details]
WIP

Attachment 130067 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11802866

[WebKit Review Bot]

Comment on attachment 130067 [details]
WIP

Attachment 130067 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11801847

[Gyuyoung Kim]

Comment on attachment 130067 [details]
WIP

Attachment 130067 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11802879

[Gustavo Noronha (kov)]

Comment on attachment 130067 [details]
WIP

Attachment 130067 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11809291

[Shinya Kawanaka]

Created attachment 130550 [details]
Patch

[Shinya Kawanaka]

Created attachment 130553 [details]
Patch

[Hajime Morrita]

Comment on attachment 130553 [details]
Patch

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

> Source/WebCore/html/shadow/HTMLShadowElement.cpp:64
> +    if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode()))

Why not treeScope()? shadowTreeRootNode() traverses down the tree and better be avoided if possible.

> Source/WebCore/html/shadow/InsertionPoint.cpp:52
>      if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) {

Ditto.

> Source/WebCore/html/shadow/InsertionPoint.h:71
> +    ShadowRoot* shadowRootAssignedFrom;

I don't think we need this.

> Source/WebCore/html/shadow/InsertionPoint.h:76
>      if (!node || node->isContentElement())

It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint().
It also even don't need to stay in Node. can be pulled down to HTMLElement.

[Shinya Kawanaka]

> > Source/WebCore/html/shadow/InsertionPoint.h:76
> >      if (!node || node->isContentElement())
> 
> It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint().
> It also even don't need to stay in Node. can be pulled down to HTMLElement.

Let's do this in another bug?
I don't think it's good idea to merge this refactoring into this patch...

[Shinya Kawanaka]

(In reply to comment #28)
> > > Source/WebCore/html/shadow/InsertionPoint.h:76
> > >      if (!node || node->isContentElement())
> > 
> > It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint().
> > It also even don't need to stay in Node. can be pulled down to HTMLElement.
> 
> Let's do this in another bug?
> I don't think it's good idea to merge this refactoring into this patch...

And I want to remove isShadowElement from Node.

[Shinya Kawanaka]

Created attachment 130561 [details]
Patch

[Shinya Kawanaka]

Created attachment 130791 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #27)
> (From update of attachment 130553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130553&action=review
> 
> > Source/WebCore/html/shadow/HTMLShadowElement.cpp:64
> > +    if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode()))
> 
> Why not treeScope()? shadowTreeRootNode() traverses down the tree and better be avoided if possible.

Done.

> 
> > Source/WebCore/html/shadow/InsertionPoint.cpp:52
> >      if (ShadowRoot* root = toShadowRoot(shadowTreeRootNode())) {
> 
> Ditto.

Done.

> 
> > Source/WebCore/html/shadow/InsertionPoint.h:71
> > +    ShadowRoot* shadowRootAssignedFrom;
> 
> I don't think we need this.

Done.

> 
> > Source/WebCore/html/shadow/InsertionPoint.h:76
> >      if (!node || node->isContentElement())
> 
> It looks Node::isContentElement() is no longer need and can be morphed to isInsertionPoint().
> It also even don't need to stay in Node. can be pulled down to HTMLElement.

Already fixed in another patch

[Hajime Morrita]

Comment on attachment 130791 [details]
Patch

Looks good. Let's land this once bots become green.

[Shinya Kawanaka]

Committed r110161: <http://trac.webkit.org/changeset/110161>