| Summary: | [Refactoring] Shadow related attach paths should be in ShadowTree. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||
| Component: | DOM | Assignee: | Hajime Morrita <morrita> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | shinyak | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 77503 | ||||||
| Attachments: |
|
||||||
|
Description
Hajime Morrita
2012-02-28 18:04:27 PST
Created attachment 129395 [details]
Patch
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. Considering code is polished enough, I expect next round will fine final ;-) 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? Committed r109203: <http://trac.webkit.org/changeset/109203> 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. |