[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





--- Comment #5 from Xiaomei Ji <xji at chromium.org>  2011-11-01 11:01:26 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.

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.

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