Bug 107640 - Refactoring: The name ContainerNode::removeChildren and ContainerNde::removeAllChilren() is confusing
Summary: Refactoring: The name ContainerNode::removeChildren and ContainerNde::removeA...
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: 107634 107790
Blocks: 107801
  Show dependency treegraph
 
Reported: 2013-01-23 00:15 PST by Hajime Morrita
Modified: 2013-01-29 12:25 PST (History)
21 users (show)

See Also:


Attachments
Patch (8.47 KB, patch)
2013-01-23 20:40 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2013-01-23 22:56 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2013-01-23 22:58 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (8.44 KB, patch)
2013-01-24 01:11 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (8.45 KB, patch)
2013-01-24 22:44 PST, Hajime Morrita
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2013-01-23 00:15:55 PST
These names are confusing. removeAllChilren() is unsafe operation and should be named like that.
Comment 1 Hajime Morrita 2013-01-23 20:40:38 PST
Created attachment 184392 [details]
Patch
Comment 2 Early Warning System Bot 2013-01-23 20:59:19 PST
Comment on attachment 184392 [details]
Patch

Attachment 184392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16079523
Comment 3 Eric Seidel (no email) 2013-01-23 22:20:28 PST
Comment on attachment 184392 [details]
Patch

Is this behavior change observable?
Comment 4 Ryosuke Niwa 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?
Comment 5 Early Warning System Bot 2013-01-23 22:27:37 PST
Comment on attachment 184392 [details]
Patch

Attachment 184392 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16073621
Comment 6 Hajime Morrita 2013-01-23 22:56:04 PST
Created attachment 184409 [details]
Patch
Comment 7 Hajime Morrita 2013-01-23 22:58:33 PST
Created attachment 184410 [details]
Patch
Comment 8 Hajime Morrita 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.
Comment 9 Ryosuke Niwa 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?
Comment 10 Hajime Morrita 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.
Comment 11 Hajime Morrita 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.
Comment 12 Ryosuke Niwa 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?
Comment 13 Early Warning System Bot 2013-01-23 23:07:06 PST
Comment on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084510
Comment 14 Benjamin Poulain 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.
Comment 15 Early Warning System Bot 2013-01-23 23:14:04 PST
Comment on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16097033
Comment 16 Build Bot 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
Comment 17 WebKit Review Bot 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
Comment 18 Build Bot 2013-01-23 23:49:46 PST
Comment on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16082528
Comment 19 Peter Beverloo (cr-android ews) 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
Comment 20 Early Warning System Bot 2013-01-24 00:15:21 PST
Comment on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16082548
Comment 21 Hajime Morrita 2013-01-24 01:11:49 PST
Created attachment 184435 [details]
Patch
Comment 22 Hajime Morrita 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.
Comment 23 Eric Seidel (no email) 2013-01-24 01:13:52 PST
Comment on attachment 184435 [details]
Patch

OK, so this is just a renaming.
Comment 24 Ryosuke Niwa 2013-01-24 01:14:55 PST
Yay!
Comment 25 Benjamin Poulain 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.
Comment 26 Hajime Morrita 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.
Comment 27 Hajime Morrita 2013-01-24 22:44:09 PST
Created attachment 184672 [details]
Patch for landing
Comment 28 WebKit Review Bot 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
Comment 29 Hajime Morrita 2013-01-24 22:47:28 PST
Committed r140784: <http://trac.webkit.org/changeset/140784>
Comment 30 Darin Adler 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!