[webkit-reviews] review denied: [Bug 23556] On RTL pages, horizontal scrollbar is missing : [Attachment 62608] patch w/ layout test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 23556: On RTL pages, horizontal scrollbar is missing
https://bugs.webkit.org/show_bug.cgi?id=23556

Attachment 62608: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=62608&action=review

------- Additional Comments from mitz at webkit.org
Thanks for tackling this bug! I think the ScrollView-based approach is a good
one.

> Index: WebCore/ChangeLog

> +	   Add frame horizontal scroll bar for RTL page.

This is not a very good description of the change. This bug also doesn’t have a
very good title, so maybe we can fix both.

> +	   https://bugs.webkit.org/show_bug.cgi?id=23556
> +
> +	   For RTL page, save left layout overflow and include it into the
document	size during layout. Use the left layout overflow when scroll
and paint

Extra space (or missing newline) up there.

> +	   the page. Behavior on LTR page should be untouched since left layout

> +	   overflow is set as 0 for LTR page.
> +
> +	   Test: fast/dom/horizontal_scrollbar_in_rtl.html

We typically use dashes, not underscores, in test names. I think this could use
more than one test. For example, test a RTL page with right overflow.

> +    // Do not move setScrollOriginX() to other places, otherwise,
> +    // content painted and scroll might not work correctly when page resize.


I don’t understand this comment. Is it warning me not to move the code
somewhere else? I don’t think that’s necessary.

> +    ScrollView::setScrollOriginX(abs(root->leftLayoutOverflow()));

No need to use abs() here. leftLayoutOverflow() is always non-positive, so you
can just use -root->leftLayoutOverflow().

> +    // Adjust scroll position to above the minimum value. y-axis should not
be
> +    // negative. The minimum value for x-axis is the left layout overflow,
> +    // which should be zero for non RTL page and the negative of
m_scrollOriginX
> +    // for RTL page.

“non RTL” is “LTR” :-)

At the platform level, I think you don’t need to refer to the page, but just
say that m_scrollOriginX sets the minimum.

Perhaps for symmetry you can define a minimumScrollPosition() method.

> -    IntSize maxScrollPosition(contentsWidth() - visibleWidth(),
contentsHeight() - visibleHeight());
> +    IntSize maxScrollPosition(contentsWidth() - visibleWidth() -
m_scrollOriginX, contentsHeight() - visibleHeight());
> +    maxScrollPosition.clampNegativeToZero();

Can this just use the maximumScrollPosition() method?

>      IntSize scroll = desiredOffset.shrunkTo(maxScrollPosition);
> -    scroll.clampNegativeToZero();
> - 
> +    // Adjust scroll position to above the minimum value. y-axis should not
be
> +    // negative. The minimum value for x-axis is the left layout overflow,
> +    // which should be zero for non RTL page and the negative of
m_scrollOriginX
> +    // for RTL page.
> +    scroll = scroll.expandedTo(IntSize(-m_scrollOriginX, 0));
> +

Same comments about the comment and same suggestion to define a
minimumScrollPosition() method.

> +void ScrollView::setScrollOriginX(int x)
> +{ 
> +    m_scrollOriginX = x;
> +#if PLATFORM(MAC)
> +    platformSetScrollOriginX();
> +#endif
> +}

This should check for platformWidget(), not do a compile-time PLATFORM check.
Do you even need to set m_scrollOriginX if there is a platform widget, or can
you just change platformSetScrollOriginX() to take a parameter and pass x to
it?

> +    void setScrollOriginX(int x);

Please omit the “x” here.

> +    int m_scrollOriginX; // Only non-zero for RTL page.

This comment is not very helpful. It might help if it explained what the value
means and what it does, but I think it’s not really necessary to comment on
member variables.

> +    void platformSetScrollOriginX();

I suggest that this method take a parameter (see above).

> +    // leftmostPosition() is not always 0 for LTR page, for example
http://www.apple.com/startpage/.

This is not a helpful comment, and the example page will change in the future.

This clips out left overflow in LTR pages, but where is the part that clips out
right overflow in RTL pages?

> +    int leftOverflow = m_bodyIsRTL ? std::min(0, leftmostPosition()) : 0;
> +    addLayoutOverflow(IntRect(leftOverflow, 0, docWidth(leftOverflow),
docHeight()));

…

> -int RenderView::docWidth() const
> +int RenderView::docWidth(int leftOverflow) const
>  {
> -    int w = rightmostPosition();
> +    int w = rightmostPosition() - leftOverflow;

I would like to suggest something a bit different: add a docLeft() method that
returns 0 for LTR pages and leftmostPosition() for RTL pages; then change the
docWidth() computation of w to return rightmostPosition() for LTR pages and
width() - leftmostPosition() for RTL pages. I think this will also clip out
right overflow for those pages.

> +    void setBodyIsRTL(bool bodyIsRTL) { m_bodyIsRTL = bodyIsRTL; }

In recent years, I have tried to make all booleans be about LTR, not RTL, so
this would be …bodyIsLTR (with the reverse meaning). Another option is to have
a setPageDirection() method that takes a TextDirection type argument.
> +	   In MAC port, set and get the original x-axis scroll position.

It’s Mac, not MAC (also in other places in this patch).

Really good! I can’t quite understand what’s going on on Mac without applying
the patch and trying it out, which I am going to do later.