Bug 71236 - refactor Scrollable::m_scrollOrigin from protected to private
Summary: refactor Scrollable::m_scrollOrigin from protected to private
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 70395
  Show dependency treegraph
 
Reported: 2011-10-31 14:03 PDT by Xiaomei Ji
Modified: 2011-11-01 15:07 PDT (History)
2 users (show)

See Also:


Attachments
patch (14.92 KB, patch)
2011-10-31 15:46 PDT, Xiaomei Ji
darin: review-
Details | Formatted Diff | Diff
patch (14.43 KB, patch)
2011-11-01 11:02 PDT, Xiaomei Ji
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2011-10-31 14:03:47 PDT
change Scrollable::m_scrollOrigin from protected to private.
Comment 1 Xiaomei Ji 2011-10-31 15:46:18 PDT
Created attachment 113095 [details]
patch
Comment 2 Darin Adler 2011-10-31 15:56:13 PDT
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?
Comment 3 Darin Adler 2011-10-31 15:56:40 PDT
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.
Comment 4 Darin Adler 2011-10-31 16:01:05 PDT
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.
Comment 5 Xiaomei Ji 2011-11-01 11:01:26 PDT
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.
Comment 6 Xiaomei Ji 2011-11-01 11:02:26 PDT
Created attachment 113196 [details]
patch
Comment 7 Xiaomei Ji 2011-11-01 15:07:11 PDT
Committed r99002: <http://trac.webkit.org/changeset/99002>