[webkit-reviews] review granted: [Bug 52128] ISO-8859-8 Hebrew text displayed reversed with dir="rtl" : [Attachment 82015] patch w/ layout test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted 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 82015: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=82015&action=review

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

> Source/WebCore/ChangeLog:11
> +	   (WebCore::::createBidiRunsForLine): For lines with visually
direction override, create bidi runs without resolving bidi levels (one run per
render object), set bidi level as 0 or 1 depends on LTR or RTL override, and
reverse runs for RTL override.

You may want to use shorter lines in the change log (~100 characters seems to
be the norm). Also change “visually” to “visual” and “depends” to “depending”.

> LayoutTests/fast/text/international/iso-8859-8.html:4
> +<meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-8"/>

The / before the > is not necessary. And in HTML5 this can be expressed simply
as <meta charset="ISO-8859-8">.

> LayoutTests/fast/text/international/iso-8859-8.html:13
> +<div contenteditable class="test">ùðá</div>
> +<div contenteditable class="test">ùðá â÷ë </div>
> +<div contenteditable class="test">ùðá abc â÷ë</div>
> +<div contenteditable class="test">abc ùðá def</div>
> +<div contenteditable class="test">ùðá <span dir=ltr>â÷ë</span></div>
> +<div contenteditable class="test">ùðá <span dir=rtl>â÷ë</span></div>

Do these really need to be contenteditable?

> LayoutTests/fast/text/international/iso-8859-8.html:24
> +    function log(str)
> +    {
> +	   var li = document.createElement("li");
> +	   li.appendChild(document.createTextNode(str));
> +	   var console = document.getElementById("console");
> +	   console.appendChild(li);
> +    }

Since you’re already including js-test-pre.js, why not use testPassed() and
testFailed()?