[Webkit-unassigned] [Bug 52128] ISO-8859-8 Hebrew text displayed reversed with dir="rtl"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52128


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81727|review?                     |review-
               Flag|                            |




--- Comment #17 from mitz at webkit.org  2011-02-09 10:33:19 PST ---
(From update of attachment 81727)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.