views: Switch Checkbox over to LabelButton.

views: Switch Checkbox over to LabelButton.

Rebase Checkbox and RadioButton on LabelButton.
Uses new Checkbox and Radio Button image assets.
(the classes previously used skia+native_theme)

See before/after comparison at http://crbug.com/155363#c79
Note: Height on Linux is slightly reduced, now closer to Windows.

BUG=155363
TEST=checkboxes and radio buttons look good in production, and in ash_shell "Examples Widgets" and "Views Examples" dialogs.
R=msw@chromium.org,sky@chromium.org,dbeam@chromium.org
COLLABORATOR=msw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201740

[msw, 2013-05-13 01:37:22]

Now that's the kind of change that I'm talking about :) Great start!

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/button.h (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/button.h:42: // TODO(msw): Add "Blue Button", Checkbox,
Radio, Menu, ComboBox, etc.
nit: update my TODO :)

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/button.h:48: STYLE_RADIO_BUTTON,
nit: consider STYLE_RADIO.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:7: #include "base/logging.h"
nit: remove

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:9: #include "ui/gfx/canvas.h"
nit: remove

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:10: #include "ui/views/controls/label.h"
nit: remove

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:18: const int kCheckboxLabelSpacing = 4;
Remove this, just use the LabelButton default Image/Text spacing.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:20: const int kFocusBorderWidth = 1;
Let's try to remove this and use default LabelButton focus border, take pics.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:82: RequestFocus();
nit: try to remove this.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:12: #include
"ui/views/controls/button/text_button.h"
nit: try to remove this and CheckboxNativeThemeBorder

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:61: // Sets/Gets whether or not the checkbox
is checked.
Add STATE_TOGGLED to LabelButton, use that for check/radio (later menu, etc).

[tfarina, 2013-05-13 01:47:12]

I'll continue tomorrow night.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/button.h (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/button.h:42: // TODO(msw): Add "Blue Button", Checkbox,
Radio, Menu, ComboBox, etc.
On 2013/05/13 01:37:22, msw wrote:
> nit: update my TODO :)

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/button.h:48: STYLE_RADIO_BUTTON,
On 2013/05/13 01:37:22, msw wrote:
> nit: consider STYLE_RADIO.

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:7: #include "base/logging.h"
On 2013/05/13 01:37:22, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:9: #include "ui/gfx/canvas.h"
On 2013/05/13 01:37:22, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:10: #include "ui/views/controls/label.h"
On 2013/05/13 01:37:22, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:18: const int kCheckboxLabelSpacing = 4;
On 2013/05/13 01:37:22, msw wrote:
> Remove this, just use the LabelButton default Image/Text spacing.

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:20: const int kFocusBorderWidth = 1;
On 2013/05/13 01:37:22, msw wrote:
> Let's try to remove this and use default LabelButton focus border, take pics.

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:82: RequestFocus();
On 2013/05/13 01:37:22, msw wrote:
> nit: try to remove this.

Done.

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:12: #include
"ui/views/controls/button/text_button.h"
On 2013/05/13 01:37:22, msw wrote:
> nit: try to remove this and CheckboxNativeThemeBorder

Done.

[tfarina, 2013-05-13 02:28:31]

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/10002/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:61: // Sets/Gets whether or not the checkbox
is checked.
On 2013/05/13 01:37:22, msw wrote:
> Add STATE_TOGGLED to LabelButton, use that for check/radio (later menu, etc).

Done.

[msw, 2013-05-16 22:40:05]

