RESOLVED FIXED 61089
JoinTextNodesCommand is never used
https://bugs.webkit.org/show_bug.cgi?id=61089
Summary JoinTextNodesCommand is never used
Ryosuke Niwa
Reported 2011-05-18 13:16:00 PDT
This command is never used anywhere. We should just get rid of it.
Attachments
Deleted JoinTextNodesCommand (16.82 KB, patch)
2011-05-18 21:41 PDT, Ryosuke Niwa
no flags
Ryosuke Niwa
Comment 1 2011-05-18 21:41:44 PDT
Created attachment 94042 [details] Deleted JoinTextNodesCommand
Ojan Vafai
Comment 2 2011-05-19 07:46:21 PDT
Did this behavior just get rolled into something else or do we just create infinite text nodes every edit you make?
Darin Adler
Comment 3 2011-05-19 07:47:03 PDT
Comment on attachment 94042 [details] Deleted JoinTextNodesCommand Lets say you have this markup: x<b>y</b>z Wouldn’t we want to join the x and z into a single text node after that? If not, why not? Is this handled with different code?
Darin Adler
Comment 4 2011-05-19 07:47:29 PDT
I meant if you deleted the "y" and then moved the insertion point away.
Darin Adler
Comment 5 2011-05-19 07:47:58 PDT
I see now that my question and Ojan’s are the same.
Ryosuke Niwa
Comment 6 2011-05-19 08:14:12 PDT
(In reply to comment #3) > (From update of attachment 94042 [details]) > Lets say you have this markup: > > x<b>y</b>z > > Wouldn’t we want to join the x and z into a single text node after that? If not, why not? Is this handled with different code? Yes: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/ApplyStyleCommand.cpp#L1470
Ryosuke Niwa
Comment 7 2011-05-19 08:22:48 PDT
I don't know how other editing commands are merging text nodes. Maybe we never do. For example, https://bugs.webkit.org/show_bug.cgi?id=61094 is caused by deletion leaving two adjacent text nodes.
Darin Adler
Comment 8 2011-05-19 08:28:11 PDT
Sorry, I was talking about deletion and not styling, but it’s good to see there is code in the styling code path that does this.
Darin Adler
Comment 9 2011-05-19 08:29:15 PDT
It's also interesting to consider: vw<b>x</b>yz Selecting starting before the w and after the y and then deleting or typing new text. I assume we’d want a single text node.
Darin Adler
Comment 10 2011-05-19 08:30:31 PDT
Comment on attachment 94042 [details] Deleted JoinTextNodesCommand It seems OK to remove this because generally speaking there’s no real value to keeping around dead untested code. I’m not sure this offers significant advantages over the explicit insert and delete idiom used in ApplyStyleCommand. We can add it back if we need it later.
Ryosuke Niwa
Comment 11 2011-05-19 08:32:37 PDT
(In reply to comment #10) > (From update of attachment 94042 [details]) > It seems OK to remove this because generally speaking there’s no real value to keeping around dead untested code. I’m not sure this offers significant advantages over the explicit insert and delete idiom used in ApplyStyleCommand. We can add it back if we need it later. Thanks for the review. My intent here was to remove dead code. Regardless of usefulness, I'd rather not have dead code in our code base.
WebKit Commit Bot
Comment 12 2011-05-19 10:44:44 PDT
The commit-queue encountered the following flaky tests while processing attachment 94042 [details]: http/tests/websocket/tests/httponly-cookie.pl bug 54097 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 13 2011-05-19 10:46:21 PDT
Comment on attachment 94042 [details] Deleted JoinTextNodesCommand Clearing flags on attachment: 94042 Committed r86854: <http://trac.webkit.org/changeset/86854>
WebKit Commit Bot
Comment 14 2011-05-19 10:46:27 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 15 2011-05-19 11:46:54 PDT
The commit-queue encountered the following flaky tests while processing attachment 94042 [details]: animations/suspend-resume-animation.html bug 48161 (authors: cmarrin@apple.com and simon.fraser@apple.com) http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.
Note You need to log in before you can comment on or make changes to this bug.