[webkit-reviews] review granted: [Bug 54623] RTL web content should have left-hand scrollbar. : [Attachment 125303] Patch v10 (Applied comments)

[bugzilla-daemon at webkit.org]

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

Attachment 125303: Patch v10 (Applied comments)
https://bugs.webkit.org/attachment.cgi?id=125303&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125303&action=review


This code looks right to me.  I think you could have simplified the #defines by
moving them as noted below, but I also think this i OK.  Thank you for hte
patch.

> Source/WebCore/rendering/RenderLayer.cpp:1831
> +    return IntRect(cornerStart(layer, bounds.x(), bounds.maxX(),
horizontalThickness),

I'm not sure what you mean by "corner start" here?

> Source/WebCore/rendering/style/RenderStyle.h:966
> +    bool shouldPlaceBlockDirectionScrollbarOnLogicalLeft() const { return
!isLeftToRightDirection() && isHorizontalWritingMode(); }

It seems it would have been simpler to put the #if around this function, and
just define a version of thsi function which only returns false when
RTL_SCROLLBAR is disabled.