[webkit-reviews] review denied: [Bug 25298] Ctrl + Right/Left arrow move forward/backward through document instead of right/left in RTL text : [Attachment 58093] path /w layout test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 25298: Ctrl + Right/Left arrow move forward/backward through document
instead of right/left in RTL text
https://bugs.webkit.org/show_bug.cgi?id=25298

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

------- Additional Comments from mitz at webkit.org
Thanks for your patch! I have some comments on this version:

Does it really make sense to use a word break iterator on a string in visual
order? I would think that the word break iterator is meant to be use on a
logically-ordered string. For example, for languages where word breaks are
based on a dictionary, like Thai, I don’t think this will work.

> +    Node* de = d->documentElement();

Please use a longer name for this variable, like “documentElement”.

> +    while (box) {
> +	   node = box->renderer()->node();
> +	   // Get strings in visual order.
> +	   if (box->direction() == LTR) {
> +	       range->setStart(node, box->caretMinOffset(), ec);
> +	       startNode ? range->setEnd(node, offset, ec) :
range->setEnd(node, box->caretMaxOffset(), ec);

Normally the ternary operator is only used in WebKit when the result is used
(for example, as a parameter to a function or assigned to a variable). In other
cases, an if statement is used.

> +	       SimplifiedBackwardsTextIterator it(range.get());

I am confused by the way you set up the range and use of a backwards text
iterator. I am really not sure how adjusting only one end of the range makes
sense; the code seems to make some assumptions about how the text iterator
works and what it returns first (you never advance the iterator) which it
should probably not be making.

> +	       string.prepend(it.characters(), it.length());
> +	   } else {
> +	       startNode ? range->setStart(node, offset, ec) :
range->setStart(node, box->caretMinOffset(), ec);
> +	       range->setEnd(node, box->caretMaxOffset(), ec);
> +	       SimplifiedBackwardsTextIterator it(range.get());
> +	       for (int i = 0; i < it.length(); ++i)
> +		   string.prepend(it.characters()[i]);

Like I said, I doubt that reversing the string will always give the correct
result. Moreover, you cannot reverse a string like this, because it will break
surrogate pairs (characters that are encoded by two UTF-16 characters).

> +	   prev = previousWordPositionBoundary(string.data(), string.size(),
string.size(), DontHaveMoreContext, needMoreContext);

Is it really true that you couldn’t provide more context to the break iterator
if needed?

> +VisiblePosition rightWordPosition(const VisiblePosition& c)

A lot of the code in this function is identical or similar to
leftWordPosition(). The common code should not be repeated. It should be
factored out (probably into file-static functions).

I think it’s great that you’ve added so many tests! Do you know if the results
in these tests match the behavior of the native text system on Mac OS X (that
is, does it behave like TextEdit?)? What about other platforms’ native text
systems?

It’s especially good that you have these tests and the expected results,
because I think you need to take a significantly different approach to this
bug, and the tests will allow you to verify that the new approach produces the
same (or better) results. It seems like the behavior you were going for here is
to return the (visually) nearest position to the left (respectively, to the
right) that is a word boundary. A naive algorithm could just iterate over word
boundaries (logically) forward and backwards along the line, then pick the one
nearest to the starting position to the left (resp. to the right). Does that
sounds like a correct description of the desired behavior? If so, perhaps the
right thing to do would be to start with an implementation of the naive
algorithm, then see how it can be made reasonably efficient.