Reusing Ok/Cancel buttons for intent picker

chrome/browser/ui/views/intent_picker_bubble_view.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/intent_picker_bubble_view.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/grit/generated_resources.h"
 #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_host_view.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/controls/scrollbar/overlay_scroll_bar.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/window/dialog_client_view.h"
 
 namespace {
 
 // Using |kMaxAppResults| as a measure of how many apps we want to show.
 constexpr size_t kMaxAppResults = arc::ArcNavigationThrottle::kMaxAppResults;
 // Main components sizes
+constexpr int kDialogDelegateInsets = 16;
 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
 
+// TODO(djacobo): Look for a better way to implement this, if not possible then
+// use the same dummy here and in the unit_tests.
+// Dummy mouse press event.
+class MousePressedEvent : public ui::Event {

[sky, 2016/10/13 13:35:07]

This seems like a total hack, can you describe why you need this? It seems like
you need it to force the first item to show an icon drop. Is that really
necessary? Assuming it is, can you pass in null? From the description:

  // *** NOTE ***: |event| has been plumbed through on a best effort basis for
  // the purposes of centering ink drop ripples on located Events.  Thus nullptr
  // has been used by clients who do not have an Event instance available to
  // them.

[djacobo_, 2016/10/14 02:35:32]

I modified MarkAsSelected() so we could pass nullptr.  Done

+ public:
+  MousePressedEvent() : Event(ui::ET_MOUSE_PRESSED, base::TimeTicks(), 0) {}
+  ~MousePressedEvent() override {}
+};
+
+// IntentPickerLabelButton
+
+// A button that represents a candidate intent handler.
+class IntentPickerLabelButton : public views::LabelButton {
+ public:
+  IntentPickerLabelButton(views::ButtonListener* listener,
+                          gfx::Image* icon,
+                          const std::string& package_name,
+                          const std::string& activity_name)
+      : LabelButton(listener,
+                    base::UTF8ToUTF16(base::StringPiece(activity_name))),
+        package_name_(package_name) {
+    SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    SetFocusBehavior(View::FocusBehavior::ALWAYS);
+    SetMinSize(gfx::Size(kMaxWidth, kRowHeight));
+    SetInkDropMode(InkDropMode::ON);
+    if (!icon->IsEmpty()) {

[sky, 2016/10/13 13:35:07]

no {}

[djacobo_, 2016/10/14 02:35:31]

Done.

+      SetImage(views::ImageButton::STATE_NORMAL, *icon->ToImageSkia());
+    }
+    SetBorder(views::Border::CreateEmptyBorder(10, 16, 10, 0));
+  }
+
+  SkColor GetInkDropBaseColor() const override { return SK_ColorBLACK; }
+
+  void MarkAsUnselected(const ui::Event& event) {
+    AnimateInkDrop(views::InkDropState::DEACTIVATED,
+                   ui::LocatedEvent::FromIfValid(&event));
+  }
+
+  void MarkAsSelected(const ui::Event& event) {
+    AnimateInkDrop(views::InkDropState::ACTIVATED,
+                   ui::LocatedEvent::FromIfValid(&event));
+  }
+
+  views::InkDropState GetTargetInkDropState() {
+    return ink_drop()->GetTargetInkDropState();
+  }
+
+ private:
+  std::string package_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(IntentPickerLabelButton);
+};
+
 // static
 void IntentPickerBubbleView::ShowBubble(
     content::WebContents* web_contents,
-    const std::vector<NameAndIcon>& app_info,
-    const ThrottleCallback& throttle_cb) {
+    const std::vector<AppInfo>& app_info,
+    const IntentPickerResponse& intent_picker_cb) {
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
-  if (!browser) {
-    throttle_cb.Run(kAppTagNoneSelected,
-                    arc::ArcNavigationThrottle::CloseReason::ERROR);
+  if (!browser || !BrowserView::GetBrowserViewForBrowser(browser)) {
+    intent_picker_cb.Run("" /* Invalid package name */,

[sky, 2016/10/13 13:35:07]

"" -> std::string() unless somewhere defines a constant as empty string for
invalid.

[djacobo_, 2016/10/14 02:35:31]

Done.

+                         arc::ArcNavigationThrottle::CloseReason::ERROR);
     return;
   }
   BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
-  if (!browser_view) {
-    throttle_cb.Run(kAppTagNoneSelected,
-                    arc::ArcNavigationThrottle::CloseReason::ERROR);
-    return;
-  }
-
   IntentPickerBubbleView* delegate =
-      new IntentPickerBubbleView(app_info, throttle_cb, web_contents);
-  delegate->set_margins(gfx::Insets());
+      new IntentPickerBubbleView(app_info, intent_picker_cb, web_contents);
+  delegate->set_margins(gfx::Insets(16, 0, 0, 0));
   delegate->set_parent_window(browser_view->GetNativeWindow());
   views::Widget* widget =
       views::BubbleDialogDelegateView::CreateBubble(delegate);
 
   delegate->SetArrowPaintType(views::BubbleBorder::PAINT_NONE);
   delegate->SetAlignment(views::BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
 
   // Using the TopContainerBoundsInScreen Rect to specify an anchor for the the
   // UI. Rect allow us to set the coordinates(x,y), the width and height for the
   // new Rectangle.
   delegate->SetAnchorRect(
       gfx::Rect(browser_view->GetTopContainerBoundsInScreen().x(),
                 browser_view->GetTopContainerBoundsInScreen().y(),
                 browser_view->GetTopContainerBoundsInScreen().width(),
                 browser_view->GetTopContainerBoundsInScreen().height() -
                     kTopContainerMerge));
   widget->Show();

[sky, 2016/10/13 13:35:07]

Why do you show first, then change a bunch of stuff and trigger layout?

[djacobo_, 2016/10/14 02:35:31]

That was a mistake, for a minute I thought Init() was called right before
Show(), but that function is tied to CreateBubbel(). Done

+  delegate->GetDialogClientView()->set_button_row_insets(
+      gfx::Insets(kDialogDelegateInsets));
+  delegate->GetDialogClientView()->Layout();
+  delegate->GetIntentPickerLabelButtonAt(0)->MarkAsSelected(
+      MousePressedEvent());
 }
 
 // static
 std::unique_ptr<IntentPickerBubbleView>
 IntentPickerBubbleView::CreateBubbleView(
-    const std::vector<NameAndIcon>& app_info,
-    const ThrottleCallback& throttle_cb,
+    const std::vector<AppInfo>& app_info,
+    const IntentPickerResponse& intent_picker_cb,
     content::WebContents* web_contents) {
   std::unique_ptr<IntentPickerBubbleView> bubble(
-      new IntentPickerBubbleView(app_info, throttle_cb, web_contents));
+      new IntentPickerBubbleView(app_info, intent_picker_cb, web_contents));
   bubble->Init();
   return bubble;
 }
 
+bool IntentPickerBubbleView::Accept() {
+  if (!intent_picker_cb_.is_null()) {
+    auto callback = intent_picker_cb_;

[sky, 2016/10/13 13:35:07]

It's worth commenting as to why you do this rather than using intent_picker_cb_
directly.

[djacobo_, 2016/10/14 02:35:31]

Adding a comment to RunCallback(), method that now concentrates repeated code
for Running and Reseting the callback. Done.

+    intent_picker_cb_.Reset();
+    callback.Run(app_info_[selected_app_tag_].package_name,
+                 arc::ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED);
+  }
+  return true;
+}
+
+bool IntentPickerBubbleView::Cancel() {
+  if (!intent_picker_cb_.is_null()) {

[sky, 2016/10/13 13:35:07]

This close is nearly the same as the accept/cancel/close. Please refactor to a
common function so that you aren't duplicating the logic in three places.

[djacobo_, 2016/10/14 02:35:31]

Done.

+    auto callback = intent_picker_cb_;
+    intent_picker_cb_.Reset();
+    callback.Run(app_info_[selected_app_tag_].package_name,
+                 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 (!intent_picker_cb_.is_null()) {
+    auto callback = intent_picker_cb_;
+    intent_picker_cb_.Reset();
+    callback.Run("" /* Invalid package name */,

[sky, 2016/10/13 13:35:07]

std::string(). But as you have this in a couple of places please use a constant
for invalid package.

[djacobo_, 2016/10/14 02:35:31]

Done.

+                 arc::ArcNavigationThrottle::CloseReason::DIALOG_DEACTIVATED);
+  }
+  return true;
+}
+
+int IntentPickerBubbleView::GetDefaultDialogButton() const {
+  return ui::DIALOG_BUTTON_OK;
+}
+
 void IntentPickerBubbleView::Init() {
-  SkColor button_text_color = SkColorSetRGB(0x42, 0x85, 0xf4);
-
   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(
-        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,
-                          *app_info_[i].second.ToImageSkia());
-    }
-    tmp_label->SetBorder(views::Border::CreateEmptyBorder(10, 16, 10, 0));
-    scrollable_view->AddChildViewAt(tmp_label, i);
+    IntentPickerLabelButton* app_button = new IntentPickerLabelButton(
+        this, &app_info_[i].icon, app_info_[i].package_name,
+        app_info_[i].activity_name);
+    app_button->set_tag(i);
+    scrollable_view->AddChildViewAt(app_button, i);
   }
 
   scroll_view_ = new views::ScrollView();
   scroll_view_->SetContents(scrollable_view);
   // Setting a customized ScrollBar which is shown only when the mouse pointer
   // is inside the ScrollView.
   scroll_view_->SetVerticalScrollBar(new views::OverlayScrollBar(false));
   // This part gives the scroll a fixed width and height. The height depends on
   // how many app candidates we got and how many we actually want to show.
   // The added 0.5 on the else block allow us to let the user know there are
   // more than |kMaxAppResults| apps accessible by scrolling the list.
   if (app_info_.size() <= kMaxAppResults) {
     scroll_view_->ClipHeightTo(kRowHeight, app_info_.size() * kRowHeight);
   } else {
     scroll_view_->ClipHeightTo(kRowHeight, (kMaxAppResults + 0.5) * kRowHeight);
   }
   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);
+  // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child

[sky, 2016/10/13 13:35:07]

Can you clarify what behavior you are seeing that is bad? I could easily see you
needing to turn on a layer for the viewport of the scrollview and make it clip
children.

[djacobo_, 2016/10/14 02:35:31]

The problem I was observing is, suppose you select the top-most row on the
scroll view and scroll so the previously highlighted row goes outside of the
visible region (provided there are enough rows). Of course the row shouldn't be
visible, but I was able to still see the highlight and icon floating outside of
the scrollview region. I was told these instructions would fix that (and they
do) but I am not very knowledgeable about why or if this is actually a proper
solution. wdyt?

[bruthig, 2016/10/14 14:08:53]

It's been a while since I looked at this, but from what I can remember I think
we weren't able to turn layer based scrolling on easily because of the feature
list check here:
https://cs.chromium.org/chromium/src/ui/views/controls/scroll_view.cc?rcl=0&l...

[sky, 2016/10/14 16:08:24]

I'm surprised your fix works. I would expect the children to show ontop of the
scrollview border. Is that not the case? The problem would be more apparent if
'this' had another child. As I said, the real fix is to turn on the layer of the
viewport. The code Ben points at below is what you want. A fix I'm fine with for
the short term is to copy/paste it here. The real fix is for scrollview to
detect a child has a layer and automatically turn on a layer for the viewport.

[bruthig, 2016/10/14 16:49:51]

To be clear here, and hopefully helpful :), this would need to check recursively
if any immediate, or non immediate child view was painting to a layer.

[djacobo_, 2016/10/14 22:30:46]

So the quick fix would be to reuse the part of the code that is never reached in
ScrollView, this is:

 background_color_ = SK_ColorWHITE;
  contents_viewport_->set_background(
      Background::CreateSolidBackground(background_color_));
  contents_viewport_->SetPaintToLayer(true);
  contents_viewport_->layer()->SetMasksToBounds(true);

Since I would prefer not to expose |contents_viewport_| I included an interface
for this as ActivateLayer(bool), it works as expected. 

I am not completely sure about how to implement what you suggest Ben, what I
understand is to check for layer()s within the |contents_viewport_| subtree,
this of course should be called after all the contents are added to the
ScrollView. In theory then, I should be able to call ActivateLayer(false) after
all the labels are added to the ScrollView? Do I need to update anything else
within the ScrollView's children?

[bruthig, 2016/10/17 13:55:42]

Sorry for my poor wording, but that is what I meant.  You should check the
entire |contents_viewport_| subtree and not just |contents_viewport_|'s
immediate children.

[djacobo_, 2016/10/17 17:11:25]

let me give it another shot in the separated bug. Ack.

+  // view layers when the Ink Drop is active.
+  SetPaintToLayer(true);
+  layer()->SetMasksToBounds(true);
+  layer()->SetFillsBoundsOpaquely(false);
+}
 
