[webkit-reviews] review denied: [Bug 71236] refactor Scrollable::m_scrollOrigin from protected to private : [Attachment 113095] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 71236: refactor Scrollable::m_scrollOrigin from protected to private
https://bugs.webkit.org/show_bug.cgi?id=71236

Attachment 113095: patch
https://bugs.webkit.org/attachment.cgi?id=113095&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=113095&action=review


> Source/WebCore/platform/ScrollView.cpp:591
>  
> +
>      // Make sure the scrollbar offsets are up to date.

Why add a second blank line here? Please don’t.

> Source/WebCore/platform/ScrollView.cpp:1264
> +    if (ScrollableArea::scrollOrigin() == origin)

Should just say scrollOrigin(), not ScrollableArea::scrollOrigin().

> Source/WebCore/platform/ScrollView.h:322
> -    IntPoint scrollOrigin() const { return m_scrollOrigin; }
> +    IntPoint scrollOrigin() const { return ScrollableArea::scrollOrigin(); }


Why is this needed at all? Can’t we just use the inherited function and not
override it? If the visibility needs to change, that can be done with "using".

> Source/WebCore/platform/ScrollableArea.h:94
> +    void setScrollOrigin(const IntPoint&);
> +    void setScrollOriginX(int);
> +    void setScrollOriginY(int);

It’s not good to have these as new public functions. It would be dangerous to
call any of these on, say, a ScrollView, bypassing the
ScrollView::setScrollOrigin function that prefers the actual scrolling.

So we need to keep these private in classes like ScrollView, somehow. It’s not
clear to me what the best way to do that is. Maybe make protected in this
class? That might not be sufficient.