[webkit-reviews] review canceled: [Bug 70395] REGRESSION: rtl horizontal scrollbar / resize bug - Body shifts on resize when scrolled all the way to the left : [Attachment 113261] patch w/ layout test

[bugzilla-daemon at webkit.org]

Xiaomei Ji <xji at chromium.org> has canceled Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 70395: REGRESSION: rtl horizontal scrollbar / resize bug - Body shifts on
resize when scrolled all the way to the left
https://bugs.webkit.org/show_bug.cgi?id=70395

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

------- Additional Comments from Xiaomei Ji <xji at chromium.org>
updated per comment #18. Thanks for the quick drive-by review.



(In reply to comment #18)
> (From update of attachment 113261 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=113261&action=review
> 
> Just a drive by review.  I don't really understand how the various Scroll*
classes fit together.
> 
> > Source/WebCore/platform/ScrollAnimator.cpp:82
> >	     m_currentPosX = offset.x();
> >	     m_currentPosY = offset.y();
> >	     notifyPositionChanged();
> 
> Nit: unindent

done.

> 
> > Source/WebCore/platform/ScrollableArea.cpp:73
> > +	 } else
> > +	     m_scrollOriginChanged = false;
> 
> Is this else necessary?  If so, why don't we set m_scrollOriginChanged =
false in setScrollOrigin(X|Y)?

That is a good question.
Look at it again, I think the same as m_scrollOriginChanged is only accessed in
ScrollView::updateScrollbars(), its value should be only reset to false in
ScrollView::updateScrollbars(), so that ScrollView::updateScrollbars() is able
to catch the once changed m_scrollOrigin and set
ScrollAnimator::m_currentPosX|Y correctly.

As to the reason why not set m_scrollOriginChanged = false in
setScrollOrigin(X|Y) is that: if you setScrollOriginX with a different
scrollOriginX value, but then setScrollOriginY with the same scrollOriginY
value, resetting m_scrollOriginChanged = false in setScrollOriginY will
override the m_scrollOriginChanged = true value set in setScrollOriginX.