[webkit-reviews] review denied: [Bug 25321] RLE/LRE characters are removed from any text entered into a text box (either using a keyboard or by pasting) : [Attachment 65626] patch w/ layout test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 25321: RLE/LRE characters are removed from any text entered into a text box
(either using a keyboard or by pasting)
https://bugs.webkit.org/show_bug.cgi?id=25321

Attachment 65626: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=65626&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +static bool unicodeBiDiControlCharacter(UChar ch)

This function name needs the word "is" in it, because it does not return a
"Unicode bi di control character". It also makes no sense to capitalize the "D"
in "bidi", which is short for "bidirectional".

> +		   if (textOffset == box->start() + box->len() + 1 &&
unicodeBiDiControlCharacter((static_cast<Text*>(currentNode))->data()[textOffse
t - 1]))
> +		       return currentPos;

This code needs a why comment. Why doe this one particular function need a
special case for Unicode bidirectional control characters. Aren't the other
characters that need this sort of special handling? What makes these
bidirectional control characters so important.

>      // Insert the character at the leftmost candidate.
> +    startPosition = startPosition.downstream();
>      startPosition = startPosition.upstream();

This code needs a why comment. Otherwise someone would just remove the call to
downstream.

These issues are enough for a review-, but I’d also like Dan Bernstein to
comment on the appropriate way of fixing this.