RESOLVED FIXED 60200
Make more member functions in EventHandler private
https://bugs.webkit.org/show_bug.cgi?id=60200
Summary Make more member functions in EventHandler private
Ryosuke Niwa
Reported 2011-05-04 12:28:33 PDT
Minimize the number of public member functions in EventHandler.
Attachments
cleanup (10.12 KB, patch)
2011-05-04 12:41 PDT, Ryosuke Niwa
no flags
Fixed qt and win builds (9.21 KB, patch)
2011-05-04 13:48 PDT, Ryosuke Niwa
no flags
Ryosuke Niwa
Comment 1 2011-05-04 12:41:24 PDT
Created attachment 92302 [details] cleanup
Eric Seidel (no email)
Comment 2 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?
Early Warning System Bot
Comment 3 2011-05-04 12:50:40 PDT
Ryosuke Niwa
Comment 4 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.
Build Bot
Comment 5 2011-05-04 13:38:30 PDT
Ryosuke Niwa
Comment 6 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.
Ryosuke Niwa
Comment 7 2011-05-04 13:48:11 PDT
Created attachment 92313 [details] Fixed qt and win builds
WebKit Review Bot
Comment 8 2011-05-04 13:49:13 PDT
Eric Seidel (no email)
Comment 9 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.
Ryosuke Niwa
Comment 10 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.
WebKit Review Bot
Comment 11 2011-05-04 14:19:50 PDT
Eric Seidel (no email)
Comment 12 2011-05-04 14:34:16 PDT
Comment on attachment 92313 [details] Fixed qt and win builds OK.
Alexey Proskuryakov
Comment 13 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.
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2011-05-04 20:07:20 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.