Summary: | Make more member functions in EventHandler private | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | UI Events | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | ap, buildbot, commit-queue, darin, dglazkov, eric, sam, webkit-ews, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 56410 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-05-04 12:28:33 PDT
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. |