Update CrOS logout button for MD.

Update CrOS logout button for MD.

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

[Evan Stade, 2016-07-26 23:30:43]

Description was changed from

==========
Update CrOS logout button for MD.

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

to

==========
Update CrOS logout button for MD.

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

[Evan Stade, 2016-07-26 23:30:43]

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

[Evan Stade, 2016-07-26 23:30:57]

depends on https://codereview.chromium.org/2187473004/

[tdanderson, 2016-07-27 14:24:57]

tdanderson@chromium.org changed reviewers:
+ yiyix@chromium.org

[tdanderson, 2016-07-27 14:24:57]

LGTM. +yiyix@ as FYI: once this lands can you please check that everything looks
up to spec with this button and your in-progress shelf separators?

https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
File ash/common/system/chromeos/session/logout_button_tray.cc (right):

https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
ash/common/system/chromeos/session/logout_button_tray.cc:60: // TODO(estade):
LogoutButton is not used in MD; remove it when possible.
We're using crbug.com/614453 to track all of the cleanup work needed when MD
becomes default. Can you include a link to that bug here?

https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
ash/common/system/chromeos/session/logout_button_tray.cc:111: // Base font size
+ 2 = 14. TODO(estade): should this 2 be shared with other
nit: TODO on new line

Also, this falls under crbug.com/623987 (general intent to share constants where
possible) - can you include a link to the bug?

[yiyix, 2016-07-27 16:06:56]

On 2016/07/27 14:24:57, tdanderson wrote:
> LGTM. +yiyix@ as FYI: once this lands can you please check that everything
looks
> up to spec with this button and your in-progress shelf separators?
> 
>
https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
> File ash/common/system/chromeos/session/logout_button_tray.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
> ash/common/system/chromeos/session/logout_button_tray.cc:60: // TODO(estade):
> LogoutButton is not used in MD; remove it when possible.
> We're using crbug.com/614453 to track all of the cleanup work needed when MD
> becomes default. Can you include a link to that bug here?
> 
>
https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
> ash/common/system/chromeos/session/logout_button_tray.cc:111: // Base font
size
> + 2 = 14. TODO(estade): should this 2 be shared with other
> nit: TODO on new line
> 
> Also, this falls under crbug.com/623987 (general intent to share constants
where
> possible) - can you include a link to the bug?

The code looks good, my only concern is that the layout of the logout button is
incorrect when the shelf is right aligned or left aligned. Is it intentional?

[tdanderson, 2016-07-27 16:19:58]

On 2016/07/27 16:06:56, yiyix wrote:
> On 2016/07/27 14:24:57, tdanderson wrote:
> > LGTM. +yiyix@ as FYI: once this lands can you please check that everything
> looks
> > up to spec with this button and your in-progress shelf separators?
> > 
> >
>
https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
> > File ash/common/system/chromeos/session/logout_button_tray.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
> > ash/common/system/chromeos/session/logout_button_tray.cc:60: //
TODO(estade):
> > LogoutButton is not used in MD; remove it when possible.
> > We're using crbug.com/614453 to track all of the cleanup work needed when MD
> > becomes default. Can you include a link to that bug here?
> > 
> >
>
https://chromiumcodereview.appspot.com/2190453002/diff/1/ash/common/system/ch...
> > ash/common/system/chromeos/session/logout_button_tray.cc:111: // Base font
> size
> > + 2 = 14. TODO(estade): should this 2 be shared with other
> > nit: TODO on new line
> > 
> > Also, this falls under crbug.com/623987 (general intent to share constants
> where
> > possible) - can you include a link to the bug?
> 
> The code looks good, my only concern is that the layout of the logout button
is
> incorrect when the shelf is right aligned or left aligned. Is it intentional?

Since the logout button looks just as bad on a vertical shelf pre-MD, I'm
not concerned about addressing that as part of this CL. I assume that
in cases where the logout button is shown, the shelf is frozen to horizontal.

Yi, consider pinging the email thread we have about how to show the
logout button to ask and/or try this out for yourself if you are able
to get this account type set up.

[Evan Stade, 2016-07-27 19:20:46]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-07-27 19:20:46]

Added bug links. The button itself looks ok to me with a vertical shelf,
although the rest of the shelf is pretty messed up. But to agree with Terry,
this didn't work before and I don't see a simple resolution, so it's something
to worry about in a future CL.

