[webkit-reviews] review denied: [Bug 52128] ISO-8859-8 Hebrew text displayed reversed with dir="rtl" : [Attachment 78755] patch 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 52128: ISO-8859-8 Hebrew text displayed reversed with dir="rtl"
https://bugs.webkit.org/show_bug.cgi?id=52128

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=78755&action=review

> Source/WebCore/ChangeLog:15
> +	   Fix rendering of ISO-8859-8 when dir="rtl".
> +	   https://bugs.webkit.org/show_bug.cgi?id=52128
> +
> +	   Test: fast/dom/iso-8859-8.html
> +
> +	   * platform/text/BidiResolver.h:
> +	   (WebCore::::createBidiRunsForLine):
> +	   * rendering/RenderBlockLineLayout.cpp:
> +	   (WebCore::RenderBlock::constructLine):
> +	   (WebCore::RenderBlock::layoutInlineChildren):
> +

This should explain what is changing and why. I can’t even review the patch
without knowing what it’s trying to do.

> Source/WebCore/platform/text/BidiResolver.h:965
> +    } else if (direction == RTL) {
> +	   reverseRuns(0, runCount() - 1);
>      }

There shouldn’t be braces around a single-line block.

> LayoutTests/ChangeLog:9
> +	   * fast/dom/iso-8859-8.html: Added.

This is not a DOM change. Please don’t put the test in fast/dom.

> LayoutTests/fast/dom/iso-8859-8.html:55
> +    function checkRenderingResult(sel, node, offset, direction, element,
index) {
> +	   for (var i = 0; i < element.innerText.length; ++i) {
> +	       sel.setBaseAndExtent(node, offset, node, offset);
> +	       if (direction == "ltr") {
> +		   sel.modify("move", "right", "character");
> +		   sel.modify("extend", "left", "character");
> +	       } else {
> +		   sel.modify("move", "left", "character");
> +		   sel.modify("extend", "right", "character");
> +	       }
> +	       if (direction == "ltr") {
> +		   assertEqual("Test " + index + " " + direction + " the " + i
+ "th character from left", sel.toString(), element.innerText[i]);
> +	       } else {
> +		   assertEqual("Test " + index + " " + direction + " the " + i
+ "th character from right", sel.toString(), element.innerText[i]);
> +	       }
> +	       node = sel.baseNode;
> +	       offset = sel.baseOffset;
> +	   }
> +    }

Using the non-trivial logic for modifying selection is a very indirect way to
test this change. The best test for this would be a render tree dump. Those
include the directionality (and override state) of individual text runs, which
should suffice. If you can’t do that, then you can test directly for the visual
position of characters using Range.getClientRects().