RESOLVED FIXED 107640
Refactoring: The name ContainerNode::removeChildren and ContainerNde::removeAllChilren() is confusing
https://bugs.webkit.org/show_bug.cgi?id=107640
Summary Refactoring: The name ContainerNode::removeChildren and ContainerNde::removeA...
Hajime Morrita
Reported 2013-01-23 00:15:55 PST
These names are confusing. removeAllChilren() is unsafe operation and should be named like that.
Attachments
Patch (8.47 KB, patch)
2013-01-23 20:40 PST, Hajime Morrita
no flags
Patch (10.52 KB, patch)
2013-01-23 22:56 PST, Hajime Morrita
no flags
Patch (10.33 KB, patch)
2013-01-23 22:58 PST, Hajime Morrita
no flags
Patch (8.44 KB, patch)
2013-01-24 01:11 PST, Hajime Morrita
no flags
Patch for landing (8.45 KB, patch)
2013-01-24 22:44 PST, Hajime Morrita
webkit.review.bot: commit-queue-
Hajime Morrita
Comment 1 2013-01-23 20:40:38 PST
Early Warning System Bot
Comment 2 2013-01-23 20:59:19 PST
Eric Seidel (no email)
Comment 3 2013-01-23 22:20:28 PST
Comment on attachment 184392 [details] Patch Is this behavior change observable?
Ryosuke Niwa
Comment 4 2013-01-23 22:25:32 PST
Comment on attachment 184392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184392&action=review > Source/WebCore/dom/ContainerNode.cpp:105 > - oldParent->removeAllChildren(); > + // removeDetachedChildren() cannot be used here sicne takeAllChildrenFrom() violates its precondition > + // and does detach() its children after the removal. > + removeAllChildrenInContainer<Node, ContainerNode>(this); Should we isolate put this in a separate patch so that we can merge it?
Early Warning System Bot
Comment 5 2013-01-23 22:27:37 PST
Hajime Morrita
Comment 6 2013-01-23 22:56:04 PST
Hajime Morrita
Comment 7 2013-01-23 22:58:33 PST
Hajime Morrita
Comment 8 2013-01-23 23:00:47 PST
> Should we isolate put this in a separate patch so that we can merge it? No longer necessary. I gave up to add an assert() since some tets hit it. I hope I could understand why it can happen. But it looks far from trivial.
Ryosuke Niwa
Comment 9 2013-01-23 23:01:12 PST
Comment on attachment 184410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184410&action=review > Source/WebCore/ChangeLog:33 > + (WebCore::InputType::destroyShadowSubtree): Replaced a wrong call site. > + * html/ValidationMessage.cpp: > + (WebCore::ValidationMessage::setMessageDOMAndStartTimer): Replaced a wrong call site. > + * html/parser/HTMLTreeBuilder.cpp: > + (WebCore::HTMLTreeBuilder::processEndTag): Replaced a wrong call site. Why don't we fix these calls in a separate patch first?
Hajime Morrita
Comment 10 2013-01-23 23:01:45 PST
(In reply to comment #8) > > Should we isolate put this in a separate patch so that we can merge it? > No longer necessary. I gave up to add an assert() since some tets hit it. > I hope I could understand why it can happen. But it looks far from trivial. Note that these failure isn't new to this change. The new assert just unveiled it.
Hajime Morrita
Comment 11 2013-01-23 23:02:20 PST
(In reply to comment #9) > > + (WebCore::InputType::destroyShadowSubtree): Replaced a wrong call site. > > + * html/ValidationMessage.cpp: > > + (WebCore::ValidationMessage::setMessageDOMAndStartTimer): Replaced a wrong call site. > > + * html/parser/HTMLTreeBuilder.cpp: > > + (WebCore::HTMLTreeBuilder::processEndTag): Replaced a wrong call site. > > Why don't we fix these calls in a separate patch first? Right, will do.
Ryosuke Niwa
Comment 12 2013-01-23 23:06:21 PST
Comment on attachment 184410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184410&action=review > Source/WebCore/ChangeLog:3 > + The name ContainerNode::removeChildren and ContainerNde::removeAllChildren() is confusing Typo: Nde. > Source/WebCore/ChangeLog:8 > + This change renames removeAllChildren() to removeDeletingChildren(), and hide as a protected method I don't think the difference between removeChildren and removeDeletingChildren is clear. Maybe removeChildrenWithoutNotifications? Or maybe removeChildrenAfterDeletionHasBegun? Or maybe even removeChildrenInDestructor?
Early Warning System Bot
Comment 13 2013-01-23 23:07:06 PST
Benjamin Poulain
Comment 14 2013-01-23 23:12:12 PST
Comment on attachment 184410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184410&action=review > Source/WebCore/ChangeLog:9 > + This change renames removeAllChildren() to removeDeletingChildren(), and hide as a protected method > + for preventing future misuse. Suggestion for the name: removeAndDelegeChildren(). > Source/WebCore/dom/ContainerNode.cpp:95 > removeAllChildrenInContainer<Node, ContainerNode>(this); Why not rename this accordingly? This does not seem to be less confusing.
Early Warning System Bot
Comment 15 2013-01-23 23:14:04 PST
Build Bot
Comment 16 2013-01-23 23:34:59 PST
Comment on attachment 184410 [details] Patch Attachment 184410 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16074601
WebKit Review Bot
Comment 17 2013-01-23 23:36:20 PST
Comment on attachment 184410 [details] Patch Attachment 184410 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16065920
Build Bot
Comment 18 2013-01-23 23:49:46 PST
Peter Beverloo (cr-android ews)
Comment 19 2013-01-23 23:56:19 PST
Comment on attachment 184410 [details] Patch Attachment 184410 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16083536
Early Warning System Bot
Comment 20 2013-01-24 00:15:21 PST
Hajime Morrita
Comment 21 2013-01-24 01:11:49 PST
Hajime Morrita
Comment 22 2013-01-24 01:13:07 PST
(In reply to comment #14) > (From update of attachment 184410 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184410&action=review > > > Source/WebCore/ChangeLog:9 > > + This change renames removeAllChildren() to removeDeletingChildren(), and hide as a protected method > > + for preventing future misuse. > > Suggestion for the name: removeAndDelegeChildren(). Well, a node isn't deleted when it is referred from somewhere else, like JS. That means the term "deleting" is also wrong. I'd choose my first choice "detached". > > > Source/WebCore/dom/ContainerNode.cpp:95 > > removeAllChildrenInContainer<Node, ContainerNode>(this); > > Why not rename this accordingly? > This does not seem to be less confusing. Done.
Eric Seidel (no email)
Comment 23 2013-01-24 01:13:52 PST
Comment on attachment 184435 [details] Patch OK, so this is just a renaming.
Ryosuke Niwa
Comment 24 2013-01-24 01:14:55 PST
Yay!
Benjamin Poulain
Comment 25 2013-01-24 01:15:24 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=184435&action=review Nits :) > Source/WebCore/ChangeLog:8 > + This chagne renames unsafe removeAllChilren() function to Typo: chagne > Source/WebCore/dom/ContainerNode.cpp:95 > + // FIXME: We should be able to ASSERT(!attached() here: https://bugs.webkit.org/show_bug.cgi?id=107801 Unbalanced parenthesis.
Hajime Morrita
Comment 26 2013-01-24 22:42:27 PST
(In reply to comment #25) > View in context: https://bugs.webkit.org/attachment.cgi?id=184435&action=review > > Nits :) Good catch :) will fix then land.
Hajime Morrita
Comment 27 2013-01-24 22:44:09 PST
Created attachment 184672 [details] Patch for landing
WebKit Review Bot
Comment 28 2013-01-24 22:45:34 PST
Comment on attachment 184672 [details] Patch for landing Rejecting attachment 184672 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 184672, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: e left on device patch: **** Can't create file /tmp/ppfHpsGX : No space left on device patch: **** Can't create file /tmp/ppS210tY : No space left on device patch: **** Can't create file /tmp/ppMPPp8X : No space left on device patch: **** Can't create file /tmp/ppRkXRhY : No space left on device patch: **** Can't create file /tmp/pp7inRr1 : No space left on device Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16122135
Hajime Morrita
Comment 29 2013-01-24 22:47:28 PST
Darin Adler
Comment 30 2013-01-29 09:26:08 PST
This is a fantastic step forward. Thank you so much. When I wrote that FIXME two years ago I never thought it would be this long before we worked out what the problem was!
Note You need to log in before you can comment on or make changes to this bug.