It looks like the logout button is only showable by policy, but I don't see
anything preventing it from coexisting with a vertical shelf, so I do think we
need to ask Sebastien what do?

+sky for md_text_button* review

[sky, 2016-07-27 19:57:08]

https://codereview.chromium.org/2190453002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2190453002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:232: SetFontList(GetMdFontList());
As anyone can call SetFontList, shouldn't you make it so that when the font
changes you automatically update padding?

[Evan Stade, 2016-07-27 20:09:47]

https://codereview.chromium.org/2190453002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/md_text_button.cc (right):

https://codereview.chromium.org/2190453002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/md_text_button.cc:232: SetFontList(GetMdFontList());
On 2016/07/27 19:57:07, sky wrote:
> As anyone can call SetFontList, shouldn't you make it so that when the font
> changes you automatically update padding?

are you suggesting making SetFontList virtual? I'm kinda hoping no one will want
to (or be allowed to) use that function on MdTextButtons although there's
nothing enforcing that today. If we make the API more restrictive, like
AdjustFontSize(), it encourages people to do the right thing. Here's an example
of existing usage that I think should be discouraged:

just_once_button_->SetFontList(gfx::FontList("Roboto-medium, 13px"));

When just_once_button_ is an MdTextButton this should instead be
just_once_button_->AdjustFontSize(1);

Most usages of SetFontList appear to use FontStyle, e.g.

  no_show_checkbox_->SetFontList(
      ui::ResourceBundle::GetSharedInstance().GetFontList(
      ui::ResourceBundle::MediumFont));

and I agree with Trent's comment here[1] that it should be phased out.

[1]
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.h?rcl=1...

[sky, 2016-07-27 21:26:01]

On Wed, Jul 27, 2016 at 1:09 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/2190453002/diff/20001/ui/views/controls/butto...
> File ui/views/controls/button/md_text_button.cc (right):
>
>
https://codereview.chromium.org/2190453002/diff/20001/ui/views/controls/butto...
> ui/views/controls/button/md_text_button.cc:232:
> SetFontList(GetMdFontList());
> On 2016/07/27 19:57:07, sky wrote:
>> As anyone can call SetFontList, shouldn't you make it so that when the
> font
>> changes you automatically update padding?
>
> are you suggesting making SetFontList virtual?

I was thinking of having it call a virtual function, but yes, they are
roughly the same.

> I'm kinda hoping no one
> will want to (or be allowed to) use that function on MdTextButtons
> although there's nothing enforcing that today.

As long as style guide says public inheritance it's hard to enforce
that, meaning it's easy to do the wrong thing and not know you did it.

  -Scott

> If we make the API more
> restrictive, like AdjustFontSize(), it encourages people to do the right
> thing. Here's an example of existing usage that I think should be
> discouraged:
>
> just_once_button_->SetFontList(gfx::FontList("Roboto-medium, 13px"));
>
> When just_once_button_ is an MdTextButton this should instead be
> just_once_button_->AdjustFontSize(1);
>
> Most usages of SetFontList appear to use FontStyle, e.g.
>
> no_show_checkbox_->SetFontList(
> ui::ResourceBundle::GetSharedInstance().GetFontList(
> ui::ResourceBundle::MediumFont));
>
> and I agree with Trent's comment here[1] that it should be phased out.
>
> [1]
>
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.h?rcl=1...
>
> https://codereview.chromium.org/2190453002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-07-28 02:55:40]

what do you think of making SetFontList virtual, and making
MdTextButton::SetFontList no-op with a NOTREACHED saying "don't use this"?

[sky, 2016-08-01 21:01:29]

Ick!

On Wed, Jul 27, 2016 at 7:55 PM,  <estade@chromium.org> wrote:
> what do you think of making SetFontList virtual, and making
> MdTextButton::SetFontList no-op with a NOTREACHED saying "don't use this"?
>
> https://codereview.chromium.org/2190453002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-08-01 21:57:38]

On 2016/08/01 21:01:29, sky wrote:
> Ick!
> 
> On Wed, Jul 27, 2016 at 7:55 PM,  <mailto:estade@chromium.org> wrote:
> > what do you think of making SetFontList virtual, and making
> > MdTextButton::SetFontList no-op with a NOTREACHED saying "don't use this"?
> >
> > https://codereview.chromium.org/2190453002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

