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

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

Very nice, but needs some improvement.

> 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 run s for RTL override.

typo: “run s”

> Source/WebCore/platform/text/BidiResolver.h:133
> +enum VisuallyDirectionOverride {
> +    NoVisuallyOverride,
> +    VisuallyLeftToRightOverride,
> +    VisuallyRightToLeftOverride
> +};

I don’t think the adverb “visually” is the right part of speech here. You could
use “visual” but really “DirectionOverride” is best.

> Source/WebCore/platform/text/BidiResolver.h:531
> +	   while (true) {

What if current == end or current.atEnd() when you enter this loop? Won’t you
miss it? I.e. is there a reason why this isn’t a while(current != end &&
!current.atEnd()) loop?

> Source/WebCore/platform/text/BidiResolver.h:535
> +	       eor = current;

Is it necessary to change eor inside the loop? Does increment() depend on it?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:680
> +		   enum VisuallyDirectionOverride override =
(style()->visuallyOrdered() ? (style()->direction() == LTR ?
VisuallyLeftToRightOverride : VisuallyRightToLeftOverride) :
NoVisuallyOverride);

You shouldn’t need “enum” here.