Summary: | [chromium] Add WebKitScrollbar interface to allow Chromium code to reuse the scrollbar code. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Abd-El-Malek <jam> | ||||||||||||
Component: | WebKit API | Assignee: | John Abd-El-Malek <jam> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | fishd, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
John Abd-El-Malek
2010-05-10 09:56:46 PDT
Created attachment 55563 [details]
Proposed patch
Attachment 55563 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebScrollbarImpl.cpp:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebKit/chromium/src/WebScrollbarImpl.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:50: Use 'using namespace std;' instead of 'using std::max;'. [build/using_std] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:54: Place brace on its own line for function definitions. [whitespace/braces] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
WebKit/chromium/src/WebScrollbarImpl.cpp:88: old_rect is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:111: One line control clauses should not use braces. [whitespace/braces] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:154: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:154: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:167: One line control clauses should not use braces. [whitespace/braces] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:174: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:195: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 237 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
I've fixed all the style issues except the "Alphabetical sorting problem." one. That seems like a bug in check-webkit-style. I filed a bug https://bugs.webkit.org/show_bug.cgi?id=38855 Created attachment 55566 [details]
Updated per check-webkit-style
Attachment 55566 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebScrollbarImpl.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 55566 [details]
Updated per check-webkit-style
WebKit/chromium/public/WebScrollbar.h:54
+ WEBKIT_API static WebScrollbar* create(WebScrollbarClient*, bool vertical);
nit: please create an enum for the orientation. that way at the
call site, there won't just be a mysterious true or false.
class WebScrollbar {
public:
enum Orientation {
Vertical,
Horizontal
};
WebKit/chromium/public/WebScrollbarClient.h:44
+ virtual void getTickmarks(WebVector<WebRect>*) const = 0;
nit: add a WebScrollbar pointer as the first argument for consistency?
WebKit/chromium/public/WebScrollbar.h:74
+ virtual void scroll(bool forward, WebScrollGranularity, float multiplier) = 0;
replace "forward" with a WebScrollbar::ScrollDirection enum:
enum ScrollDirection {
ScrollUp,
ScrollDown,
...
WebKit/chromium/public/WebScrollbar.h:44
+ enum WebScrollGranularity {
move this into the WebScrollbar class as:
enum ScrollGranularity {
ScrollByLine,
...
WebKit/chromium/public/WebScrollbar.h:59
+ virtual int getThickness() const = 0;
nit: webkit style has attribute getters named without the "get" prefix,
so this should just be "thickness()"
however, perhaps thickness should really just be a static function
named defaultThickness(). you can implement that by using
ScrollbarTheme::nativeTheme()->scrollbarThickness().
WebKit/chromium/public/WebScrollbar.h:62
+ virtual int getPosition() const = 0;
nit: getPosition -> position
WebKit/chromium/public/WebScrollbar.h:68
+ virtual void setLocation(const WebRect&) = 0;
nit: i would group this with thickness since they are both about geometry.
WebKit/chromium/public/WebScrollbar.h:65
+ virtual void setPosition(int position) = 0;
position and location sound so similar. how about using the term
"value" instead of position? then you can have the following:
int value() const;
setValue(int);
int maxValue() const;
setMaxValue(int);
where setMaxValue replaces setTotalSize.
WebKit/chromium/src/WebScrollbarImpl.cpp:136
+ case WebInputEvent::MouseDown: {
nit: a helper function per case would be nice here.
implementation looks good otherwise.
Thanks, all done except: (In reply to comment #6) > (From update of attachment 55566 [details]) > WebKit/chromium/public/WebScrollbar.h:54 > + WEBKIT_API static WebScrollbar* create(WebScrollbarClient*, bool vertical); > nit: please create an enum for the orientation. that way at the > call site, there won't just be a mysterious true or false. > > class WebScrollbar { > public: > enum Orientation { > Vertical, > Horizontal > }; > > WebKit/chromium/public/WebScrollbarClient.h:44 > + virtual void getTickmarks(WebVector<WebRect>*) const = 0; > nit: add a WebScrollbar pointer as the first argument for consistency? > > WebKit/chromium/public/WebScrollbar.h:74 > + virtual void scroll(bool forward, WebScrollGranularity, float multiplier) = 0; > replace "forward" with a WebScrollbar::ScrollDirection enum: > > enum ScrollDirection { > ScrollUp, > ScrollDown, > ... > I made it have an enum for forward/backward. This makes the Chromium implementation easier since pepper_scrollbar_widget doesn't know if it's vertical/horizontal scrollbar after construction. > WebKit/chromium/public/WebScrollbar.h:44 > + enum WebScrollGranularity { > move this into the WebScrollbar class as: > > enum ScrollGranularity { > ScrollByLine, > ... > > WebKit/chromium/public/WebScrollbar.h:59 > + virtual int getThickness() const = 0; > nit: webkit style has attribute getters named without the "get" prefix, > so this should just be "thickness()" > > however, perhaps thickness should really just be a static function > named defaultThickness(). you can implement that by using > ScrollbarTheme::nativeTheme()->scrollbarThickness(). > > WebKit/chromium/public/WebScrollbar.h:62 > + virtual int getPosition() const = 0; > nit: getPosition -> position > > WebKit/chromium/public/WebScrollbar.h:68 > + virtual void setLocation(const WebRect&) = 0; > nit: i would group this with thickness since they are both about geometry. > > WebKit/chromium/public/WebScrollbar.h:65 > + virtual void setPosition(int position) = 0; > position and location sound so similar. how about using the term > "value" instead of position? then you can have the following: > > int value() const; > setValue(int); > > int maxValue() const; > setMaxValue(int); > > where setMaxValue replaces setTotalSize. maxValue is different from totalSize. i.e. total size is the length of the document, while maxValue is the highest integer that value() could be (which is totalSize - scrollbar_length). > > WebKit/chromium/src/WebScrollbarImpl.cpp:136 > + case WebInputEvent::MouseDown: { > nit: a helper function per case would be nice here. > > implementation looks good otherwise. Created attachment 55587 [details]
updated
Attachment 55587 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebScrollbarImpl.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/src/WebScrollbarImpl.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 55593 [details]
fix spacing
Attachment 55593 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebScrollbarImpl.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 55593 [details]
fix spacing
WebKit/chromium/ChangeLog:16
+ (WebKit::WebScrollbarImpl::getThickness):
nit: please re-create this ChangeLog given the API changes
WebKit/chromium/public/WebScrollbar.h:56
+ // These values must match WebCore::ScrollGranularity values
nit: indentation
WebKit/chromium/public/WebScrollbar.h:62
+ };
nit: indentation
WebKit/chromium/public/WebScrollbar.h:45
+ // These values must match WebCore::ScrollbarOrientation values
nit: this comment could become stale. if the WebCore enum changes,
we might end up writing a translation layer for the enums instead of
changing our API. in general, i just avoid mentioning WebCore
implementation details in the WebKit API headers.
WebKit/chromium/public/WebScrollbar.h:70
+ static int defaultThickness();
nit: add WEBKIT_API
WebKit/chromium/public/WebScrollbar.h:82
+ virtual void setTotalSize(int size) = 0;
it would be helpful to explain what this means. it is not so obvious
from reading these comments how the range of values and the total size
fit together. ascii art might help.
WebKit/chromium/public/WebScrollbarClient.h:42
+ virtual void positionChanged(WebScrollbar*) = 0;
nit: valueChanged
WebKit/chromium/src/AssertMatchingEnums.cpp:293
+ #endif
nit: add a set for orientation?
WebKit/chromium/src/WebScrollbarImpl.cpp:81
+ m_scrollbar->invalidate();
nit: indentation
WebKit/chromium/src/WebScrollbarImpl.cpp:153
+ WebMouseEvent mousedown = *static_cast<const WebMouseEvent*>(&event);
indentation
WebKit/chromium/src/WebScrollbarImpl.cpp:169
+ }
indentation
WebKit/chromium/src/WebScrollbarImpl.cpp:184
+ }
indentation
WebKit/chromium/src/WebScrollbarImpl.cpp:191
+ return m_scrollbar->mouseExited();
lots of indentation issues in this file
(In reply to comment #12) > (From update of attachment 55593 [details]) > WebKit/chromium/ChangeLog:16 > + (WebKit::WebScrollbarImpl::getThickness): > nit: please re-create this ChangeLog given the API changes > > WebKit/chromium/public/WebScrollbar.h:56 > + // These values must match WebCore::ScrollGranularity values > nit: indentation > > WebKit/chromium/public/WebScrollbar.h:62 > + }; > nit: indentation > > WebKit/chromium/public/WebScrollbar.h:45 > + // These values must match WebCore::ScrollbarOrientation values > nit: this comment could become stale. if the WebCore enum changes, > we might end up writing a translation layer for the enums instead of > changing our API. in general, i just avoid mentioning WebCore > implementation details in the WebKit API headers. ah I see, sure done. I was just copying other examples. > > WebKit/chromium/public/WebScrollbar.h:70 > + static int defaultThickness(); > nit: add WEBKIT_API > > WebKit/chromium/public/WebScrollbar.h:82 > + virtual void setTotalSize(int size) = 0; > it would be helpful to explain what this means. it is not so obvious > from reading these comments how the range of values and the total size > fit together. ascii art might help. added an example and expanded the comment > > WebKit/chromium/public/WebScrollbarClient.h:42 > + virtual void positionChanged(WebScrollbar*) = 0; > nit: valueChanged > > WebKit/chromium/src/AssertMatchingEnums.cpp:293 > + #endif > nit: add a set for orientation? I already did? I'm not sure we need to do that. The WebCore object API only sets it on construction. > > WebKit/chromium/src/WebScrollbarImpl.cpp:81 > + m_scrollbar->invalidate(); > nit: indentation > > WebKit/chromium/src/WebScrollbarImpl.cpp:153 > + WebMouseEvent mousedown = *static_cast<const WebMouseEvent*>(&event); > indentation > > WebKit/chromium/src/WebScrollbarImpl.cpp:169 > + } > indentation > > WebKit/chromium/src/WebScrollbarImpl.cpp:184 > + } > indentation > > WebKit/chromium/src/WebScrollbarImpl.cpp:191 > + return m_scrollbar->mouseExited(); > lots of indentation issues in this file Created attachment 55624 [details]
update
Attachment 55624 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebScrollbarImpl.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 55624 [details]
update
r=me w/ the setTotalSize -> setDocumentSize change that we discussed
Committed r59110 (In reply to comment #17) > Committed r59110 Looks like the gyp changes didn't get checked in with this. I committed them in rr59156 |