-  buttons_layout->set_main_axis_alignment(
-      views::BoxLayout::MAIN_AXIS_ALIGNMENT_END);
-  footer->AddChildView(just_once_button_);
-  footer->AddChildView(always_button_);
-  AddChildView(footer);
+base::string16 IntentPickerBubbleView::GetWindowTitle() const {
+  return l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_OPEN_WITH);
+}
+
+base::string16 IntentPickerBubbleView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  return l10n_util::GetStringUTF16(button == ui::DIALOG_BUTTON_OK
+                                       ? IDS_INTENT_PICKER_BUBBLE_VIEW_JUST_ONCE
+                                       : IDS_INTENT_PICKER_BUBBLE_VIEW_ALWAYS);
 }
 
 IntentPickerBubbleView::IntentPickerBubbleView(
-    const std::vector<NameAndIcon>& app_info,
-    ThrottleCallback throttle_cb,
+    const std::vector<AppInfo>& app_info,
+    IntentPickerResponse intent_picker_cb,
     content::WebContents* web_contents)
     : views::BubbleDialogDelegateView(nullptr /* anchor_view */,
                                       views::BubbleBorder::TOP_CENTER),
       WebContentsObserver(web_contents),
-      was_callback_run_(false),
-      throttle_cb_(throttle_cb),
-      selected_app_tag_(kAppTagNoneSelected),
-      always_button_(nullptr),
-      just_once_button_(nullptr),
+      intent_picker_cb_(intent_picker_cb),
+      selected_app_tag_(0),
       scroll_view_(nullptr),
       app_info_(app_info) {}
 
 IntentPickerBubbleView::~IntentPickerBubbleView() {
   SetLayoutManager(nullptr);
 }
 
 // If the widget gets closed without an app being selected we still need to use
 // the callback so the caller can Resume the navigation.
 void IntentPickerBubbleView::OnWidgetDestroying(views::Widget* widget) {
-  if (!was_callback_run_) {
-    was_callback_run_ = true;
-    throttle_cb_.Run(
-        kAppTagNoneSelected,
-        arc::ArcNavigationThrottle::CloseReason::DIALOG_DEACTIVATED);
+  if (!intent_picker_cb_.is_null()) {
+    auto callback = intent_picker_cb_;
+    intent_picker_cb_.Reset();
+    callback.Run("" /* Invalid package name */,
+                 arc::ArcNavigationThrottle::CloseReason::DIALOG_DEACTIVATED);
   }
 }
 
-int IntentPickerBubbleView::GetDialogButtons() const {
-  return ui::DIALOG_BUTTON_NONE;
-}
-
 void IntentPickerBubbleView::ButtonPressed(views::Button* sender,
                                            const ui::Event& event) {
-  switch (sender->tag()) {
-    case static_cast<int>(Option::ALWAYS):
-      was_callback_run_ = true;
-      throttle_cb_.Run(selected_app_tag_,
-                       arc::ArcNavigationThrottle::CloseReason::ALWAYS_PRESSED);
-      GetWidget()->Close();
-      break;
-    case static_cast<int>(Option::JUST_ONCE):
-      was_callback_run_ = true;
-      throttle_cb_.Run(
-          selected_app_tag_,
-          arc::ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED);
-      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_LT(static_cast<size_t>(sender->tag()), app_info_.size());
+  GetIntentPickerLabelButtonAt(selected_app_tag_)->MarkAsUnselected(event);
+
+  selected_app_tag_ = sender->tag();
+  GetIntentPickerLabelButtonAt(selected_app_tag_)->MarkAsSelected(event);
 }
 
 gfx::Size IntentPickerBubbleView::GetPreferredSize() const {
   gfx::Size ps;
   ps.set_width(kMaxWidth);
   int apps_height = app_info_.size();
   // We are showing |kMaxAppResults| + 0.5 rows at max, the extra 0.5 is used so
   // the user can notice that more options are available.
   if (app_info_.size() > kMaxAppResults) {
     apps_height = (kMaxAppResults + 0.5) * kRowHeight;
   } else {
     apps_height *= kRowHeight;
   }
-  ps.set_height(apps_height + kHeaderHeight + kFooterHeight);
+  ps.set_height(apps_height + kDialogDelegateInsets);
   return ps;
 }
 
 // If the actual web_contents gets destroyed in the middle of the process we
 // should inform the caller about this error.
 void IntentPickerBubbleView::WebContentsDestroyed() {
-  if (!was_callback_run_) {
-    was_callback_run_ = true;
-    throttle_cb_.Run(kAppTagNoneSelected,
-                     arc::ArcNavigationThrottle::CloseReason::ERROR);
+  if (!intent_picker_cb_.is_null()) {

[sky, 2016/10/13 13:35:07]

Refactor this to the common place too. That said, is this really necessary?
Doesn't the Close() function you're now overriding cover it?

[djacobo_, 2016/10/14 02:35:31]

Refactored, but WebContentsDestroyed() is still needed, I did test destroying
the WebContents form outside of this class and the picker didn't close and
respond to the caller as we do with this method. Done

[sky, 2016/10/14 16:08:24]

Sorry if I wasn't clear. You definitely want the WebContentsDestroyed()
override, but shouldn't the GetWidget()->Close() call in this function trigger
IntentPickerBubbleView::Close(), so that you don't need to call to the callback
here?

[djacobo_, 2016/10/14 22:30:46]

Done.

+    auto callback = intent_picker_cb_;
+    intent_picker_cb_.Reset();
+    callback.Run("" /* Invalid package name */,
+                 arc::ArcNavigationThrottle::CloseReason::ERROR);
   }
   GetWidget()->Close();
 }
 
-views::LabelButton* IntentPickerBubbleView::GetLabelButtonAt(size_t index) {
+IntentPickerLabelButton* IntentPickerBubbleView::GetIntentPickerLabelButtonAt(
+    size_t index) {
   views::View* temp_contents = scroll_view_->contents();
-  return static_cast<views::LabelButton*>(temp_contents->child_at(index));
+  return static_cast<IntentPickerLabelButton*>(temp_contents->child_at(index));
 }
 
-void IntentPickerBubbleView::SetLabelButtonBackgroundColor(size_t index,
-                                                           SkColor color) {
-  views::LabelButton* temp_lb = GetLabelButtonAt(index);
-  temp_lb->set_background(views::Background::CreateSolidBackground(color));
-  temp_lb->SchedulePaint();
+gfx::ImageSkia IntentPickerBubbleView::GetAppImageForTesting(size_t index) {
+  return GetIntentPickerLabelButtonAt(index)->GetImage(
+      views::Button::ButtonState::STATE_NORMAL);
 }
+
+views::InkDropState IntentPickerBubbleView::GetInkDropStateForTesting(
+    size_t index) {
+  return GetIntentPickerLabelButtonAt(index)->GetTargetInkDropState();
+}
+
+void IntentPickerBubbleView::PressButtonForTesting(size_t index,
+                                                   const ui::Event& event) {
+  views::Button* button =
+      static_cast<views::Button*>(GetIntentPickerLabelButtonAt(index));
+  ButtonPressed(button, event);
+}