WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug