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

Wed Feb 2 16:56:56 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has granted 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 81005: Patch
https://bugs.webkit.org/attachment.cgi?id=81005&action=review

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

I'd say r+ but please address the following comments before landing this patch.

> Source/WebCore/ChangeLog:11
> +	   Avoids a caret cycling issue with certain content (e.g. tables)
found at the very
> +	   end of a document due to a bug in nextLeafWithSameEditability.

Description should appear before tests.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:1
> +<html>

Please put <!DOCTYPE html>

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:3
> +  <script src=../editing.js language="JavaScript" type="text/JavaScript"
></script>

Please remove the indentation, and language/type attributes.  They just clutter
the test.

Also, you can move this into body right before another script element.	That
way, you can get rid of head.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:12
> +<script language="JavaScript" type="text/JavaScript">

Ditto about language and type attributes.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:14
> + layoutTestController.dumpAsText();

We should probably give two or four spaces instead of one to be consistent with
the rest of tests.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:19
> + moveSelectionForwardByLineCommand();

Ditto about the spaces.

> LayoutTests/editing/selection/move-by-line-cycles-in-table.html:21
> +document.write(getSelection().baseOffset == 4 ? "PASS" : "FAIL");

It'll be nice if you printed getSelection().baseOffset in the case of FAIL so
that it's easier to diagnose the problem if the test failed on bots.