[Webkit-unassigned] [Bug 71236] refactor Scrollable::m_scrollOrigin from protected to private

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=71236


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113095|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2011-10-31 16:01:05 PST ---
(From update of attachment 113095)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.