[MD] Apply appropriate call to action styling to infobar buttons.

[MD] Apply appropriate call to action styling to infobar buttons.

BUG=596238
Committed: https://crrev.com/5d04bab763700c788f8084337e70a805be2ffa2d
Cr-Commit-Position: refs/heads/master@{#383545}

[Evan Stade, 2016-03-23 22:53:33]

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

[commit-bot: I haz the power, 2016-03-23 22:54:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831673002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831673002/1

[commit-bot: I haz the power, 2016-03-23 23:09:13]

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

[commit-bot: I haz the power, 2016-03-23 23:09:13]

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

[Evan Stade, 2016-03-24 21:26:05]

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

[Peter Kasting, 2016-03-25 04:57:48]

LGTM

https://codereview.chromium.org/1831673002/diff/10001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/1831673002/diff/10001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/confirm_infobar.cc:87: }
Nit: It seems unfortunate to have this mostly-duplicated block, especially
because it's easy to miss the difference in how the call to action is set
between the two buttons.

Something like this refactoring would use the same code to create both buttons. 
You could also collapse the first helper into the second helper and write things
more like they appear in the current CL, but this seemed a little cleaner.

namespace {

views::LabelButton* CreateButton(
    ConfirmInfoBarDelegate* delegate,
    ConfirmInfoBarDelegate::InfoBarButton button_type) {
  if (!ui::MaterialDesignController::IsModeMaterial())
    return CreateTextButton(this, delegate->GetButtonLabel(button_type));

  views::MdTextButton* button =
      CreateMdTextButton(this, delegate->GetButtonLabel(button_type));

  // When there are two buttons, the OK button gets a strong call to action and
  // the cancel button gets none.
  const views::MdTextButton::CallToAction two_button_cta =
      (button_type == ConfirmInfoBarDelegate::BUTTON_OK)
          ? views::MdTextButton::STRONG_CALL_TO_ACTION
          : views::MdTextButton::NO_CALL_TO_ACTION;
  button->SetCallToAction((delegate->GetButtons() == button_type)
                              ? views::MdTextButton::WEAK_CALL_TO_ACTION
                              : two_button_cta);
  return button;
}

}  // namespace

views::LabelButton* ConfirmInfoBar::CreateAndAddButton(
    ConfirmInfoBarDelegate::InfoBarButton button_type) {
  views::LabelButton* button = CreateButton(GetDelegate(), button_type);
  AddViewToContentArea(button);
  button->SizeToPreferredSize();
  return button;
}

void ConfirmInfoBar::ViewHierarchyChanged(...) {
  ...
    if (delegate->GetButtons() & ConfirmInfoBarDelegate::BUTTON_OK) {
      ok_button_ = CreateAndAddButton(ConfirmInfoBarDelegate::BUTTON_OK);
      if (delegate->OKButtonTriggersUACPrompt()) {
        ...
      }
    }
    if (delegate->GetButtons() & ConfirmInfoBarDelegate::BUTTON_CANCEL) {
      cancel_button_ =
          CreateAndAddButton(ConfirmInfoBarDelegate::BUTTON_CANCEL);
    }

[Evan Stade, 2016-03-28 19:13:43]

https://codereview.chromium.org/1831673002/diff/10001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/1831673002/diff/10001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/confirm_infobar.cc:87: }
On 2016/03/25 04:57:48, Peter Kasting wrote:
> Nit: It seems unfortunate to have this mostly-duplicated block, especially
> because it's easy to miss the difference in how the call to action is set
> between the two buttons.
> 
> Something like this refactoring would use the same code to create both
buttons. 
> You could also collapse the first helper into the second helper and write
things
> more like they appear in the current CL, but this seemed a little cleaner.
> 
> namespace {
> 
> views::LabelButton* CreateButton(
>     ConfirmInfoBarDelegate* delegate,
>     ConfirmInfoBarDelegate::InfoBarButton button_type) {
>   if (!ui::MaterialDesignController::IsModeMaterial())
>     return CreateTextButton(this, delegate->GetButtonLabel(button_type));
> 
>   views::MdTextButton* button =
>       CreateMdTextButton(this, delegate->GetButtonLabel(button_type));
> 
>   // When there are two buttons, the OK button gets a strong call to action
and
>   // the cancel button gets none.
>   const views::MdTextButton::CallToAction two_button_cta =
>       (button_type == ConfirmInfoBarDelegate::BUTTON_OK)
>           ? views::MdTextButton::STRONG_CALL_TO_ACTION
>           : views::MdTextButton::NO_CALL_TO_ACTION;
>   button->SetCallToAction((delegate->GetButtons() == button_type)
>                               ? views::MdTextButton::WEAK_CALL_TO_ACTION
>                               : two_button_cta);
>   return button;
> }
> 
> }  // namespace
> 
> views::LabelButton* ConfirmInfoBar::CreateAndAddButton(
>     ConfirmInfoBarDelegate::InfoBarButton button_type) {
>   views::LabelButton* button = CreateButton(GetDelegate(), button_type);
>   AddViewToContentArea(button);
>   button->SizeToPreferredSize();
>   return button;
> }
> 
> void ConfirmInfoBar::ViewHierarchyChanged(...) {
>   ...
>     if (delegate->GetButtons() & ConfirmInfoBarDelegate::BUTTON_OK) {
>       ok_button_ = CreateAndAddButton(ConfirmInfoBarDelegate::BUTTON_OK);
>       if (delegate->OKButtonTriggersUACPrompt()) {
>         ...
>       }
>     }
>     if (delegate->GetButtons() & ConfirmInfoBarDelegate::BUTTON_CANCEL) {
>       cancel_button_ =
>           CreateAndAddButton(ConfirmInfoBarDelegate::BUTTON_CANCEL);
>     }

I don't see this as a huge win because I don't see that much duplication. The
additional ternary is not really de-duping anything. I take consolation from the
thought of how this code will look like when the non-material path is gone (no
need for temporary button var for example).

[Evan Stade, 2016-03-28 19:13:47]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-03-28 19:14:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1831673002/10001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1831673002/10001

[commit-bot: I haz the power, 2016-03-28 19:55:53]

Committed patchset #2 (id:10001)

[commit-bot: I haz the power, 2016-03-28 19:58:07]

Description was changed from

==========
[MD] Apply appropriate call to action styling to infobar buttons.

BUG=596238
==========

to

==========
[MD] Apply appropriate call to action styling to infobar buttons.

BUG=596238

Committed: https://crrev.com/5d04bab763700c788f8084337e70a805be2ffa2d
Cr-Commit-Position: refs/heads/master@{#383545}
==========

[commit-bot: I haz the power, 2016-03-28 19:58:09]

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