[webkit-reviews] review canceled: [Bug 23556] On RTL pages, horizontal scrollbar is missing : [Attachment 53150] A proposed fix 5

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has canceled 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 53150: A proposed fix 5
https://bugs.webkit.org/attachment.cgi?id=53150&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
Jeremy,

Thank you for your comments.
I have updated my patch to replaced null-checks with ASSERT() expressions and
to use temporary variables.

Regards,

Hironori Bono

(In reply to comment #28)
> (From update of attachment 53150 [details])
> 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.