[webkit-reviews] review denied: [Bug 50012] Moving cursor down in table cycles at the end of a row : [Attachment 80981] Patch

Wed Feb 2 15:53:11 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 50012: Moving cursor down in table cycles at the end of a row
https://bugs.webkit.org/show_bug.cgi?id=50012

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

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

Ah great!  I had imagined this would have been much harder to fix.  Good job
fixing it.  r- for stylistic issues.

> Source/WebCore/ChangeLog:7
> +

Please explain what caused the bug and how you fixed it. You can almost copy &
paste your comment #c9.

> Source/WebCore/editing/visible_units.cpp:589
>      while (n) {
>	   if (editable == n->isContentEditable())
>	       return n;

It's sad that this while loop is identical to that of the function below yet we
don't share code.  Is there anyway we can share code?

> LayoutTests/ChangeLog:7
> +

Please explain what kind of test you're adding, etc...

> LayoutTests/ChangeLog:9
> +	   * editing/selection/move-by-line-006-expected.txt: Added.
> +	   * editing/selection/move-by-line-006.html: Added.

It would be nice if the test name reflected what the test is about rather than
just having a number 006.

> LayoutTests/editing/selection/move-by-line-006.html:1
> +<html>

Missing <!DOCTYPE html>

> LayoutTests/editing/selection/move-by-line-006.html:17
> +    <script src=../editing.js language="JavaScript" type="text/JavaScript"
></script>
> +    <script>
> +	   function test()
> +	   {
> +	       if (window.layoutTestController)
> +		   layoutTestController.dumpAsText();
> +
> +	       var target = document.getElementById("target");
> +	       getSelection().setBaseAndExtent(target.firstChild, 0,
target.firstChild, 0);
> +	       for (var i=0; i<2; i++)
> +		   moveSelectionForwardByLineCommand();
> +
> +	       document.getElementById("result").innerText =
getSelection().baseOffset == 6 ? "PASS" : "FAIL";
> +	   }
> +    </script>

We don't normally indent tags like this.  And I don't think we need to wait
until load event.  You can just run all of them when the parser inserts the
script element into the document if you moved the script element after table. 
If you do that, then you can just use document.write to print PASS/FAIL instead
of having a #result paragraph.

Also, no space between i and 2, and no need to specify type and language in the
script tag.

> LayoutTests/editing/selection/move-by-line-006.html:21
> +    <p>
> +	   Test for <i><a
href="http://bugs.webkit.org/show_bug.cgi?id=50012">http://bugs.webkit.org/show
_bug.cgi?id=50012</a>

Ditto about indentation.