[Webkit-unassigned] [Bug 23556] On RTL pages, horizontal scrollbar is missing

[bugzilla-daemon at webkit.org]

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





--- Comment #28 from Jeremy Moskovich <playmobil at google.com>  2010-04-12 14:08:18 PST ---
(From update of attachment 53150)
Obligatory disclaimer: I'm not a reviewer, but here are my comments.

> Index: WebCore/rendering/RenderBox.cpp
> ===================================================================
> --- WebCore/rendering/RenderBox.cpp	(revision 57464)
> +++ WebCore/rendering/RenderBox.cpp	(working copy)
> @@ -182,9 +182,12 @@ void RenderBox::styleDidChange(StyleDiff
> +    if (isBody()) {
>          document()->setTextColor(style()->color());
> +        if (view() && view()->style())

Can view() or view()->style() ever be NULL? Same goes for other places in the
patch.

>  void RenderBox::updateBoxModelInfoFromStyle()
> @@ -608,7 +611,11 @@ void RenderBox::paintRootBoxDecorations(
> +    // The bounding rectangle of an RTL view is (min(leftmostPosition(), 0), 0, docWidth(), docHeight())
> +    // So, we also include the leftmost position for RTL views so we can fill its entire canvas.
Great comment.

> +    if (view() && view()->style()->direction() == RTL && view()->leftmostPosition() < 0)
Again, are you sure you need to null-check view() ?

> @@ -117,6 +117,14 @@ void RenderView::layout()
> +    if (style() && style()->direction() == RTL && leftmostPosition() < 0) {
Using a temp. variable may ease readability here a bit since you use the same
expression in the return statement.

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