Bug 38854 – [chromium] Add WebKitScrollbar interface to allow Chromium code to reuse the scrollbar code.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[John Abd-El-Malek]

This allows us to reuse the scrollbar logic and theming from Pepper.

[John Abd-El-Malek]

Created attachment 55563 [details]
Proposed patch

[WebKit Review Bot]

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.

[John Abd-El-Malek]

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

[John Abd-El-Malek]

Created attachment 55566 [details]
Updated per check-webkit-style

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

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.

[John Abd-El-Malek]

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.

[John Abd-El-Malek]

Created attachment 55587 [details]
updated

[WebKit Review Bot]

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.

[John Abd-El-Malek]

Created attachment 55593 [details]
fix spacing

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

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

[John Abd-El-Malek]

(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

[John Abd-El-Malek]

Created attachment 55624 [details]
update

[WebKit Review Bot]

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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 55624 [details]
update

r=me w/ the setTotalSize -> setDocumentSize change that we discussed

[John Abd-El-Malek]

Committed r59110

[John Abd-El-Malek]

(In reply to comment #17)
> Committed r59110

Looks like the gyp changes didn't get checked in with this. I committed them in rr59156