[webkit-reviews] review denied: [Bug 24303] Using keyboard select RTL text, Highlight goes to opposite direction from FF&IE : [Attachment 28815] patch w/ Layout test (version 2)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 24303: Using keyboard select RTL text, Highlight goes to opposite direction
from FF&IE
https://bugs.webkit.org/show_bug.cgi?id=24303

Attachment 28815: patch w/ Layout test (version 2)
https://bugs.webkit.org/attachment.cgi?id=28815&action=review

------- Additional Comments from mitz at webkit.org
> Looks like FF does not across block boundary when extending selection.
> But currently, Webkit across block boundary while extending selection if the
> blocks are parent-child. It can extend selection inward or outward.
> For example: 
> <div> whatever <div dir="rtl"> just a test</div> end</div>
> selection can be extended from the block "just a test" to "whatever ... end",

> and vice versa.
>
> In the current fix, when checking the directionality of enclosing block 
> (SelectionController::directionOfEnclosingBlock()), I used the starting point

> of the selection (m_sel.base().node()), not the ending point
> (m_sel.extend().node()).

What does Firefox do when the selection already crosses a block boundary and
the extent is in a block that has different directionality from the base? I
think using the directionality of the block enclosing the extent makes more
sense, because the extent is what is being modified, although I realize that it
may be weird when crossing into a different direction, like you said.

> +TextDirection SelectionController::directionOfEnclosingBlock() {
> +    Node *n = m_sel.base().node();

The * should go next to "Node".

> +    Node* enclosingBlockNode = enclosingBlock(n);
> +    if (enclosingBlockNode) {

You can write this as
       if (Node* enclosingBlockNode = enclosingBlock(n))...

or better yet, you can save some indentation by reversing the condition and
turning this into an early return:
       if (!enclosingBlockNode)
	   return LTR;

> +	   WebCore::RenderObject* renderer = enclosingBlockNode->renderer();

Since this function is in the WebCore namespace, you shouldn't add the
"WebCore::" prefix.

> +	   if (renderer) {
> +	       if (renderer->style()->direction() == WebCore::RTL) {
> +		   return RTL;
> +	       }

The WebKit coding style is to omit braces around single-line blocks like the
above. But the inner if statement is unnecessary. You can simply write
	   if (renderer)
	       return renderer->style()->direction();

> +	   case CharacterGranularity:
> +	       if (directionOfEnclosingBlock() == RTL) {
> +		   pos = pos.previous(true);
> +	       } else {
> +		   pos = pos.next(true);
> +	       }

Please remove the braces around those single-line blocks.

In existing WebCore code, sometimes the code for the RTL case comes first and
sometimes it comes second. In new code, I would rather make the LTR case come
first.

> +	   case WordGranularity:
> +	       if (directionOfEnclosingBlock() == RTL) {
> +		   pos = previousWordPosition(pos);
> +	       } else {
> +		   pos = nextWordPosition(pos);
> +	       }

Ditto re: braces and order.