Bug 79854 - [Refactoring] Shadow related attach paths should be in ShadowTree.
Summary: [Refactoring] Shadow related attach paths should be in ShadowTree.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 77503
  Show dependency treegraph
 
Reported: 2012-02-28 18:04 PST by Hajime Morrita
Modified: 2012-02-29 00:11 PST (History)
1 user (show)

See Also:


Attachments
Patch (9.51 KB, patch)
2012-02-28 23:00 PST, Hajime Morrita
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-02-28 18:04:27 PST
Some shadow related code is remaining in Element.cpp, which better live in ShadowTree.
Comment 1 Hajime Morrita 2012-02-28 23:00:39 PST
Created attachment 129395 [details]
Patch
Comment 2 Ryosuke Niwa 2012-02-28 23:14:51 PST
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.
Comment 3 Hajime Morrita 2012-02-28 23:48:30 PST
Considering code is polished enough, I expect next round will fine final ;-)
Comment 4 Shinya Kawanaka 2012-02-29 00:09:44 PST
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?
Comment 5 Hajime Morrita 2012-02-29 00:09:57 PST
Committed r109203: <http://trac.webkit.org/changeset/109203>
Comment 6 Shinya Kawanaka 2012-02-29 00:11:38 PST
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.