Refactoring: The name ContainerNode::removeChildren and ContainerNde::removeAllChilren() is confusing

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

These names are confusing. removeAllChilren() is unsafe operation and should be named like that.

[Hajime Morrita]

Created attachment 184392 [details]
Patch

[Early Warning System Bot]

Comment on attachment 184392 [details]
Patch

Attachment 184392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16079523

[Eric Seidel (no email)]

Comment on attachment 184392 [details]
Patch

Is this behavior change observable?

[Ryosuke Niwa]

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 on attachment 184392 [details]
Patch

Attachment 184392 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16073621

[Hajime Morrita]

Created attachment 184409 [details]
Patch

[Hajime Morrita]

Created attachment 184410 [details]
Patch

[Hajime Morrita]

> 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 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]

(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]

(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 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 on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084510

[Benjamin Poulain]

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 on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16097033

[Build Bot]

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 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 on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16082528

[Peter Beverloo (cr-android ews)]

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 on attachment 184410 [details]
Patch

Attachment 184410 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16082548

[Hajime Morrita]

Created attachment 184435 [details]
Patch

[Hajime Morrita]

(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 on attachment 184435 [details]
Patch

OK, so this is just a renaming.

[Ryosuke Niwa]

Yay!

[Benjamin Poulain]

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]

(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]

Created attachment 184672 [details]
Patch for landing

[WebKit Review Bot]

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]

Committed r140784: <http://trac.webkit.org/changeset/140784>

[Darin Adler]

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!