This allows us to reuse the scrollbar logic and theming from Pepper.
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