| Summary: | JoinTextNodesCommand is never used | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
| Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Enhancement | CC: | commit-queue, darin, dglazkov, enrica, eric, justin.garcia, leviw, morrita, ojan, sullivan, tkent, tony | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Ryosuke Niwa
2011-05-18 13:16:00 PDT
Created attachment 94042 [details]
Deleted JoinTextNodesCommand
Did this behavior just get rolled into something else or do we just create infinite text nodes every edit you make? 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?
I meant if you deleted the "y" and then moved the insertion point away. I see now that my question and Ojan’s are the same. (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 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. 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. 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.
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.
(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. 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. Comment on attachment 94042 [details] Deleted JoinTextNodesCommand Clearing flags on attachment: 94042 Committed r86854: <http://trac.webkit.org/changeset/86854> All reviewed patches have been landed. Closing bug. 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. |