How's this looking? Can you post before/after pics?

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
File ui/views/controls/button/button.h (right):

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/button.h:38: STATE_TOGGLED,
nit: I'd prefer this ordered before STATE_DISABLED.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:304: case STATE_TOGGLED:  return
ui::NativeTheme::kToggled;
nit: I'd prefer this to come before STATE_DISABLED.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:333: void
LabelButton::GetExtraParams(ui::NativeTheme::ExtraParams* params) const {
Move this impl to match the changed decl ordering.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
File ui/views/controls/button/label_button_border.cc (right):

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:97:
//DCHECK(!strcmp(view.GetClassName(), LabelButton::kViewClassName));
You can just remove this DCHECK.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:142: if (style() ==
Button::STYLE_CHECKBOX)
Would you mind making this function a switch instead? Then
STYLE_NATIVE_TEXTBUTTON / STYLE_CHECKBOX / STYLE_RADIO could all use the same
case statement.

[tfarina, 2013-05-18 00:54:08]

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
File ui/views/controls/button/button.h (right):

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/button.h:38: STATE_TOGGLED,
On 2013/05/16 22:40:05, msw wrote:
> nit: I'd prefer this ordered before STATE_DISABLED.

removed

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:304: case STATE_TOGGLED:  return
ui::NativeTheme::kToggled;
On 2013/05/16 22:40:05, msw wrote:
> nit: I'd prefer this to come before STATE_DISABLED.
ditto.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:333: void
LabelButton::GetExtraParams(ui::NativeTheme::ExtraParams* params) const {
On 2013/05/16 22:40:05, msw wrote:
> Move this impl to match the changed decl ordering.

Done.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
File ui/views/controls/button/label_button_border.cc (right):

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:97:
//DCHECK(!strcmp(view.GetClassName(), LabelButton::kViewClassName));
On 2013/05/16 22:40:05, msw wrote:
> You can just remove this DCHECK.

Done.

https://codereview.chromium.org/15061006/diff/30001/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:142: if (style() ==
Button::STYLE_CHECKBOX)
On 2013/05/16 22:40:05, msw wrote:
> Would you mind making this function a switch instead? Then
> STYLE_NATIVE_TEXTBUTTON / STYLE_CHECKBOX / STYLE_RADIO could all use the same
> case statement.

Done.

[msw, 2013-05-18 01:04:56]

More comments, let me know when you need another pass.
Also, LMK if you have trouble adding/using checked images.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:17: : LabelButton(NULL, label) {
Restore the checked_(false) init.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:18: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
nit: move this below SetStyle (just before its use).

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
File ui/views/controls/button/label_button_border.cc (right):

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:137: case Button::STYLE_BUTTON:
Remove this duplicated case for STYLE_BUTTON.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:145: default:
Make this a STYLE_COUNT case, and don't offer a default.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:149: NOTREACHED();
Can you remove this NOTREACHED and the extra return below?
(if you can't remove this return, that's okay)

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
File ui/views/controls/button/radio_button.cc (right):

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/radio_button.cc:20: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
nit: move this below SetStyle (just before its use).

[tfarina, 2013-05-18 02:30:49]

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:17: : LabelButton(NULL, label) {
On 2013/05/18 01:04:56, msw wrote:
> Restore the checked_(false) init.

Done.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:18: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
On 2013/05/18 01:04:56, msw wrote:
> nit: move this below SetStyle (just before its use).

Done.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:18: ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();
On 2013/05/18 01:04:56, msw wrote:
> nit: move this below SetStyle (just before its use).

Done.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
File ui/views/controls/button/label_button_border.cc (right):

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:137: case Button::STYLE_BUTTON:
On 2013/05/18 01:04:56, msw wrote:
> Remove this duplicated case for STYLE_BUTTON.

Done.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:145: default:
On 2013/05/18 01:04:56, msw wrote:
> Make this a STYLE_COUNT case, and don't offer a default.

Done.

https://codereview.chromium.org/15061006/diff/58010/ui/views/controls/button/...
ui/views/controls/button/label_button_border.cc:149: NOTREACHED();
On 2013/05/18 01:04:56, msw wrote:
> Can you remove this NOTREACHED and the extra return below?
> (if you can't remove this return, that's okay)

Done.

[msw, 2013-05-18 03:46:30]

More comments, but relax this weekend, enjoy family time!

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:38: virtual const gfx::ImageSkia&
GetImage(ButtonState for_state) OVERRIDE;
group this with the functions above and change the comment to Overridden from
LabelButton.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:39: void SetCheckedImage(ButtonState
for_state, const gfx::ImageSkia& image);
Add a blank line and this comment above:
// Set the image shown for the specified checked button state.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:52: gfx::ImageSkia
checked_state_images_[STATE_COUNT];
Comment: // The images for each checked button state.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:69: const gfx::ImageSkia&
LabelButton::GetFocusedImage(ButtonState for_state) {
Don't make a separate GetFocusedImage. Check if the button is focused in
GetImage, instead of hardcoding [0] up there and [1] here, like this:
const size_t focused_index = HasFocus() ? 1 : 0;
... button_state_images_[focused_index][for_state] ...

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/label_button.h:107: // The image shown in the button.
nit: s/image/label/

[tfarina, 2013-05-18 18:43:23]

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:38: virtual const gfx::ImageSkia&
GetImage(ButtonState for_state) OVERRIDE;
On 2013/05/18 03:46:30, msw wrote:
> group this with the functions above and change the comment to Overridden from
> LabelButton.

Done.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:39: void SetCheckedImage(ButtonState
for_state, const gfx::ImageSkia& image);
On 2013/05/18 03:46:30, msw wrote:
> Add a blank line and this comment above:
> // Set the image shown for the specified checked button state.

Done.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:52: gfx::ImageSkia
checked_state_images_[STATE_COUNT];
On 2013/05/18 03:46:30, msw wrote:
> Comment: // The images for each checked button state.

Done.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/label_button.cc:69: const gfx::ImageSkia&
LabelButton::GetFocusedImage(ButtonState for_state) {
On 2013/05/18 03:46:30, msw wrote:
> Don't make a separate GetFocusedImage. Check if the button is focused in
> GetImage, instead of hardcoding [0] up there and [1] here, like this:
> const size_t focused_index = HasFocus() ? 1 : 0;
> ... button_state_images_[focused_index][for_state] ...

Done.

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/15061006/diff/69005/ui/views/controls/button/...
ui/views/controls/button/label_button.h:107: // The image shown in the button.
On 2013/05/18 03:46:30, msw wrote:
> nit: s/image/label/

Done.

[tfarina, 2013-05-18 18:44:49]

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:35:
*rb.GetImageSkiaNamed(IDR_CHECKBOX_CHECKED_PRESSED));
We still need IDR_CHECKBOX_FOCUSED_CHECKED, IDR_CHECKBOX_FOCUSED_CHECKED_HOVER,
IDR_CHECKBOX_FOCUSED_CHECKED_PRESSED.

Where that will go?

[msw, 2013-05-18 19:42:09]

Please tell me there are no more button images we can't handle after this.

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:54: gfx::ImageSkia
checked_state_images_[STATE_COUNT];
Sigh, I guess rather than complicating LabelButton more, this should become:
gfx::ImageSkia images_[2][2][STATE_COUNT];

Then change SetCheckedImage to:
SetCustomImage(bool checked, bool focused, ButtonState for_state, const
gfx::ImageSkia& image) {
const size_t checked_index = checked ? 1 : 0;
const size_t focused_index = focused ? 1 : 0;
images[checked_index][focused_index][for_state] = image;
...
}

And change the overridden GetImage to do the same with:
const size_t checked_index = checked() ? 1 : 0;
const size_t focused_index = HasFocus() ? 1 : 0;

Then undo the SetFocusedImage changes to LabelButton.

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
ui/views/controls/button/label_button.h:31: const gfx::ImageSkia&
GetFocusedImage(ButtonState for_state);
remove

[tfarina, 2013-05-18 21:10:33]

The behavior of those buttons are still broken. Looks dumb :(

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:54: gfx::ImageSkia
checked_state_images_[STATE_COUNT];
On 2013/05/18 19:42:09, msw wrote:
> Sigh, I guess rather than complicating LabelButton more, this should become:
> gfx::ImageSkia images_[2][2][STATE_COUNT];
> 
> Then change SetCheckedImage to:
> SetCustomImage(bool checked, bool focused, ButtonState for_state, const
> gfx::ImageSkia& image) {
> const size_t checked_index = checked ? 1 : 0;
> const size_t focused_index = focused ? 1 : 0;
> images[checked_index][focused_index][for_state] = image;
> ...
> }
> 
> And change the overridden GetImage to do the same with:
> const size_t checked_index = checked() ? 1 : 0;
> const size_t focused_index = HasFocus() ? 1 : 0;
> 
> Then undo the SetFocusedImage changes to LabelButton.

Done.

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/15061006/diff/82001/ui/views/controls/button/...
ui/views/controls/button/label_button.h:31: const gfx::ImageSkia&
GetFocusedImage(ButtonState for_state);
On 2013/05/18 19:42:09, msw wrote:
> remove

Done.

[msw, 2013-05-21 00:35:17]

If you follow my suggestions, it should work as expected.
If anything, you *might* also need to trigger image changes on focused/checked
state changes, but maybe not.

This is blocking transparency in dialogs with checks/radios.
I'll patch this in and help on it tomorrow, if needed.
Let's get this in ASAP, we can polish in followups.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:23: SetImage(STATE_NORMAL,
*rb.GetImageSkiaNamed(IDR_CHECKBOX));
nit: I'm not sure how you ordered these, I'd do:
(1) all unchecked&unfocused states
(2) all checked&unfocused states
(3) all unchecked&focused states
(4) all checked&focused states
And do the same for the radio button class init.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:25: true, false, STATE_NORMAL,
*rb.GetImageSkiaNamed(IDR_CHECKBOX_CHECKED));
nit: put 3 state args on the line above, do the same below and for radio.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:26: SetCustomImage(true,
nit: combine 3 state args onto one line here and below and for radio.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:56: SetImage(STATE_HOVERED,
*rb.GetImageSkiaNamed(IDR_CHECKBOX_HOVER));
Use SetCustomImage for everything, same for radio.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:79: if (!checked_)
I don't think we want this early return at all any more.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:39: // Set the image shown for each button
state depending on wheter it is
nit: whether

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
File ui/views/controls/button/radio_button.cc (right):

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/radio_button.cc:26: SetCustomImage(
nit: ditto for 3 state args on SetCustomImage lines and ordering.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/radio_button.cc:58: SetImage(STATE_HOVERED,
*rb.GetImageSkiaNamed(IDR_RADIO_HOVER));
ditto for using SetCustomImage for everything.

[tfarina, 2013-05-21 01:56:52]

This isn't in an usable state. :(

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:23: SetImage(STATE_NORMAL,
*rb.GetImageSkiaNamed(IDR_CHECKBOX));
On 2013/05/21 00:35:17, msw wrote:
> nit: I'm not sure how you ordered these, I'd do:
> (1) all unchecked&unfocused states
> (2) all checked&unfocused states
> (3) all unchecked&focused states
> (4) all checked&focused states
> And do the same for the radio button class init.

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:25: true, false, STATE_NORMAL,
*rb.GetImageSkiaNamed(IDR_CHECKBOX_CHECKED));
On 2013/05/21 00:35:17, msw wrote:
> nit: put 3 state args on the line above, do the same below and for radio.

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:26: SetCustomImage(true,
On 2013/05/21 00:35:17, msw wrote:
> nit: combine 3 state args onto one line here and below and for radio.

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:56: SetImage(STATE_HOVERED,
*rb.GetImageSkiaNamed(IDR_CHECKBOX_HOVER));
On 2013/05/21 00:35:17, msw wrote:
> Use SetCustomImage for everything, same for radio.

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.cc:79: if (!checked_)
On 2013/05/21 00:35:17, msw wrote:
> I don't think we want this early return at all any more.

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
File ui/views/controls/button/checkbox.h (right):

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/checkbox.h:39: // Set the image shown for each button
state depending on wheter it is
On 2013/05/21 00:35:17, msw wrote:
> nit: whether

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
File ui/views/controls/button/radio_button.cc (right):

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/radio_button.cc:26: SetCustomImage(
On 2013/05/21 00:35:17, msw wrote:
> nit: ditto for 3 state args on SetCustomImage lines and ordering.

Done.

https://codereview.chromium.org/15061006/diff/90002/ui/views/controls/button/...
ui/views/controls/button/radio_button.cc:58: SetImage(STATE_HOVERED,
*rb.GetImageSkiaNamed(IDR_RADIO_HOVER));
On 2013/05/21 00:35:17, msw wrote:
> ditto for using SetCustomImage for everything.

Done.

[msw, 2013-05-21 02:24:11]

Glad to hear that it's looking alright; please include some screenshots after
you address my comments.

https://codereview.chromium.org/15061006/diff/103001/ui/views/controls/button...
File ui/views/controls/button/button.h (right):

https://codereview.chromium.org/15061006/diff/103001/ui/views/controls/button...
ui/views/controls/button/button.h:46: STYLE_CHECKBOX,
Sorry for the reversal here, but remove these styles and their use from
LabelButtonBorder::GetInsets(), instead add a LabelButtonBorder::insets_ and
SetInsets(), and make LabelButtonBorder::GetInsets() return |insets_| before the
switch if they're not empty().

Then make Checkbox cast border() to a LabelButtonBorder and call SetInsets()
with the proper values. RadioButton will just inherit the correct insets from
Checkbox.

https://codereview.chromium.org/15061006/diff/103001/ui/views/controls/button...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/15061006/diff/103001/ui/views/controls/button...
ui/views/controls/button/label_button.h:74: ImageView* image_;
nit: revert this change to expose |image_|, I think SetCustomImage should work
okay during initialization without explicitly resetting |image_|'s content.

[tfarina, 2013-05-21 02:50:53]

http://imgur.com/2DYw7wq

Looks a bit better, but the transition of the states are pretty broken. You will
see what I mean when you patch this in.

https://codereview.chromium.org/15061006/diff/103001/ui/views/controls/button...
File ui/views/controls/button/button.h (right):

https://codereview.chromium.org/15061006/diff/103001/ui/views/controls/button...
ui/views/controls/button/button.h:46: STYLE_CHECKBOX,
On 2013/05/21 02:24:11, msw wrote:
> Sorry for the reversal here, but remove these styles and their use from
> LabelButtonBorder::GetInsets(), instead add a LabelButtonBorder::insets_ and
> SetInsets(), and make LabelButtonBorder::GetInsets() return |insets_| before
the
> switch if they're not empty().
> 
> Then make Checkbox cast border() to a LabelButtonBorder and call SetInsets()
> with the proper values. RadioButton will just inherit the correct insets from
> Checkbox.

Done.

[msw, 2013-05-22 00:36:10]

https://codereview.chromium.org/15061006/diff/108004/ui/views/controls/button...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/108004/ui/views/controls/button...
ui/views/controls/button/checkbox.cc:30: SetCustomImage(false, false,
STATE_DISABLED,
nit: remove the disabled comment and group this with the other
"Unchecked/Unfocused images", at the end (in the order that matches the STATE_*
enum); same for radio.

[tfarina, 2013-05-22 03:03:59]

https://codereview.chromium.org/15061006/diff/108004/ui/views/controls/button...
File ui/views/controls/button/checkbox.cc (right):

https://codereview.chromium.org/15061006/diff/108004/ui/views/controls/button...
ui/views/controls/button/checkbox.cc:30: SetCustomImage(false, false,
STATE_DISABLED,
On 2013/05/22 00:36:10, msw wrote:
> nit: remove the disabled comment and group this with the other
> "Unchecked/Unfocused images", at the end (in the order that matches the
STATE_*
> enum); same for radio.

Done.

[msw, 2013-05-22 04:08:27]

Hey Scott, please take a look; thanks!
Dan, please test autofill changes on Win and Linux; thanks!

[sky, 2013-05-22 15:40:20]

LGTM

[msw, 2013-05-23 00:50:20]

I uploaded Patch Set 25, which fixes RTL alignment and focus border placement
(with custom insets); landing.

[commit-bot: I haz the power, 2013-05-23 00:52:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/15061006/184001

[commit-bot: I haz the power, 2013-05-23 01:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/15061006/211001

[commit-bot: I haz the power, 2013-05-23 02:03:28]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-05-23 02:07:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/15061006/211001

[commit-bot: I haz the power, 2013-05-23 10:45:53]

Change committed as 201740