[Webkit-unassigned] [Bug 54623] RTL web content should have left-hand scrollbar.

[bugzilla-daemon at webkit.org]

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





--- Comment #25 from Jeremy Moskovich <playmobil at google.com>  2011-09-15 01:52:22 PST ---
(From update of attachment 107460)
View in context: https://bugs.webkit.org/attachment.cgi?id=107460&action=review

informal review since I'm not a WebKit reviewer.

> Source/WebCore/ChangeLog:6
> +        This change adds a new flag ENABLE_OVERFLOW_MIRRORING and render

nit: I think ENABLE_OVERFLOW_MIRRORING is a little hard to understand.  How about ENABLE_RTL_SCROLLBARS ?

> Source/WebCore/rendering/RenderBlock.cpp:1468
> +        if (positionedObject->style()->position() != FixedPosition) {

nit: I think rewriting this as:
if (positionedObject->style()->position() == FixedPosition)
   continue;
#ifdef ...

Would be a little cleaner since you wouldn' t need to repeat the if inside the #ifdef.

> Source/WebCore/rendering/RenderBox.cpp:1194
> +#else

Can you remove the #else and reuse the same call to contract() ? This may make things easier to follow...

> Source/WebCore/rendering/style/RenderStyle.h:529
> +    bool isRightToLeftDirection() const { return direction() == RTL; }

I may be wrong but I seem to recall Mitz mentioning that he wanted everything phrased as being related to isLTR so you may want to remove this and just use a locale isRTL = !bla->isLeftToRightDirection() where you need htat.

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