Adding a Views scrollbar implementation.

Adding a Views scrollbar implementation.

BUG=none
TEST=Verified basic functionality using views_examples.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98434

[dnicoara, 2011-08-17 19:52:22]

Hi Ben,

This is the Views implementation of the Scrollbar. Could you take a look
whenever you have a bit of time?

Thanks,
Daniel

[Ben Goodger (Google), 2011-08-19 17:44:33]

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
File views/controls/scrollbar/native_scroll_bar_views.cc (right):

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.cc:22: class ScrollBarButton :
public CustomButton {
Rather than reinvent the wheel here I'd rather you look at how to get all this
functionality out of our existing BitmapScrollBar class, which hand-rolls a
scrollbar with custom bitmaps. You will probably have to enhance it to support
native theme rendering.

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
File views/controls/scrollbar/native_scroll_bar_views.h (right):

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:57: void MoveBy(int o);
What is the unit. Pixels or something else?

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:60: void MovePage(bool
positive);
I would prefer if this expressed direction up or down.

MoveByPage.

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:63: void MoveStep(bool
positive);
MoveByStep.

http://codereview.chromium.org/7669028/diff/3001/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:69: void MoveToBottom();
Prefer start/end terminology to top/bottom left/right, since scrollbars can have
different orientations.

[Ben Goodger (Google), 2011-08-23 16:05:15]

A good looking CL. Close. Can you update the examples to show uses of both types
of scrollbars and add a unittest?

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
File views/controls/scrollbar/base_scroll_bar.cc (right):

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
views/controls/scrollbar/base_scroll_bar.cc:3: // found in the LICENSE file.
Is this code identical to BitmapScrollBar with the bitmap bits removed? It would
be helpful for reviewing this if you had made this file using svn cp so I could
see the differences.

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
File views/controls/scrollbar/base_scroll_bar.h (right):

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
views/controls/scrollbar/base_scroll_bar.h:90: virtual void ScrollToPosition(int
position);
Are these overrides or introduced here? If the latter, there should be
documentation. If the former, they should be OVERRIDE and have a section marker.

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
File views/controls/scrollbar/base_scroll_bar_thumb.h (right):

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
views/controls/scrollbar/base_scroll_bar_thumb.h:53: 
remove the NL

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
views/controls/scrollbar/base_scroll_bar_thumb.h:66: 
remove the NL

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bit...
File views/controls/scrollbar/bitmap_scroll_bar.h (right):

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bit...
views/controls/scrollbar/bitmap_scroll_bar.h:61: protected:
NL before

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
File views/controls/scrollbar/native_scroll_bar_views.cc (right):

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.cc:200: else
no else after return

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
File views/controls/scrollbar/native_scroll_bar_views.h (right):

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:34: // Overridden from View
for layout purpose.
View overrides:

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:45: // Overridden from View.
Move adjacent to other View overrides, and update the .cc order too.

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
views/controls/scrollbar/native_scroll_bar_views.h:52: // Overridden from
NativeScrollBarWrapper.
Change comment style to match the other section headers.

[dnicoara, 2011-08-23 19:46:28]

On 2011/08/23 16:05:15, Ben Goodger (Google) wrote:
> A good looking CL. Close. Can you update the examples to show uses of both
types
> of scrollbars and add a unittest?
> 

Done.

>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> File views/controls/scrollbar/base_scroll_bar.cc (right):
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> views/controls/scrollbar/base_scroll_bar.cc:3: // found in the LICENSE file.
> Is this code identical to BitmapScrollBar with the bitmap bits removed? It
would
> be helpful for reviewing this if you had made this file using svn cp so I
could
> see the differences.
> 

Sorry, in git it looked as if it was moved. The code should be identical.

>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> File views/controls/scrollbar/base_scroll_bar.h (right):
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> views/controls/scrollbar/base_scroll_bar.h:90: virtual void
ScrollToPosition(int
> position);
> Are these overrides or introduced here? If the latter, there should be
> documentation. If the former, they should be OVERRIDE and have a section
marker.
> 
> 

They are introduced here. I added comments for their purpose.

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> File views/controls/scrollbar/base_scroll_bar_thumb.h (right):
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> views/controls/scrollbar/base_scroll_bar_thumb.h:53: 
> remove the NL
> 
> 

Done

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bas...
> views/controls/scrollbar/base_scroll_bar_thumb.h:66: 
> remove the NL
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bit...
> File views/controls/scrollbar/bitmap_scroll_bar.h (right):
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/bit...
> views/controls/scrollbar/bitmap_scroll_bar.h:61: protected:
> NL before
> 
> 

Done

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
> File views/controls/scrollbar/native_scroll_bar_views.cc (right):
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
> views/controls/scrollbar/native_scroll_bar_views.cc:200: else
> no else after return
> 
> 

Done

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
> File views/controls/scrollbar/native_scroll_bar_views.h (right):
> 
>
http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
> views/controls/scrollbar/native_scroll_bar_views.h:34: // Overridden from View
> for layout purpose.
> View overrides:
> 
> 

Done

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
> views/controls/scrollbar/native_scroll_bar_views.h:45: // Overridden from
View.
> Move adjacent to other View overrides, and update the .cc order too.
> 
> 

Done

http://codereview.chromium.org/7669028/diff/5002/views/controls/scrollbar/nat...
> views/controls/scrollbar/native_scroll_bar_views.h:52: // Overridden from
> NativeScrollBarWrapper.
> Change comment style to match the other section headers.

Done

[Ben Goodger (Google), 2011-08-23 21:18:58]

LGTM

[commit-bot: I haz the power, 2011-08-25 12:53:27]

Can't process patch for file views/controls/scrollbar/base_scroll_bar.h.
A +

[commit-bot: I haz the power, 2011-08-25 13:01:31]

Can't process patch for file views/controls/scrollbar/base_scroll_bar.h.
A +