Bug 68939 - Simplify ReplaceSelectionCommand::positionAtStartOfInsertedContent
Summary: Simplify ReplaceSelectionCommand::positionAtStartOfInsertedContent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 68874
  Show dependency treegraph
 
Reported: 2011-09-27 14:37 PDT by Ryosuke Niwa
Modified: 2011-09-27 22:03 PDT (History)
7 users (show)

See Also:


Attachments
Simplified the function with additional fixes and refactoring (12.54 KB, patch)
2011-09-27 15:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
new patch (14.41 KB, patch)
2011-09-27 16:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed comments (14.48 KB, patch)
2011-09-27 17:38 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-09-27 14:37:25 PDT
I don't think we should be calling nextCandidate in positionAtStartOfInsertedContent. We need to simplify this function in order to resolve the bug 68874.
Comment 1 Ryosuke Niwa 2011-09-27 15:36:46 PDT
Created attachment 108916 [details]
Simplified the function with additional fixes and refactoring
Comment 2 Ryosuke Niwa 2011-09-27 16:07:19 PDT
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.
Comment 3 Ryosuke Niwa 2011-09-27 16:19:31 PDT
(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.
Comment 4 Ryosuke Niwa 2011-09-27 16:24:26 PDT
Created attachment 108926 [details]
new patch
Comment 5 Darin Adler 2011-09-27 17:15:40 PDT
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 6 Ryosuke Niwa 2011-09-27 17:22:23 PDT
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*?
Comment 7 Ryosuke Niwa 2011-09-27 17:38:42 PDT
Created attachment 108943 [details]
Addressed comments
Comment 8 Ryosuke Niwa 2011-09-27 18:12:23 PDT
Thanks for reviewing these patches!
Comment 9 Ryosuke Niwa 2011-09-27 22:03:17 PDT
Committed r96187: <http://trac.webkit.org/changeset/96187>