how else can we deprecate that function? It would be nice if, as we went around
updating buttons for MD, we were forced to address this issue as appropriate
(because of switching from LabelButton to MdTextButton).

[sky, 2016-08-02 13:30:06]

The problem with what you propose is that it's easy to miss a call to
the function in some code path. So, the code can compile and work fine
in some circumstances but then you hit the code path and the wrong
behavior results.

Explicitly setting the font is a reasonable use of a button. Seems to
me we should continue to support it.

  -Scott

On Mon, Aug 1, 2016 at 2:57 PM,  <estade@chromium.org> wrote:
> On 2016/08/01 21:01:29, sky wrote:
>> Ick!
>>
>> On Wed, Jul 27, 2016 at 7:55 PM, <mailto:estade@chromium.org> wrote:
>> > what do you think of making SetFontList virtual, and making
>> > MdTextButton::SetFontList no-op with a NOTREACHED saying "don't use
>> > this"?
>> >
>> > https://codereview.chromium.org/2190453002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> how else can we deprecate that function? It would be nice if, as we went
> around
> updating buttons for MD, we were forced to address this issue as appropriate
> (because of switching from LabelButton to MdTextButton).
>
> https://codereview.chromium.org/2190453002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-08-02 14:01:32]

On 2016/08/02 13:30:06, sky wrote:
> The problem with what you propose is that it's easy to miss a call to
> the function in some code path. So, the code can compile and work fine
> in some circumstances but then you hit the code path and the wrong
> behavior results.
> 
> Explicitly setting the font is a reasonable use of a button. Seems to
> me we should continue to support it.

I don't agree that we should allow people to set arbitrary fonts. I can't think
of a reason to allow setting a different typeface, for example. This just leads
to mistakes where engineers see a mock with roboto (because it was created in
the context of chromeos) and think they need to apply roboto manually on Windows
or Mac or wherever. The set of allowable changes to fonts can be whittled down
to probably just size. I don't even think weight should be modifiable because it
will just lead to inconsistencies. Particularly in the case of "standard looking
button", i.e. MdTextButton, I think we should be more restrictive. ainslie@ et
al are busy trying to come up with design guidelines that apply to all secondary
UI, and I'm simultaneously trying to make it harder for engineers to diverge
from these guidelines.

In codesearch I see 15 calls to LabelButton::SetFontList, broken down as
follows:
1 seems completely pointless
6 just change the size
2 change the weight but will go away in MD land
3 calls set it to "Roboto-regular, 13px", which should actually just be +1 font
size
2 are unit tests
1 is inside MdTextButton

[sky, 2016-08-02 19:25:24]

Fair enough, and if we are going to remove the function entirely I'm ok with
temporary solutions.

How about providing SetFontList in MdTextButton and making it private? That way
folks can't call it directly.

[Evan Stade, 2016-08-03 14:02:55]

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

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

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-03 14:03:48]

On 2016/08/02 19:25:24, sky wrote:
> Fair enough, and if we are going to remove the function entirely I'm ok with
> temporary solutions.
> 
> How about providing SetFontList in MdTextButton and making it private? That
way
> folks can't call it directly.

done (s/private/protected)

[Evan Stade, 2016-08-03 14:13:47]

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

[commit-bot: I haz the power, 2016-08-03 14:14:02]

Dry run: 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-03 14:45:27]

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

[commit-bot: I haz the power, 2016-08-03 14:45:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-03 15:36:36]

LGTM

[Evan Stade, 2016-08-03 15:39:47]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-08-03 15:39:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2190453002/#ps80001
(title: "fix override")

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

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-03 16:37:26]

Description was changed from

==========
Update CrOS logout button for MD.

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

to

==========
Update CrOS logout button for MD.

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

[commit-bot: I haz the power, 2016-08-03 16:37:28]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Update CrOS logout button for MD.

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

to

==========
Update CrOS logout button for MD.

BUG=617478

Committed: https://crrev.com/ab41358106be0e51b0ef68f0354da9e8b3471175
Cr-Commit-Position: refs/heads/master@{#409535}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/ab41358106be0e51b0ef68f0354da9e8b3471175
Cr-Commit-Position: refs/heads/master@{#409535}