WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Hajime Morrita
Comment 1
2013-01-23 20:40:38 PST
Created
attachment 184392
[details]
Patch
Early Warning System Bot
Comment 2
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
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
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
Comment 6
2013-01-23 22:56:04 PST
Created
attachment 184409
[details]
Patch
Hajime Morrita
Comment 7
2013-01-23 22:58:33 PST
Created
attachment 184410
[details]
Patch
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
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 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
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 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
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 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
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
Comment 21
2013-01-24 01:11:49 PST
Created
attachment 184435
[details]
Patch
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
Committed
r140784
: <
http://trac.webkit.org/changeset/140784
>
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.
Top of Page
Format For Printing
XML
Clone This Bug