[webkit-reviews] review canceled: [Bug 85856] [Chromium] Cannot show the left side of an RTL element with a vertical scrollbar : [Attachment 158240] Patch v12 (fixed regressions, but dirty)

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 85856: [Chromium] Cannot show the left side of an RTL element with a
vertical scrollbar
https://bugs.webkit.org/show_bug.cgi?id=85856

Attachment 158240: Patch v12 (fixed regressions, but dirty)
https://bugs.webkit.org/attachment.cgi?id=158240&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158240&action=review


> Source/WebCore/ChangeLog:12
> +	   layout offsets.

'right' is used 5 times and the way it's written is confusing: I had a hard
time saying if you are talking about the direction and just make them 'right'.

>> Source/WebCore/ChangeLog:27
>> +	    (WebCore::RenderBlock::paintChildren): Move the paint rectangles of
RTL children right.
> 
> It feels wrong that we're moving the paint rectangles.  Why is it better to
move the paint offset rather than changing the RenderObject's offset?

I agree with Tony here: we should probably patch the offsets at layout time and
store them updated instead of requiring this code. Your approach only invites
bugs in our code as we can easily forget a code path. I couldn't find any in
the repaint code, but hit-testing looks suspicious as only RenderBlock was
updated (but neither RenderReplaced nor RenderLayer was).

> Source/WebCore/dom/Element.cpp:421
> +	   if (renderer->style() &&
renderer->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft())

Can we really have renderer->style() be NULL? Do you know when this occurs?

> Source/WebCore/rendering/RenderBlock.cpp:1681
> +	       addOverflowFromChild(positionedObject,
IntSize(positionedObject->x(), positionedObject->y()));

This should be LayoutSize.

> Source/WebCore/rendering/RenderBox.cpp:1576
> +    if (o->style() &&
o->style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() && o->isBox())

Again why do you NULL check style() here when none of the surrounding code does
this check?

Also it should be |styleToUse|, we cache style() (IIRC it is because region may
have made this call more expensive).