Bug 79854 – [Refactoring] Shadow related attach paths should be in ShadowTree.

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

[Hajime Morrita]

Some shadow related code is remaining in Element.cpp, which better live in ShadowTree.

[Hajime Morrita]

Created attachment 129395 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 129395 [details]
Patch

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

r=me provided comments below are addressed.

> Source/WebCore/ChangeLog:13
> +        Even after this change, there are some storange looking parts in ShadowTree attachment.

Nit: s/storange/strange/. But what is "strange" about them? Could you be more explicit?

> Source/WebCore/dom/ContainerNode.cpp:770
> +    attachAsNode();

I don't think this is a good change.  Node::attach() is much clearer.

> Source/WebCore/dom/ContainerNode.cpp:779
> -    Node::detach();
> +    detachAsNode();

Ditto.

[Hajime Morrita]

Considering code is polished enough, I expect next round will fine final ;-)

[Shinya Kawanaka]

Comment on attachment 129395 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:777
> +    detachChildren();

This means detaching children twice? We should remove for-loop?

[Hajime Morrita]

Committed r109203: <http://trac.webkit.org/changeset/109203>

[Shinya Kawanaka]

Comment on attachment 129395 [details]
Patch

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

> Source/WebCore/dom/ShadowTree.cpp:176
> +

this empty line is not necessary.