[webkit-reviews] review denied: [Bug 54534] RTL lineboundary left/right is reversed when cursor is at start of RTL container : [Attachment 82744] Patch

Wed Feb 16 20:19:49 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Benjamin (Ben) Kalman
<kalman at chromium.org>'s request for review:
Bug 54534: RTL lineboundary left/right is reversed when cursor is at start of
RTL container
https://bugs.webkit.org/show_bug.cgi?id=54534

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

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

The change looks fine but r- because you should address at least some of the
style issues below:

> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:2
> +

I would personally put <body> than a blank line

> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:9
> +
> +<pre id="console"></pre>
> +

Are these blank lines needed?

> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:14
> +    function log(text) {
> +	  
document.getElementById("console").appendChild(document.createTextNode(text +
"\n"));
> +    }
> +

I don't think we need to indent all these functions.  It just bloats the
patch/file size.

> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:31
> +	   assertEquals("", getSelection().toString(), "extend left from
left");

Isn't it better to check the offset values here?  For example, this wouldn't
detect when the caret moved to the opposite side, right?

> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:51
> +    for (var i = 0; i < testContainers.length; i++) {
> +	   runTestForContainer(testContainers[i]);
> +    }

No curly brackets around one line statements.

> LayoutTests/editing/selection/extend-left-right-by-lineboundary.html:56
> +	   while (testContainers.length > 0) {
> +	       document.body.removeChild(testContainers[0]);
> +	   }

Ditto.

> Source/WebCore/editing/SelectionController.cpp:421
> -	   if (directionOfEnclosingBlock() == LTR)
> -	       pos = pos.next(true);
> -	   else
> -	       pos = pos.previous(true);
> +	   pos = (directionOfEnclosingBlock() == LTR) ?  pos.next(true) :
pos.previous(true);

I'm not sure if we should use ternary expression here.	It seems like we use
explicit if/else expressions everywhere else in this file.