[webkit-reviews] review granted: [Bug 5241] Space and tab characters "sent" by an input method give totally different results than typing them directly : [Attachment 79545] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted MORITA Hajime
<morrita at google.com>'s request for review:
Bug 5241: Space and tab characters "sent" by an input method give totally
different results than typing them directly
https://bugs.webkit.org/show_bug.cgi?id=5241

Attachment 79545: Patch
https://bugs.webkit.org/attachment.cgi?id=79545&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79545&action=review

r=me provided you respond to the following two comments.

> LayoutTests/editing/inserting/insert-composition-whitespace.html:65
> +test("div", " ", "\xA0");
> +test("div", "  ", "\xA0\xA0");
> +test("div", "   ", "\xA0\xA0\xA0");

This isn't right.  Why are we having 3 nbsp's here without any regular spaces?

This might be a bug in stringWithRebalancedWhitespace.	It inserts nbsp at the
beginning and the end AFTER replacing twoSpaces with a pattern.  We should
probably putting nbsp's at each end first and then replace two spaces with the
pattern.  That could be a separate fix but we should definitely file a bug, and
refer to the bug in the test if we're not fixing it in this patch.

> Source/WebCore/editing/CompositeEditCommand.cpp:439
> +    String text = static_cast<Text*>(node)->data();

Nit: I think there's an extra space here before String.

> Source/WebCore/editing/CompositeEditCommand.cpp:446
> +    rebalanceWhitespaceOnTextSubstring(static_cast<Text*>(node),
position.deprecatedEditingOffset(), position.deprecatedEditingOffset());

We should not be calling deprecatedEditingOffset().  Because we've already
checked that position is PositionIsOffsetInAnchor, we should be calling
offsetInContainerNode() instead.