Reusing Ok/Cancel buttons for intent picker

chrome/browser/ui/views/intent_picker_bubble_view.cc

Index: chrome/browser/ui/views/intent_picker_bubble_view.cc
diff --git a/chrome/browser/ui/views/intent_picker_bubble_view.cc b/chrome/browser/ui/views/intent_picker_bubble_view.cc
index 0d46d817d4822b33e4f3f64474e5f87bd3a8bc7d..1ac1eafc384c073c598151b2f4bf1af26faa203f 100644
--- a/chrome/browser/ui/views/intent_picker_bubble_view.cc
+++ b/chrome/browser/ui/views/intent_picker_bubble_view.cc
@@ -15,7 +15,9 @@
 #include "content/public/browser/navigation_handle.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/material_design/material_design_controller.h"
 #include "ui/gfx/canvas.h"
+#include "ui/views/animation/ink_drop_highlight.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/scroll_view.h"
@@ -31,23 +33,40 @@ constexpr size_t kMaxAppResults = arc::ArcNavigationThrottle::kMaxAppResults;
 // Main components sizes
 constexpr int kRowHeight = 40;
 constexpr int kMaxWidth = 320;
-constexpr int kHeaderHeight = 60;
-constexpr int kFooterHeight = 68;
 // Inter components padding
-constexpr int kButtonSeparation = 8;
 constexpr int kLabelImageSeparation = 12;
 
 // UI position wrt the Top Container
 constexpr int kTopContainerMerge = 3;
 constexpr size_t kAppTagNoneSelected = static_cast<size_t>(-1);
 
-// Arbitrary negative values to use as tags on the |always_button_| and
-// |just_once_button_|. These are negative to differentiate from the app's tags
-// which are always >= 0.
-enum class Option : int { ALWAYS = -2, JUST_ONCE };
-
 }  // namespace
 
+// IntentPickerMenuButton -----------------------------------------------------
+
+// This is a convenience class for the buttons on the intent picker so we can
+// manipulate the internal hovering behavior.
+
+class IntentPickerMenuButton : public views::LabelButton {

[Evan Stade, 2016/08/18 05:45:41]

I don't believe this class is necessary. You could just create a LabelButton and
set the properties you want. For the ink drop color, set_ink_drop_base_color
should work.

[djacobo_, 2016/08/24 22:18:35]

Hmm, that is true for all but SetInkDropMode(), it is a protected method and I
have seen it called only on classes that directly or indirectly inherit from
views::CustomButton. is there a public accessory method for this? I am guessing
something like

LabelButton* button = ...;
button->set_ink_drop_mode();

Or does this means we will not have hovering effects in the meantime?

[Evan Stade, 2016/08/26 22:44:23]

I don't know if there's a good reason for SetInkDropMode to be protected.
+bruthig

[bruthig, 2016/08/30 19:16:36]

It should be ok to promote InkDropHostView::SetInkDropMode from protected to
public.

If you do this can you remove the accessor in ink_drop_host_view_test_api as
well?

[djacobo_, 2016/09/06 23:21:23]

Done, change here https://codereview.chromium.org/2289403002/.

[bruthig, 2016/09/07 14:58:43]

Is this needed anymore?

[djacobo_, 2016/09/08 00:10:30]

Not really since I can already access SetInkDropMode with this inner class, up
to you guys whether you want that interface public or not.

[bruthig, 2016/09/08 15:13:12]

Ok, let's abandon that other change then.

[djacobo_, 2016/09/08 21:27:06]

Done.

+ public:
+  IntentPickerMenuButton(views::ButtonListener* listener,
+                         const base::string16& title)
+      : LabelButton(listener, title) {
+    SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    SetFocusBehavior(View::FocusBehavior::ALWAYS);
+    SetImageLabelSpacing(kLabelImageSeparation);
+    if (ui::MaterialDesignController::IsModeMaterial()) {

[Evan Stade, 2016/08/18 05:45:41]

this isn't a useful check because it always returns true. You could replace it
with IsSecondaryUiMaterial() but I take it you want the MD look before it's
launched to other dialogs.

[djacobo_, 2016/08/24 22:18:35]

I am removing this whole inner-class, done.

+      SetInkDropMode(InkDropMode::ON);
+      set_has_ink_drop_action_on_click(true);
+    }
+  }
+
+  SkColor GetInkDropBaseColor() const override { return SK_ColorBLACK; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(IntentPickerMenuButton);
+};
+
 // static
 void IntentPickerBubbleView::ShowBubble(
     content::NavigationHandle* handle,
@@ -86,6 +105,10 @@ void IntentPickerBubbleView::ShowBubble(
                 browser_view->GetTopContainerBoundsInScreen().width(),
                 browser_view->GetTopContainerBoundsInScreen().height() -
                     kTopContainerMerge));
+  widget->client_view()->AsDialogClientView()->ok_button()->SetState(

[Evan Stade, 2016/08/18 05:45:41]

use IsDialogButtonEnabled()

[djacobo_, 2016/08/24 22:18:35]

Awesome, done.

+      views::Button::STATE_DISABLED);
+  widget->client_view()->AsDialogClientView()->cancel_button()->SetState(
+      views::Button::STATE_DISABLED);
   widget->Show();
 }
 
