WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Attachment 92302
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8557626
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
Attachment 92302
[details]
did not build on win: Build output:
http://queues.webkit.org/results/8554631
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
Attachment 92302
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/8557644
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
Attachment 92302
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8571152
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.
Top of Page
Format For Printing
XML
Clone This Bug