Summary: | [Refactoring] attach() following detach() should be replaced with Node::forceReattach() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||
Component: | DOM | Assignee: | Hajime Morrita <morrita> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, dominicc | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Hajime Morrita
2011-05-17 18:18:32 PDT
Is there anything special about forceReattach compared to detach and attach? (eg detach and attach a bug because ...?, or just slow because ...?) (In reply to comment #1) > Is there anything special about forceReattach compared to detach and attach? (eg detach and attach a bug because ...?, or just slow because ...?) Nothing special, it's just a shorthand. Also, it's something we should work to replace every instance of detach()/attach() (anti)pattern with something smarter or gentler. Rounding all occurrences into one place seems like a good start. Created attachment 94184 [details]
Patch
Comment on attachment 94184 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94184&action=review Thank you for taking on this! > Source/WebCore/ChangeLog:9 > + - Replaced detach() following attach() with reattach() Great name! > Source/WebCore/dom/CharacterData.cpp:185 > + reattach(); This will never hit unless the node is already attached, so the value of ReattachType is irrelevant here. > Source/WebCore/dom/Node.h:454 > + void reattach(ReattachType = ReattachIfAttached); Looking at the callsites, I think ReattachAlways should be the default. > Source/WebCore/dom/Node.h:746 > + if (!attached()) { > + if (type == ReattachAlways) > + attach(); This seems wrong. Why are we attempting to attach here? Created attachment 94373 [details]
Patch
Comment on attachment 94184 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94184&action=review Hi Dimitri, thank you for the first round! Here is update. >> Source/WebCore/dom/CharacterData.cpp:185 >> + reattach(); > > This will never hit unless the node is already attached, so the value of ReattachType is irrelevant here. I splited reattach() and reattachIfAttached() an use reattach() here. >> Source/WebCore/dom/Node.h:454 >> + void reattach(ReattachType = ReattachIfAttached); > > Looking at the callsites, I think ReattachAlways should be the default. Now there are reattach() and reattachIfAttached(). >> Source/WebCore/dom/Node.h:746 >> + attach(); > > This seems wrong. Why are we attempting to attach here? Makes sense. I clarified this by splitting the function into two. Created attachment 94375 [details]
synced to HEAD
Committed r87123: <http://trac.webkit.org/changeset/87123> |