Minimize the number of public member functions in EventHandler.
Created attachment 92302 [details] cleanup
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?
Attachment 92302 [details] did not build on qt: Build output: http://queues.webkit.org/results/8557626
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.
Attachment 92302 [details] did not build on win: Build output: http://queues.webkit.org/results/8554631
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.
Created attachment 92313 [details] Fixed qt and win builds
Attachment 92302 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8557644
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.
(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.
Attachment 92302 [details] did not build on mac: Build output: http://queues.webkit.org/results/8571152
Comment on attachment 92313 [details] Fixed qt and win builds OK.
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 on attachment 92313 [details] Fixed qt and win builds Clearing flags on attachment: 92313 Committed r85826: <http://trac.webkit.org/changeset/85826>
All reviewed patches have been landed. Closing bug.