Bug 71236 – refactor Scrollable::m_scrollOrigin from protected to private

• Component: DOM
• Status: RESOLVED FIXED

[Xiaomei Ji]

change Scrollable::m_scrollOrigin from protected to private.

[Xiaomei Ji]

Created attachment 113095 [details]
patch

[Darin Adler]

Comment on attachment 113095 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113095&action=review

> Source/WebCore/platform/ScrollableArea.h:91
> +    int scrollOriginX() const { return m_scrollOrigin.x(); }
> +    int scrollOriginY() const { return m_scrollOrigin.y(); }

This seems unnecessary. Why not write scrollOrigin().x() at the call site?

[Darin Adler]

Comment on attachment 113095 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113095&action=review

> Source/WebCore/platform/ScrollableArea.cpp:69
> +    if (m_scrollOrigin != origin)
> +        m_scrollOrigin = origin;

The if statement adds no value.

> Source/WebCore/platform/ScrollableArea.cpp:75
> +    if (m_scrollOrigin.x() != x)
> +        m_scrollOrigin.setX(x);

The if statement adds no value.

> Source/WebCore/platform/ScrollableArea.cpp:81
> +    if (m_scrollOrigin.y() != y)
> +        m_scrollOrigin.setY(y);

The if statement adds no value.

[Darin Adler]

Comment on attachment 113095 [details]
patch

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.

[Xiaomei Ji]

Comment on attachment 113095 [details]
patch

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.

Thanks for the quick review!

removed the blank line.

>> Source/WebCore/platform/ScrollView.cpp:1264
>> +    if (ScrollableArea::scrollOrigin() == origin)
> 
> Should just say scrollOrigin(), not ScrollableArea::scrollOrigin().

changed.

>> Source/WebCore/platform/ScrollView.h:322
>> +    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".

Yes, you are right. it is not needed. I changed the caller side in FrameView.cpp

>> Source/WebCore/platform/ScrollableArea.cpp:69
>> +        m_scrollOrigin = origin;
> 
> The if statement adds no value.

removed and moved the assignment into inline in .h

>> Source/WebCore/platform/ScrollableArea.cpp:75
>> +        m_scrollOrigin.setX(x);
> 
> The if statement adds no value.

removed.

>> Source/WebCore/platform/ScrollableArea.cpp:81
>> +        m_scrollOrigin.setY(y);
> 
> The if statement adds no value.

removed.

>> Source/WebCore/platform/ScrollableArea.h:91
>> +    int scrollOriginY() const { return m_scrollOrigin.y(); }
> 
> This seems unnecessary. Why not write scrollOrigin().x() at the call site?

removed.

>> Source/WebCore/platform/ScrollableArea.h:94
>> +    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.

Yes, you are right that it should not be public.  I changed m_scrollOrigin from protected to private in ScrollableArea. but the value is set in ScrollView and RenderLayer. I changed those setters as protected for now.

[Xiaomei Ji]

Created attachment 113196 [details]
patch

[Xiaomei Ji]

Committed r99002: <http://trac.webkit.org/changeset/99002>