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

Fri Nov 25 16:04:06 PST 2011

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

--- Comment #34 from Ryosuke Niwa <rniwa at webkit.org>  2011-11-25 16:04:05 PST ---
(From update of attachment 114018)
View in context: https://bugs.webkit.org/attachment.cgi?id=114018&action=review

Nits.

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> Source/WebCore/rendering/RenderLayer.cpp:2147
> +                                        absBounds.y() + box->borderTop(),
> +                                        m_vBar->width(),
> +                                        absBounds.height() - (box->borderTop() + box->borderBottom()) - scrollCorner.height()));

Wrong indentation. "absBounds.y() + box->borderTop()" should be exactly 4 spaces to the right of "m_vBar->".

> Source/WebCore/rendering/RenderLayer.cpp:2157
> +                                        m_hBar->height()));

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:2572
> +                            box->borderTop(),
> +                            m_vBar->width(),
> +                            box->height() - (box->borderTop() + box->borderBottom()) - (m_hBar ? m_hBar->height() : resizeControlSize));

Ditto.

> Source/WebCore/rendering/RenderLayer.cpp:2594
> +                            box->height() - box->borderBottom() - m_hBar->height(),
> +                            box->width() - (box->borderLeft() + box->borderRight()) - (m_vBar ? m_vBar->width() : resizeControlSize),
> +                            m_hBar->height());

Ditto.

> LayoutTests/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description.

> LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:1
> +<html>

No DOCTYPE?

> LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:16
> +// Save the vertical-scroll offset of the above <div> element before sending a
> +// click event. If we successfully scroll down the element, this offset should
> +// become greather than this value.

I don't think we need this comment.

> LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:24
> +    eventSender.mouseMoveTo(10, 300);

You can do mouseMoveTo(overflow.offsetLeft + 5, overflow.offsetTop + overflow.offsetHeight - 50); instead of putting div at the right location.

> LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:29
> +    // Wait until we finish rendering the element.
> +    setTimeout(finished, 1000);

One second seems like an excess amount of time to wait. Probably setTimeout(finished, 0) is sufficient.
Also, I don't this comment is useful because the code is quite self-evident.

> LayoutTests/platform/chromium/fast/events/rtl-scrollbar.html:34
> +    // Verify the vertical-scroll offset becomes greater than the saved one.

The code is self-evident.

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