Summary: | Simplify ReplaceSelectionCommand::positionAtStartOfInsertedContent | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, enrica, eric, morrita, ojan, tkent, tony | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 68874 | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-09-27 14:37:25 PDT
Created attachment 108916 [details]
Simplified the function with additional fixes and refactoring
Comment on attachment 108916 [details] Simplified the function with additional fixes and refactoring View in context: https://bugs.webkit.org/attachment.cgi?id=108916&action=review > Source/WebCore/rendering/RenderText.cpp:1606 > -unsigned RenderText::caretMaxRenderedOffset() const > +unsigned RenderText::renderedTextLength() const Come to think of it, caretMaxOffset should call this function. Will do in a minute. (In reply to comment #2) > (From update of attachment 108916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108916&action=review > > > Source/WebCore/rendering/RenderText.cpp:1606 > > -unsigned RenderText::caretMaxRenderedOffset() const > > +unsigned RenderText::renderedTextLength() const > > Come to think of it, caretMaxOffset should call this function. Will do in a minute. Ugh... not really. That won't work. Created attachment 108926 [details]
new patch
Comment on attachment 108926 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=108926&action=review > Source/WebCore/ChangeLog:12 > + This change revealed a bug in removeUnrenderedTextNodesAtEnds that text nodes without any visible > + text at ends are not removed when it has a render object. Fixed the bug by checking the length of > + the rendered text. Since you fixed a bug we need to add a test case to show that bug in action, or call out which test cases shows us the bug. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:559 > + return node->renderer() && node->renderer()->isText() && toRenderText(node->renderer())->renderedTextLength() > 0; I think that text nodes are guaranteed to have RenderText renderers or no renderer at all, so I don’t think the isText check is needed. If it is needed, we should check in a test case that exercises that code path. Comment on attachment 108926 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=108926&action=review >> Source/WebCore/ChangeLog:12 >> + the rendered text. > > Since you fixed a bug we need to add a test case to show that bug in action, or call out which test cases shows us the bug. It's editing/pasteboard/pasting-word-in-div-extra-line.html. >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:559 >> + return node->renderer() && node->renderer()->isText() && toRenderText(node->renderer())->renderedTextLength() > 0; > > I think that text nodes are guaranteed to have RenderText renderers or no renderer at all, so I don’t think the isText check is needed. If it is needed, we should check in a test case that exercises that code path. On the other hand, I didn't want to make this function hard to use. I don't want someone to start calling this function without checking that node is a text node. I guess I can change the argument type to Text*? Created attachment 108943 [details]
Addressed comments
Thanks for reviewing these patches! Committed r96187: <http://trac.webkit.org/changeset/96187> |