[webkit-reviews] review granted: [Bug 114721] REGRESSION (r147588): Line breaks occur in the middle of Hebrew words at haaertz.co.il and other websites : [Attachment 198801] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Glenn Adams <glenn at skynav.com>'s
request for review:
Bug 114721: REGRESSION (r147588): Line breaks occur in the middle of Hebrew
words at haaertz.co.il and other websites
https://bugs.webkit.org/show_bug.cgi?id=114721

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=198801&action=review


> LayoutTests/fast/text/line-break-after-empty-inline-hebrew.html:18
> +    testRunner.dumpAsText();
> +function getLineWidths(paragraphNumber) {

Nit: Needs blank line

> LayoutTests/fast/text/line-break-after-empty-inline-hebrew.html:33
> +}
> +var widths1 = getLineWidths(0);

Here too.

> LayoutTests/fast/text/line-break-after-empty-inline-hebrew.html:38
> +if (widths1.length != widths2.length) {
> +    results += 'FAIL: different number of lines, got ' + widths2.length + ',
expected ' + widths1.length;
> +} else {

Single line block -> no braces.

> LayoutTests/fast/text/line-break-after-inline-latin1.html:10
> +<p>With span inline on first word, Latitn1 in first position of second
word.</p>

Typo Latin1

> LayoutTests/fast/text/line-break-after-inline-latin1.html:16
> +<p>Without span, Latitn1 in first position of second word.</p>

Typo Latin1

> LayoutTests/fast/text/line-break-after-inline-latin1.html:25
> +    testRunner.dumpAsText();
> +function mergeRect(rects, rect) {

Nit: needs blank line

> LayoutTests/fast/text/line-break-after-inline-latin1.html:29
> +    if (!rects.length) {
> +	   newRects.push(rect);
> +    } else {

Single line block -> no braces

> LayoutTests/fast/text/line-break-after-inline-latin1.html:33
> +	       if (!rect) {
> +		   newRects.push(r);

Ditto

> LayoutTests/fast/text/line-break-after-inline-latin1.html:41
> +		   if (rect.left + rect.width > r.left + r.width) {
> +		       r.width = rect.left + rect.width - r.left;
> +		   }

Ditto

> LayoutTests/fast/text/line-break-after-inline-latin1.html:46
> +	       } else {
> +		   newRects.push(r);
> +	       }

Ditto

> LayoutTests/fast/text/line-break-after-inline-latin1.html:50
> +	   if (rect) {
> +	       newRects.push(rect);
> +	   }

Ditto

> LayoutTests/fast/text/line-break-after-inline-latin1.html:57
> +    for (var i = 0; i < newRects.length; ++i) {
> +	   rects = mergeRect(rects, newRects[i]);
> +    }

Ditto

> LayoutTests/fast/text/line-break-after-inline-latin1.html:85
> +    if (rects1.length != rects2.length) {
> +	   results = appendResults(results, 'FAIL: different number of lines,
got ' + rects1.length + ', expected ' + rects2.length);
> +    } else {

Ditto

> LayoutTests/fast/text/line-break-after-inline-latin1.html:99
> +for (var i = 0; i < 3; i++ ) {
> +    results = compareParagraphLineRects(i*2 + 1, i*2 + 1 + 6, results);
> +}

Ditto