Win: Add listbox view.

Win: Add listbox view.

See http://codereview.chromium.org/2799042 for how I intend to use this.

BUG=45546
TEST=None

BUG=45546
TEST=None

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

[Nico, 2010-06-29 21:43:06]

I tried to use views/controls/table, but it has the following shortcomings:

1.) I didn't find a way to enforce that exactly one row is selected at all
times.
2.) I didn't find a way to have a single column that fills the full width of the
table.

Writing a wrapping class around the listbox common control was easy and
addresses these concerns.

The view is not necessary on chromeos, but I made it so that it's easy to add
views/gtk support.

[sky, 2010-06-29 22:22:34]

http://codereview.chromium.org/2815034/diff/40001/4004
File views/controls/listbox/listbox.cc (right):

http://codereview.chromium.org/2815034/diff/40001/4004#newcode42
views/controls/listbox/listbox.cc:42: Layout();
View::DidChangeBounds does this, so you shouldn't need to override it.

http://codereview.chromium.org/2815034/diff/40001/4004#newcode46
views/controls/listbox/listbox.cc:46: if (!native_wrapper_)
Can you use FillLayout instead of this?

http://codereview.chromium.org/2815034/diff/40001/4005
File views/controls/listbox/listbox.h (right):

http://codereview.chromium.org/2815034/diff/40001/4005#newcode41
views/controls/listbox/listbox.h:41: // Returns the 0-based index of the
currently selected row.
Document what this returns if nothing is selected.

http://codereview.chromium.org/2815034/diff/40001/4005#newcode53
views/controls/listbox/listbox.h:53: void Layout();
nit: newline between 53 and 54.

http://codereview.chromium.org/2815034/diff/40001/4006
File views/controls/listbox/native_listbox_win.cc (right):

http://codereview.chromium.org/2815034/diff/40001/4006#newcode79
views/controls/listbox/native_listbox_win.cc:79: LPARAM l_param, LRESULT*
result) {
nit: spacing

http://codereview.chromium.org/2815034/diff/40001/4006#newcode114
views/controls/listbox/native_listbox_win.cc:114: ListBox_AddString(hwnd,
strings_[i].c_str());
UTF16ToWide

http://codereview.chromium.org/2815034/diff/40001/4006#newcode118
views/controls/listbox/native_listbox_win.cc:118: SetWindowLongPtr(hwnd,
GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
Do you need this? It doesn't seem like you use it.

http://codereview.chromium.org/2815034/diff/40001/4007
File views/controls/listbox/native_listbox_win.h (right):

http://codereview.chromium.org/2815034/diff/40001/4007#newcode12
views/controls/listbox/native_listbox_win.h:12: #include
"views/controls/listbox/native_listbox_wrapper.h"
This should be before line 11.

http://codereview.chromium.org/2815034/diff/40001/4007#newcode19
views/controls/listbox/native_listbox_win.h:19: explicit
NativeListboxWin(Listbox* listbox,
nit: one space indent from public.

http://codereview.chromium.org/2815034/diff/40001/4007#newcode26
views/controls/listbox/native_listbox_win.h:26: int SelectedRow() const;
Make all your overrides virtual.

http://codereview.chromium.org/2815034/diff/40001/4010
File views/views.gyp (right):

http://codereview.chromium.org/2815034/diff/40001/4010#newcode100
views/views.gyp:100: 'controls/listbox/native_listbox_wrapper.h',
You should only compile these on windows.

[Nico, 2010-06-29 22:49:23]

Thanks!

http://codereview.chromium.org/2815034/diff/40001/4004
File views/controls/listbox/listbox.cc (right):

http://codereview.chromium.org/2815034/diff/40001/4004#newcode42
views/controls/listbox/listbox.cc:42: Layout();
On 2010/06/29 22:22:34, sky wrote:
> View::DidChangeBounds does this, so you shouldn't need to override it.

Done.

http://codereview.chromium.org/2815034/diff/40001/4004#newcode46
views/controls/listbox/listbox.cc:46: if (!native_wrapper_)
On 2010/06/29 22:22:34, sky wrote:
> Can you use FillLayout instead of this?

Nice! Done.

http://codereview.chromium.org/2815034/diff/40001/4005
File views/controls/listbox/listbox.h (right):

http://codereview.chromium.org/2815034/diff/40001/4005#newcode41
views/controls/listbox/listbox.h:41: // Returns the 0-based index of the
currently selected row.
On 2010/06/29 22:22:34, sky wrote:
> Document what this returns if nothing is selected.

Done.

http://codereview.chromium.org/2815034/diff/40001/4005#newcode53
views/controls/listbox/listbox.h:53: void Layout();
On 2010/06/29 22:22:34, sky wrote:
> nit: newline between 53 and 54.

Done.

http://codereview.chromium.org/2815034/diff/40001/4006
File views/controls/listbox/native_listbox_win.cc (right):

http://codereview.chromium.org/2815034/diff/40001/4006#newcode79
views/controls/listbox/native_listbox_win.cc:79: LPARAM l_param, LRESULT*
result) {
On 2010/06/29 22:22:34, sky wrote:
> nit: spacing

Done.

http://codereview.chromium.org/2815034/diff/40001/4006#newcode114
views/controls/listbox/native_listbox_win.cc:114: ListBox_AddString(hwnd,
strings_[i].c_str());
On 2010/06/29 22:22:34, sky wrote:
> UTF16ToWide

Done.

http://codereview.chromium.org/2815034/diff/40001/4006#newcode118
views/controls/listbox/native_listbox_win.cc:118: SetWindowLongPtr(hwnd,
GWLP_USERDATA, reinterpret_cast<LONG_PTR>(this));
On 2010/06/29 22:22:34, sky wrote:
> Do you need this? It doesn't seem like you use it.

Done. (copypasta.)

http://codereview.chromium.org/2815034/diff/40001/4007
File views/controls/listbox/native_listbox_win.h (right):

http://codereview.chromium.org/2815034/diff/40001/4007#newcode19
views/controls/listbox/native_listbox_win.h:19: explicit
NativeListboxWin(Listbox* listbox,
On 2010/06/29 22:22:34, sky wrote:
> nit: one space indent from public.

Done.

http://codereview.chromium.org/2815034/diff/40001/4007#newcode26
views/controls/listbox/native_listbox_win.h:26: int SelectedRow() const;
On 2010/06/29 22:22:34, sky wrote:
> Make all your overrides virtual.

Done.

http://codereview.chromium.org/2815034/diff/40001/4010
File views/views.gyp (right):

http://codereview.chromium.org/2815034/diff/40001/4010#newcode100
views/views.gyp:100: 'controls/listbox/native_listbox_wrapper.h',
On 2010/06/29 22:22:34, sky wrote:
> You should only compile these on windows.

Done.

[sky, 2010-06-29 23:17:24]

LGTM