Bug 60200 - Make more member functions in EventHandler private
Summary: Make more member functions in EventHandler private
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 56410
  Show dependency treegraph
 
Reported: 2011-05-04 12:28 PDT by Ryosuke Niwa
Modified: 2011-05-04 20:07 PDT (History)
9 users (show)

See Also:


Attachments
cleanup (10.12 KB, patch)
2011-05-04 12:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed qt and win builds (9.21 KB, patch)
2011-05-04 13:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-05-04 12:28:33 PDT
Minimize the number of public member functions in EventHandler.
Comment 1 Ryosuke Niwa 2011-05-04 12:41:24 PDT
Created attachment 92302 [details]
cleanup
Comment 2 Eric Seidel (no email) 2011-05-04 12:42:14 PDT
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?
Comment 3 Early Warning System Bot 2011-05-04 12:50:40 PDT
Attachment 92302 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8557626
Comment 4 Ryosuke Niwa 2011-05-04 13:35:34 PDT
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.
Comment 5 Build Bot 2011-05-04 13:38:30 PDT
Attachment 92302 [details] did not build on win:
Build output: http://queues.webkit.org/results/8554631
Comment 6 Ryosuke Niwa 2011-05-04 13:41:15 PDT
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.
Comment 7 Ryosuke Niwa 2011-05-04 13:48:11 PDT
Created attachment 92313 [details]
Fixed qt and win builds
Comment 8 WebKit Review Bot 2011-05-04 13:49:13 PDT
Attachment 92302 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8557644
Comment 9 Eric Seidel (no email) 2011-05-04 14:02:42 PDT
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.
Comment 10 Ryosuke Niwa 2011-05-04 14:07:28 PDT
(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.
Comment 11 WebKit Review Bot 2011-05-04 14:19:50 PDT
Attachment 92302 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8571152
Comment 12 Eric Seidel (no email) 2011-05-04 14:34:16 PDT
Comment on attachment 92313 [details]
Fixed qt and win builds

OK.
Comment 13 Alexey Proskuryakov 2011-05-04 14:45:33 PDT
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.
Comment 14 WebKit Commit Bot 2011-05-04 20:07:14 PDT
Comment on attachment 92313 [details]
Fixed qt and win builds

Clearing flags on attachment: 92313

Committed r85826: <http://trac.webkit.org/changeset/85826>
Comment 15 WebKit Commit Bot 2011-05-04 20:07:20 PDT
All reviewed patches have been landed.  Closing bug.