Bug 60200 – Make more member functions in EventHandler private

• Component: UI Events
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Minimize the number of public member functions in EventHandler.

[Ryosuke Niwa]

Created attachment 92302 [details]
cleanup

[Eric Seidel (no email)]

Comment on attachment 92302 [details]
cleanup

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

> Source/WebCore/page/EventHandler.cpp:777
> -                mainFrame->eventHandler()->setPanScrollInProgress(true);
> +                mainFrame->eventHandler()->m_panScrollInProgress = true;

This seems strictly worse.  Can't we just make that method private?

[Early Warning System Bot]

Attachment 92302 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8557626

[Ryosuke Niwa]

Comment on attachment 92302 [details]
cleanup

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

>> Source/WebCore/page/EventHandler.cpp:777
>> +                mainFrame->eventHandler()->m_panScrollInProgress = true;
> 
> This seems strictly worse.  Can't we just make that method private?

I could but I don't think having a setter/getter for a private variable that's only accessed within the class is useful since member functions can access the variable directly without using the setter/getter; i.e. it doesn't enforce or hide anything.

[Build Bot]

Attachment 92302 [details] did not build on win:
Build output: http://queues.webkit.org/results/8554631

[Ryosuke Niwa]

Comment on attachment 92302 [details]
cleanup

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

>>> Source/WebCore/page/EventHandler.cpp:777
>>> +                mainFrame->eventHandler()->m_panScrollInProgress = true;
>> 
>> This seems strictly worse.  Can't we just make that method private?
> 
> I could but I don't think having a setter/getter for a private variable that's only accessed within the class is useful since member functions can access the variable directly without using the setter/getter; i.e. it doesn't enforce or hide anything.

For example, EventHandler::startPanScrolling directly sets the value of m_panScrollInProgress without going through setPanScrollInProgress and EventHandler::stopAutoscrollTimer checks the value of m_panScrollInProgress directly.

[Ryosuke Niwa]

Created attachment 92313 [details]
Fixed qt and win builds

[WebKit Review Bot]

Attachment 92302 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8557644

[Eric Seidel (no email)]

I guess I long ago bought into the accessors-are-strictly-better-than-direct-access logic.  I like being able to set breakpoints in accessors (although that's not always possible in inline accessors).

I don't think this really matters here or there.  but when I find myself wanting to write foo->m_bar and i'm not in a copy constructor, I stop and write an accessor.

[Ryosuke Niwa]

(In reply to comment #9)
> I guess I long ago bought into the accessors-are-strictly-better-than-direct-access logic.  I like being able to set breakpoints in accessors (although that's not always possible in inline accessors).
> 
> I don't think this really matters here or there.  but when I find myself wanting to write foo->m_bar and i'm not in a copy constructor, I stop and write an accessor.

I think what this code is telling me is that m_panScrollInProgress is a page-level concept and not per-frame concept.  The situation should improve if we make EventHandler a page-level class and separate per-frame states as Sam suggested.

[WebKit Review Bot]

Attachment 92302 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8571152

[Eric Seidel (no email)]

Comment on attachment 92313 [details]
Fixed qt and win builds

OK.

[Alexey Proskuryakov]

We generally don't use accessor functions from within the class - there were even some patches landed solely to replace function calls with direct variable access.

I think it's nice to immediately see that there is no side effect from the given line of code.

[WebKit Commit Bot]

Comment on attachment 92313 [details]
Fixed qt and win builds

Clearing flags on attachment: 92313

Committed r85826: <http://trac.webkit.org/changeset/85826>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.