Bug 38854 - [chromium] Add WebKitScrollbar interface to allow Chromium code to reuse the scrollbar code.
Summary: [chromium] Add WebKitScrollbar interface to allow Chromium code to reuse the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: John Abd-El-Malek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-10 09:56 PDT by John Abd-El-Malek
Modified: 2010-05-11 10:12 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (26.17 KB, patch)
2010-05-10 09:58 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
Updated per check-webkit-style (25.85 KB, patch)
2010-05-10 10:24 PDT, John Abd-El-Malek
fishd: review-
Details | Formatted Diff | Diff
updated (27.10 KB, patch)
2010-05-10 12:09 PDT, John Abd-El-Malek
no flags Details | Formatted Diff | Diff
fix spacing (27.10 KB, patch)
2010-05-10 12:51 PDT, John Abd-El-Malek
fishd: review-
Details | Formatted Diff | Diff
update (27.62 KB, patch)
2010-05-10 17:02 PDT, John Abd-El-Malek
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2010-05-10 09:56:46 PDT
This allows us to reuse the scrollbar logic and theming from Pepper.
Comment 1 John Abd-El-Malek 2010-05-10 09:58:15 PDT
Created attachment 55563 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-05-10 10:00:37 PDT
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.
Comment 3 John Abd-El-Malek 2010-05-10 10:21:54 PDT
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
Comment 4 John Abd-El-Malek 2010-05-10 10:24:29 PDT
Created attachment 55566 [details]
Updated per check-webkit-style
Comment 5 WebKit Review Bot 2010-05-10 10:28:00 PDT
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 6 Darin Fisher (:fishd, Google) 2010-05-10 10:40:49 PDT
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.
Comment 7 John Abd-El-Malek 2010-05-10 11:22:49 PDT
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.
Comment 8 John Abd-El-Malek 2010-05-10 12:09:05 PDT
Created attachment 55587 [details]
updated
Comment 9 WebKit Review Bot 2010-05-10 12:12:00 PDT
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.
Comment 10 John Abd-El-Malek 2010-05-10 12:51:17 PDT
Created attachment 55593 [details]
fix spacing
Comment 11 WebKit Review Bot 2010-05-10 12:56:46 PDT
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 12 Darin Fisher (:fishd, Google) 2010-05-10 16:20:10 PDT
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
Comment 13 John Abd-El-Malek 2010-05-10 16:56:21 PDT
(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
Comment 14 John Abd-El-Malek 2010-05-10 17:02:01 PDT
Created attachment 55624 [details]
update
Comment 15 WebKit Review Bot 2010-05-10 17:04:34 PDT
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 16 Darin Fisher (:fishd, Google) 2010-05-10 17:07:05 PDT
Comment on attachment 55624 [details]
update

r=me w/ the setTotalSize -> setDocumentSize change that we discussed
Comment 17 John Abd-El-Malek 2010-05-10 17:10:50 PDT
Committed r59110
Comment 18 John Abd-El-Malek 2010-05-11 10:12:20 PDT
(In reply to comment #17)
> Committed r59110

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