[webkit-reviews] review denied: [Bug 23556] On RTL pages, horizontal scrollbar is missing : [Attachment 53324] A proposed fix 6

Sat May 22 23:38:59 PDT 2010

mitz at webkit.org has denied Hironori Bono <hbono 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 53324: A proposed fix 6
https://bugs.webkit.org/attachment.cgi?id=53324&action=review

------- Additional Comments from mitz at webkit.org
> +    if (isBody()) {
>	   document()->setTextColor(style()->color());
> +	   ASSERT(view() && view()->style());

1. In principle, rather than asserting a conjunction, you should assert each
condition separately, so that if the assertion fails, you know which condition
was false.
2. In this case, and in similar cases in this patch, I find that it is
pointless to assert that a pointer is non-null if you are immediately going to
dereference it.

> +	   view()->style()->setDirection(style()->direction());

It’s wrong to mutate the RenderView’s style like this. In general, it is almost
never a good idea to change a RenderStyle anywhere other than in the style
selector. This holds true in this case as well. After this code runs, if the
<html> element’s style is recomputed, it may inherit the wrong direction from
the root, e.g.

<html>
    <body style="direction: rtl;">
	<script>
	    document.body.offsetTop; // force style recalc and layout
	    document.documentElement.style.color = "red";
	    document.body.offsetTop;
	    alert(getComputedStyle(document.documentElement).direction);
	</script>
    </body>
</html>

would alert "rtl" instead of "ltr".

It is also not clear to me that this behavior should be limited to when the
<body> element has direction: rtl. What about the case where the <html> element
has it but <body> is ltr?

> +    if (style()->direction() == RTL) {
> +	   int left = leftmostPosition();
> +	   if (left < 0) {
> +	       setHasOverflowClip(true);

I don’t understand this. What side effect of hasOverflowClip() are you trying
to achieve? It seems really dangerous to set hasOverflowClip() for the root,
and very strange to touch this flag during layout.