Better CrOS logout button for vertical trays.

Better CrOS logout button for vertical trays.

Improve support for images in MdTextButton.

BUG=617478
Committed: https://crrev.com/71ec8bb0602ef81ba5fb89163bd16c17910bab97
Cr-Commit-Position: refs/heads/master@{#410505}

[Evan Stade, 2016-08-06 02:35:21]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 02:35:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-08-06 02:36:40]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org, tdanderson@chromium.org

[commit-bot: I haz the power, 2016-08-06 03:28:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 03:28:52]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-08-08 15:57:35]

LGTM with comments below

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ash/common/syste...
File ash/common/system/chromeos/session/logout_button_tray.cc (right):

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ash/common/syste...
ash/common/system/chromeos/session/logout_button_tray.cc:132: //
TrayBackgroundView::SetShelfAlignment can lay it out correctly.
nit: () after SetShelfAlignment

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ui/gfx/vector_ic...
File ui/gfx/vector_icons_sources.gypi (right):

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ui/gfx/vector_ic...
ui/gfx/vector_icons_sources.gypi:12: 'vector_icons/ash_logout.1x.icon',
I suggest renaming to shelf_logout to be consistent with the other
shelf-specific icons we already have.

[Evan Stade, 2016-08-08 16:00:57]

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ash/common/syste...
File ash/common/system/chromeos/session/logout_button_tray.cc (right):

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ash/common/syste...
ash/common/system/chromeos/session/logout_button_tray.cc:132: //
TrayBackgroundView::SetShelfAlignment can lay it out correctly.
On 2016/08/08 15:57:35, tdanderson wrote:
> nit: () after SetShelfAlignment

will do

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ui/gfx/vector_ic...
File ui/gfx/vector_icons_sources.gypi (right):

https://chromiumcodereview.appspot.com/2218323002/diff/20001/ui/gfx/vector_ic...
ui/gfx/vector_icons_sources.gypi:12: 'vector_icons/ash_logout.1x.icon',
On 2016/08/08 15:57:35, tdanderson wrote:
> I suggest renaming to shelf_logout to be consistent with the other
> shelf-specific icons we already have.

makes sense

[sadrul, 2016-08-08 20:25:09]

https://codereview.chromium.org/2218323002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/session/logout_button_tray.cc (right):

https://codereview.chromium.org/2218323002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/session/logout_button_tray.cc:194:
button_->SetAccessibleName(title);
Do you need to explicitly do this (since LabelButton::SetText() already does
this too)?

https://codereview.chromium.org/2218323002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/2218323002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:49: virtual void SetText(const
base::string16& text);
Can there a different virtual OnTextChanged() that Md- overrides instead?

[Evan Stade, 2016-08-08 22:40:54]

https://codereview.chromium.org/2218323002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/session/logout_button_tray.cc (right):

https://codereview.chromium.org/2218323002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/session/logout_button_tray.cc:194:
button_->SetAccessibleName(title);
On 2016/08/08 20:25:08, sadrul wrote:
> Do you need to explicitly do this (since LabelButton::SetText() already does
> this too)?

it's necessary in the vertical shelf case. Moved to inside the else branch.

https://codereview.chromium.org/2218323002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/2218323002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:49: virtual void SetText(const
base::string16& text);
On 2016/08/08 20:25:08, sadrul wrote:
> Can there a different virtual OnTextChanged() that Md- overrides instead?

I just went and made this change, but then I realized that this was inconsistent
with SetEnabledTextColors() and AdjustFontSize() (as well as many other
functions like View::SetBorder and View::SetVisible). I don't feel strongly one
way or the other about which is better but I'd rather keep it consistent with
other setters in this class.

[Evan Stade, 2016-08-08 22:48:43]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 22:49:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-08-08 22:56:49]

lgtm

https://codereview.chromium.org/2218323002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/2218323002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:49: virtual void SetText(const
base::string16& text);
On 2016/08/08 22:40:54, Evan Stade wrote:
> On 2016/08/08 20:25:08, sadrul wrote:
> > Can there a different virtual OnTextChanged() that Md- overrides instead?
> 
> I just went and made this change, but then I realized that this was
inconsistent
> with SetEnabledTextColors() and AdjustFontSize() (as well as many other
> functions like View::SetBorder and View::SetVisible). I don't feel strongly
one
> way or the other about which is better but I'd rather keep it consistent with
> other setters in this class.

OK

[Evan Stade, 2016-08-08 23:01:43]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-08-08 23:01:48]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-08-08 23:01:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2218323002/#ps40001
(title: "reviews")

[commit-bot: I haz the power, 2016-08-08 23:03:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 00:03:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 00:03:46]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-08-09 00:03:59]

Description was changed from

==========
Better CrOS logout button for vertical trays.

Improve support for images in MdTextButton.

BUG=617478
==========

to

==========
Better CrOS logout button for vertical trays.

Improve support for images in MdTextButton.

BUG=617478

Committed: https://crrev.com/71ec8bb0602ef81ba5fb89163bd16c17910bab97
Cr-Commit-Position: refs/heads/master@{#410505}
==========

[commit-bot: I haz the power, 2016-08-09 00:04:01]

Patchset 3 (id:??) landed as
https://crrev.com/71ec8bb0602ef81ba5fb89163bd16c17910bab97
Cr-Commit-Position: refs/heads/master@{#410505}