[Webkit-unassigned] [Bug 23556] Right-to-left pages should be scrollable to reveal left overflow

[bugzilla-daemon at webkit.org]

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


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74338|review?                     |review-
               Flag|                            |




--- Comment #65 from Dave Hyatt <hyatt at apple.com>  2010-11-21 11:11:53 PST ---
(From update of attachment 74338)
View in context: https://bugs.webkit.org/attachment.cgi?id=74338&action=review

Looking pretty good now.  One more round should do it.  I just don't like the directionChanged stuff, but everything else looks fine.

> WebCore/page/FrameView.cpp:453
> +    if (size == contentsSize() && root->directionChanged())

directionChanged() doesn't seem necessary to me.  It seems like you can just check if scrollOriginX changes from zero to non-zero.

> WebCore/platform/ScrollView.cpp:1038
> +    // FIXME: need corresponding functionality from platformWidget.

Does this FIXME still apply?

> WebCore/rendering/RenderBox.cpp:316
> +            if (viewStyle->direction() != style()->direction())
> +                viewRenderer->setDirectionChanged();

As mentioned above, I don't think this directionChanged() boolean should be necessary.

> WebCore/rendering/RenderBox.cpp:772
> +    // its margins, and including its left layout overflow.
> +    int bx = tx - marginLeft() + view()->leftLayoutOverflow();

I don't like the addition to the comment.  You're still just covering the entire canvas when you include leftLayoutOverflow, so I don't think you needed to change the comment at all.

> WebCore/rendering/RenderView.h:166
> +    void setDirectionChanged() {m_directionChanged = true;}
> +    bool directionChanged() {return m_directionChanged;}
> +    void resetDirectionChanged() {m_directionChanged = false;}

Remove if you can.  I don't think you should need it.

> WebCore/rendering/RenderView.h:182
> +    int docWidth(int) const;

Include the parameter name here.  It isn't obvious what the parameter is, so the name should be there.

> WebCore/rendering/RenderView.h:243
> +    bool m_directionChanged;

Remove.

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