@@ -101,79 +124,57 @@ IntentPickerBubbleView::CreateBubbleView(
   return bubble;
 }
 
-void IntentPickerBubbleView::Init() {
-  SkColor button_text_color = SkColorSetRGB(0x42, 0x85, 0xf4);
+bool IntentPickerBubbleView::Accept() {
+  // Internally this button uses the |JUST ONCE| logic.

[Evan Stade, 2016/08/18 05:45:41]

I don't understand this comment and I don't think it's necessary. Ditto below.

[djacobo_, 2016/08/24 22:18:35]

Done.

+  if (!was_callback_run_) {
+    was_callback_run_ = true;

[Evan Stade, 2016/08/18 05:45:41]

instead of adding was_callback_run_, you can use Reset() and is_null()

[djacobo_, 2016/08/24 22:18:35]

I did use something like:
if (!throttle_cb_.is_null()) {
    throttle_cb_.Run(params);
    throttle_cb_.Reset();
}

and got a crash, poked with the code for a while and this way Accept() and
Cancel() were trying to execute throttle_cb_.Run() (yes, did the same change to
both methods). So ATM I would like to stick with this implementation if you
don't mind, I could investigate further next if you want.

[djacobo_, 2016/08/24 22:41:04]

Arg, I didn't undo the changes, my bad reverting this part.

[Evan Stade, 2016/08/26 22:44:23]

Yes, please figure out why it's crashing. If it's because running the callback
somehow tries to close this dialog, that's probably something work fixing.

[djacobo_, 2016/08/30 01:33:21]

Somehow Reset()-ing the callback after actually Run()-ing it let the
ArcNavigationThrottle in a bad state, the issue was observed on another UI which
can be displayed before this one under certain scenarios (it's an incognito
warning).

Using the same approach here and on the other class. (adding you as a reviewer
so you can see what's going on).

+    throttle_cb_.Run(
+        selected_app_tag_,
+        arc::ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED);
+  }
+  return true;
+}
+
+bool IntentPickerBubbleView::Cancel() {
+  // Internally this button uses the |ALWAYS| logic.
+  if (!was_callback_run_) {
+    was_callback_run_ = true;
+    throttle_cb_.Run(selected_app_tag_,
+                     arc::ArcNavigationThrottle::CloseReason::ALWAYS_PRESSED);
+  }
+  return true;
+}
 
+bool IntentPickerBubbleView::Close() {
+  // Whenever closing the bubble without pressing |JUST ONCE| or |ALWAYS| we
+  // need to report back that the user didn't select anything.
+  if (!was_callback_run_) {
+    was_callback_run_ = true;
+    throttle_cb_.Run(
+        kAppTagNoneSelected,
+        arc::ArcNavigationThrottle::CloseReason::DIALOG_DEACTIVATED);
+  }
+  return true;
+}
+
+int IntentPickerBubbleView::GetDefaultDialogButton() const {
+  return ui::DIALOG_BUTTON_NONE;
+}
+
+void IntentPickerBubbleView::Init() {
   views::BoxLayout* general_layout =
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0);
   SetLayoutManager(general_layout);
 
-  // Create a view for the upper part of the UI, managed by a GridLayout to
-  // allow precise padding.
-  View* header = new View();
-  views::GridLayout* header_layout = new views::GridLayout(header);
-  header->SetLayoutManager(header_layout);
-  views::Label* open_with = new views::Label(
-      l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_OPEN_WITH));
-  open_with->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  open_with->SetFontList(gfx::FontList("Roboto-medium, 15px"));
-
-  views::ColumnSet* column_set = header_layout->AddColumnSet(0);
-  column_set->AddPaddingColumn(0, 16);
-  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
-                        views::GridLayout::FIXED, kMaxWidth, kMaxWidth);
-  column_set->AddPaddingColumn(0, 16);
-  // Tell the GridLayout where to start, then proceed to place the views.
-  header_layout->AddPaddingRow(0.0, 21);
-  header_layout->StartRow(0, 0);
-  header_layout->AddView(open_with);
-  header_layout->AddPaddingRow(0.0, 24);
-
-  always_button_ = new views::LabelButton(
-      this, l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_ALWAYS));
-  always_button_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
-  always_button_->SetFontList(gfx::FontList("Roboto-medium, 13px"));
-  always_button_->set_tag(static_cast<int>(Option::ALWAYS));
-  always_button_->SetMinSize(gfx::Size(80, 32));
-  always_button_->SetTextColor(views::Button::STATE_DISABLED, SK_ColorGRAY);
-  always_button_->SetTextColor(views::Button::STATE_NORMAL, button_text_color);
-  always_button_->SetTextColor(views::Button::STATE_HOVERED, button_text_color);
-  always_button_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
-  always_button_->SetState(views::Button::STATE_DISABLED);
-  always_button_->SetBorder(views::Border::CreateEmptyBorder(gfx::Insets(16)));
-
-  just_once_button_ = new views::LabelButton(
-      this, l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_JUST_ONCE));
-  just_once_button_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
-  just_once_button_->SetFontList(gfx::FontList("Roboto-medium, 13px"));
-  just_once_button_->set_tag(static_cast<int>(Option::JUST_ONCE));
-  just_once_button_->SetMinSize(gfx::Size(80, 32));
-  just_once_button_->SetState(views::Button::STATE_DISABLED);
-  just_once_button_->SetTextColor(views::Button::STATE_DISABLED, SK_ColorGRAY);
-  just_once_button_->SetTextColor(views::Button::STATE_NORMAL,
-                                  button_text_color);
-  just_once_button_->SetTextColor(views::Button::STATE_HOVERED,
-                                  button_text_color);
-  just_once_button_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
-  just_once_button_->SetBorder(
-      views::Border::CreateEmptyBorder(gfx::Insets(16)));
-
-  header_layout->StartRow(0, 0);
-  AddChildView(header);
-
   // Creates a view to hold the views for each app.
   views::View* scrollable_view = new views::View();
   views::BoxLayout* scrollable_layout =
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0);
   scrollable_view->SetLayoutManager(scrollable_layout);
   for (size_t i = 0; i < app_info_.size(); ++i) {
-    views::LabelButton* tmp_label = new views::LabelButton(
+    IntentPickerMenuButton* tmp_label = new IntentPickerMenuButton(

[Evan Stade, 2016/08/18 05:45:41]

I don't think tmp_ is a good prefix for local variable names

[djacobo_, 2016/08/24 22:18:35]

Done.

         this, base::UTF8ToUTF16(base::StringPiece(app_info_[i].first)));
-    tmp_label->SetFocusBehavior(View::FocusBehavior::ALWAYS);
-    tmp_label->SetFontList(gfx::FontList("Roboto-regular, 13px"));
-    tmp_label->SetImageLabelSpacing(kLabelImageSeparation);
     tmp_label->SetMinSize(gfx::Size(kMaxWidth, kRowHeight));
-    tmp_label->SetMaxSize(gfx::Size(kMaxWidth, kRowHeight));
     tmp_label->set_tag(i);
     if (!app_info_[i].second.IsEmpty()) {
       tmp_label->SetImage(views::ImageButton::STATE_NORMAL,
@@ -199,19 +200,22 @@ void IntentPickerBubbleView::Init() {
   }
   AddChildView(scroll_view_);
 
-  // The lower part of the Picker contains only the 2 buttons
-  // |just_once_button_| and |always_button_|.
-  View* footer = new View();
-  footer->SetBorder(views::Border::CreateEmptyBorder(24, 0, 12, 12));
-  views::BoxLayout* buttons_layout = new views::BoxLayout(
-      views::BoxLayout::kHorizontal, 0, 0, kButtonSeparation);
-  footer->SetLayoutManager(buttons_layout);
-
-  buttons_layout->set_main_axis_alignment(
-      views::BoxLayout::MAIN_AXIS_ALIGNMENT_END);
-  footer->AddChildView(just_once_button_);
-  footer->AddChildView(always_button_);
-  AddChildView(footer);
+  if (ui::MaterialDesignController::IsModeMaterial()) {
+    SetPaintToLayer(true);
+    layer()->SetMasksToBounds(true);
+    layer()->SetFillsBoundsOpaquely(false);
+  }
+}
+
+base::string16 IntentPickerBubbleView::GetWindowTitle() const {
+  return l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_OPEN_WITH);
+}
+
+base::string16 IntentPickerBubbleView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  if (button == ui::DIALOG_BUTTON_OK)
+    return l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_JUST_ONCE);
+  return l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_ALWAYS);
 }
 
 IntentPickerBubbleView::IntentPickerBubbleView(
@@ -224,8 +228,6 @@ IntentPickerBubbleView::IntentPickerBubbleView(
       was_callback_run_(false),
       throttle_cb_(throttle_cb),
       selected_app_tag_(kAppTagNoneSelected),
-      always_button_(nullptr),
-      just_once_button_(nullptr),
       scroll_view_(nullptr),
       app_info_(app_info) {}
 
@@ -245,38 +247,22 @@ void IntentPickerBubbleView::OnWidgetDestroying(views::Widget* widget) {
 }
 
 int IntentPickerBubbleView::GetDialogButtons() const {
-  return ui::DIALOG_BUTTON_NONE;
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;

[Evan Stade, 2016/08/18 05:45:41]

this is the default

[djacobo_, 2016/08/24 22:18:35]

Done.

 }
 
 void IntentPickerBubbleView::ButtonPressed(views::Button* sender,
                                            const ui::Event& event) {
-  switch (sender->tag()) {
-    case static_cast<int>(Option::ALWAYS):
-      throttle_cb_.Run(selected_app_tag_,
-                       arc::ArcNavigationThrottle::CloseReason::ALWAYS_PRESSED);
-      was_callback_run_ = true;
-      GetWidget()->Close();
-      break;
-    case static_cast<int>(Option::JUST_ONCE):
-      throttle_cb_.Run(
-          selected_app_tag_,
-          arc::ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED);
-      was_callback_run_ = true;
-      GetWidget()->Close();
-      break;
-    default:
-      // The selected app must be a value in the range [0, app_info_.size()-1].
-      DCHECK(static_cast<size_t>(sender->tag()) < app_info_.size());
-      // The user cannot click on the |always_button_| or |just_once_button_|
-      // unless an explicit app has been selected.
-      if (selected_app_tag_ != kAppTagNoneSelected)
-        SetLabelButtonBackgroundColor(selected_app_tag_, SK_ColorWHITE);
-      selected_app_tag_ = sender->tag();
-      SetLabelButtonBackgroundColor(selected_app_tag_,
-                                    SkColorSetRGB(0xeb, 0xeb, 0xeb));
-      always_button_->SetState(views::Button::STATE_NORMAL);
-      just_once_button_->SetState(views::Button::STATE_NORMAL);
-  }
+  // The selected app must be a value in the range [0, app_info_.size()-1].
+  DCHECK(static_cast<size_t>(sender->tag()) < app_info_.size());
+  // The user cannot click on the |always_button_| or |just_once_button_|
+  // unless an explicit app has been selected.
+  if (selected_app_tag_ != kAppTagNoneSelected)
+    SetIntentPickerMenuButtonBackgroundColor(selected_app_tag_, SK_ColorWHITE);
+  selected_app_tag_ = sender->tag();
+  SetIntentPickerMenuButtonBackgroundColor(selected_app_tag_,

[Evan Stade, 2016/08/18 05:45:41]

seems like you should be setting the ink drop state to ACTIVATED instead of
setting a bg color (like when you click the app menu button and the menu is
showing)

[djacobo_, 2016/08/24 22:18:35]

If we wanted to use InkDropState, shouldn't we have our own button class and
call AnimateInkDrop() ? I will make this change if you think is necessary, I
just want to know if that's the correct way of implementing it.

[bruthig, 2016/08/30 19:16:36]

I can't speak definitively but I agree with Evan.  If you are going to use an
InkDropHighlight then I think it would make sense to use the ripple to show
pressed/activated state.  I'm not sure if you actually need the ACTIVATED state
but be aware that the LabelButton doesn't make use of the ACTIVATED state.  We
can probably speak offline about this if needed.

[djacobo_, 2016/09/06 23:21:23]

As chatted offline, using a custom class that overrides NotifyClick(), using
InkDropState::HIDDEN and InkDropState::ACTIVATED instead of manually changing
the background color.

+                                           SkColorSetRGB(0xeb, 0xeb, 0xeb));
+  GetDialogClientView()->ok_button()->SetState(views::Button::STATE_NORMAL);

[Evan Stade, 2016/08/18 05:45:41]

GetDialogClientView()->UpdateDialogButtons();

[djacobo_, 2016/08/24 22:18:35]

Done.

+  GetDialogClientView()->cancel_button()->SetState(views::Button::STATE_NORMAL);
 }
 
 gfx::Size IntentPickerBubbleView::GetPreferredSize() const {
@@ -290,7 +276,7 @@ gfx::Size IntentPickerBubbleView::GetPreferredSize() const {
   } else {
     apps_height *= kRowHeight;
   }
-  ps.set_height(apps_height + kHeaderHeight + kFooterHeight);
+  ps.set_height(apps_height);
   return ps;
 }
 
@@ -305,14 +291,22 @@ void IntentPickerBubbleView::WebContentsDestroyed() {
   GetWidget()->Close();
 }
 
-views::LabelButton* IntentPickerBubbleView::GetLabelButtonAt(size_t index) {
+IntentPickerMenuButton* IntentPickerBubbleView::GetIntentPickerMenuButtonAt(
+    size_t index) {
   views::View* temp_contents = scroll_view_->contents();
-  return static_cast<views::LabelButton*>(temp_contents->child_at(index));
+  return static_cast<IntentPickerMenuButton*>(temp_contents->child_at(index));
 }
 
-void IntentPickerBubbleView::SetLabelButtonBackgroundColor(size_t index,
-                                                           SkColor color) {
-  views::LabelButton* temp_lb = GetLabelButtonAt(index);
+void IntentPickerBubbleView::SetIntentPickerMenuButtonBackgroundColor(
+    size_t index,
+    SkColor color) {
+  IntentPickerMenuButton* temp_lb = GetIntentPickerMenuButtonAt(index);

[Evan Stade, 2016/08/18 05:45:41]

s/temp_lb/button

[djacobo_, 2016/08/24 22:18:35]

Done.

   temp_lb->set_background(views::Background::CreateSolidBackground(color));
   temp_lb->SchedulePaint();
 }
+
+// Method for testing, this way we don't need to expose IntentPickerMenuButton
+gfx::ImageSkia IntentPickerBubbleView::GetAppImageForTesting(size_t index) {
+  return GetIntentPickerMenuButtonAt(index)->GetImage(
+      views::Button::ButtonState::STATE_NORMAL);
+}