[webkit-reviews] review requested: [Bug 54623] RTL web content should have left-hand scrollbar. : [Attachment 122368] Patch v9 (renamed function names)

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  for review:
Bug 54623: RTL web content should have left-hand scrollbar.
https://bugs.webkit.org/show_bug.cgi?id=54623

Attachment 122368: Patch v9 (renamed function names)
https://bugs.webkit.org/attachment.cgi?id=122368&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
Greetings Niwa-san,

Many thanks for your comments.
I have renamed the functions added by this change. Also, I have disabled moving
scrollbars when -webkit-writing-mode is vertical-lr or vertical rl. (I notice
we also need to change Y positions to support them.) Should this change support
them as well, or should I use another change to support them?
By the way, this change ALWAYS moves horizontal scrollbars of RTL elements on
Chromium. That is, this change needs rebaselines for some layout tests that use
RTL elements: "fast/block/float/026.html", "fast/block/float/028.html",
"fast/events/offsetX-offsetY.html". (Unfortunately, the first two tests are
pixel ones and I cannot get rebaselined results for all platforms.)

Regards,

Hironori Bono

(In reply to comment #41)
> (From update of attachment 121427 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=121427&action=review
> 
> Great! New patch looks much cleaner.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1952
> > +LayoutUnit RenderLayer::rtlAwareVerticalScrollbarX(int minX, int maxX)
const
> 
> We normally use the word "start" to mean left for LTR and right for RTL so
how about verticalScrollbarStart?
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1962
> > +LayoutUnit RenderLayer::rtlAwareHorizontalScrollbarX(int minX) const
> 
> horizontalScrollbarStart?