[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





--- Comment #18 from Xiaomei Ji <xji at chromium.org>  2011-02-09 18:01:35 PST ---
Thanks for the quick review!
Please see my comments inline.


(In reply to comment #17)
> (From update of attachment 81727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81727&action=review
> 
> > 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.


LeftToRightOverride and RightToLeftOverride are defined in WTF::Unicode.
If I reuse the names here, I need to prepend every usage of the existing names with WTF::Unicode, which I think is not neat and not consistent. That is why "Visually" is prepended here.

> 
> > 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?

You are right.
It should be:
        eor = Iterator();
        while(current != end && !current.atEnd()) {
            eor = current;
            increment();
        }


> 
> > Source/WebCore/platform/text/BidiResolver.h:535
> > +            eor = current;
> 
> Is it necessary to change eor inside the loop? Does increment() depend on it?

increment() does not depend or update it.
But eor need to be assigned inside the loop to be the inclusive end of the line.
On exit of the loop, current will be the exclusive end of the line (similar as string.length() vs. string.length() -1).

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