Adding a ScrollView for IntentPickerBubbleView

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 c5f007a5ca7746472d5fe094fadf0bfffa071754..83c800039fe3cea380a03decd93a39a55664cc92 100644
--- a/chrome/browser/ui/views/intent_picker_bubble_view.cc
+++ b/chrome/browser/ui/views/intent_picker_bubble_view.cc
@@ -4,8 +4,6 @@
 
 #include "chrome/browser/ui/views/intent_picker_bubble_view.h"
 
-#include <algorithm>
-
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
@@ -18,16 +16,22 @@
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/canvas.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 content {
+class WebContents;
+}  // namespace content
+
 namespace {
 
-// TODO(djacobo): Add a scroll bar and remove kMaxAppResults.
-// Discriminating app results when the list is longer than |kMaxAppResults|
-constexpr size_t kMaxAppResults = 3;
+// 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 kRowHeight = 40;
 constexpr int kMaxWidth = 320;
@@ -39,7 +43,7 @@ constexpr int kLabelImageSeparation = 12;
 
 // UI position wrt the Top Container
 constexpr int kTopContainerMerge = 3;
-constexpr int kAppTagNoneSelected = -1;
+constexpr size_t kAppTagNoneSelected = -1;

[Luis Héctor Chávez, 2016/07/21 21:55:34]

ok, just do an explicit cast: static_cast<size_t>(-1);

[djacobo_, 2016/07/22 01:22:07]

Done.

 
 // 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
@@ -89,6 +93,18 @@ void IntentPickerBubbleView::ShowBubble(
   widget->Show();
 }
 
+// static
+std::unique_ptr<IntentPickerBubbleView>
+IntentPickerBubbleView::CreateBubbleForTesting(
+    const std::vector<NameAndIcon>& app_info,
+    const ThrottleCallback& throttle_cb,
+    content::WebContents* web_contents) {
+  std::unique_ptr<IntentPickerBubbleView> bubble(
+      new IntentPickerBubbleView(app_info, throttle_cb, web_contents));
+  bubble->Init();
+  return bubble;
+}
+
 void IntentPickerBubbleView::Init() {
   SkColor button_text_color = SkColorSetRGB(0x42, 0x85, 0xf4);
 
@@ -128,6 +144,8 @@ void IntentPickerBubbleView::Init() {
   always_button_->SetTextColor(views::Button::STATE_HOVERED, button_text_color);
   always_button_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
   always_button_->SetState(views::Button::STATE_DISABLED);
+  // Insets in the format top, left, bottom, right.

[sky, 2016/07/21 22:57:06]

This comment isn't particularly useful when all the values are the same. You can
also use CreateEmptytBorder(gfx::Insets(16))

[djacobo_, 2016/07/22 01:22:07]

Nice shorthand, applied the same to |just_once_button_|. Done

+  always_button_->SetBorder(views::Border::CreateEmptyBorder(16, 16, 16, 16));
 
   just_once_button_ = new views::LabelButton(
       this, l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_JUST_ONCE));
@@ -142,8 +160,19 @@ void IntentPickerBubbleView::Init() {
   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(16, 16, 16, 16));
+
+  // Adding the |open_with| label to the picker.

[sky, 2016/07/21 22:57:06]

Is this comment relevant here?

[djacobo_, 2016/07/22 01:22:07]

I believe is easy to see what's going on by the code. Removing.

+  header_layout->StartRow(0, 0);
+  AddChildViewAt(header, static_cast<int>(ViewsId::HEADER));

[sky, 2016/07/21 22:57:06]

Based on the name I would expect ViewId to correspond to the View::id(), but
that isn't the case. It's the index of the view in the parent. That's confusing.
AFAICT the reason you're doing that is because you want to get a specific child
on line 323, which even with the enums is error prone. Instead make scroll_view
a member and get rid of ViewId. Much simpler that way.

[djacobo_, 2016/07/22 01:22:07]

You are right and the idea sounds better, modifications in place, thanks!

 
-  for (size_t i = 0; i < std::min(app_info_.size(), kMaxAppResults); ++i) {
+  // Setting a view and a layout for a ScrollView for the apps' list.

[sky, 2016/07/21 22:57:06]

This comment is confusing. I think you mean something like:
// Creates a view to hold the views for each app.

[djacobo_, 2016/07/22 01:22:07]

Oh wow I messed up pretty badly indeed, Done.

+  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);
@@ -152,30 +181,41 @@ void IntentPickerBubbleView::Init() {
     tmp_label->SetMinSize(gfx::Size(kMaxWidth, kRowHeight));
     tmp_label->SetMaxSize(gfx::Size(kMaxWidth, kRowHeight));
     tmp_label->set_tag(i);
-    const gfx::ImageSkia* icon_ = app_info_[i].second.ToImageSkia();
-    tmp_label->SetImage(views::ImageButton::STATE_NORMAL, *icon_);
-    header_layout->StartRow(0, 0);
-    header_layout->AddView(tmp_label);
+    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);
   }
 
-  // Adding the Upper part of the Intent Picker |open_with| label and all the
-  // app options to |this|.
-  header_layout->StartRow(0, 0);
-  header_layout->AddPaddingRow(0, 12);
-  AddChildView(header);
+  views::ScrollView* scroll_view = new views::ScrollView();
+  scroll_view->SetContents(scrollable_view);
+  scroll_view->SetVerticalScrollBar(new views::OverlayScrollBar(false));

[sky, 2016/07/21 22:57:06]

Is it really necessary to create an overlayscrollbar? Assuming yes, add a
comment.

[djacobo_, 2016/07/22 01:22:07]

Yes, I want the ScrollBar for this bubble to be shown only when the user set his
pointer inside of the ScrollView boundaries. Adding a comment.

+  // 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

[sky, 2016/07/21 22:57:06]

I don't understand the *.5. Why not just add 1 as you want 1 extra pixel, right?

[djacobo_, 2016/07/22 01:22:07]

What I was trying to say is, we are required to show *half a row* extra, that's
why we have, height = (kMaxAppResults + 0.5) * kRowHeight; 

Thus is not simply 1 extra pixel, but half a row will be displayed, which is
intended to let the user know more apps are listed by scrolling.

[sky, 2016/07/22 16:32:54]

My mistake, got it.

+  // more than |kMaxAppResults| apps accessible by scrolling the list.
+  if (app_info_.size() <= kMaxAppResults) {
+    scroll_view->ClipHeightTo(kMaxWidth, app_info_.size() * kRowHeight);

[sky, 2016/07/21 22:57:06]

The first argument is the min height, not a width.

[djacobo_, 2016/07/22 01:22:07]

Totally misread the interface, moving to 1 Row at min. Done.

+  } else {
+    scroll_view->ClipHeightTo(kMaxWidth, (kMaxAppResults + 0.5) * kRowHeight);
+  }
+  AddChildViewAt(scroll_view, static_cast<int>(ViewsId::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, 12, 12, kButtonSeparation);
+      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);
+  AddChildViewAt(footer, static_cast<int>(ViewsId::FOOTER));
 }
 
 IntentPickerBubbleView::IntentPickerBubbleView(
@@ -196,6 +236,15 @@ IntentPickerBubbleView::~IntentPickerBubbleView() {
   SetLayoutManager(nullptr);
 }
 
+size_t IntentPickerBubbleView::GetAppInfoSizeForTesting() const {
+  return app_info_.size();
+}
+
+views::LabelButton* IntentPickerBubbleView::GetLabelButtonAtForTesting(
+    size_t index) {
+  return GetLabelButtonAt(index);
+}
+
 // 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) {
@@ -215,24 +264,30 @@ 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;
+      if (!was_callback_run_) {

[sky, 2016/07/21 22:57:06]

Why do you need was_callback_run_?

[djacobo_, 2016/07/22 01:22:07]

I don't want the callback being run twice since that could end in a bad state.

[sky, 2016/07/22 16:32:54]

How can that happen? AFAICT the widget is closed on the next line?

[djacobo_, 2016/07/22 22:37:24]

We can get rid of the if (!was_callback_run_) checkers here, but I still need
the flag so the bubble don't use the callback again on exit (or if the
WebContents gets destroyed calling WebContentsDestroyed().

[sky, 2016/07/22 23:45:56]

That I understand. My question is solely about the need to add the checks in
this function.

[djacobo_, 2016/07/23 00:20:31]

Oh ok, we actually don't need it anymore. The thing is, in a previous version I
was writing testing methods which relied in the fact that a callback should
never be triggered . However now we have a dummy function on the testing class,
which makes the was_callback_run_ flag unnecessary for this part.

+        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;
+      if (!was_callback_run_) {
+        throttle_cb_.Run(
+            selected_app_tag_,
+            arc::ArcNavigationThrottle::CloseReason::JUST_ONCE_PRESSED);
+        was_callback_run_ = true;
+      }
       GetWidget()->Close();
       break;
     default:
-      // TODO(djacobo): Paint the background of the selected button on a
-      // different color, so the user has a clear remainder of his selection.
       // The user cannot click on the |always_button_| or |just_once_button_|
       // unless an explicit app has been selected.
+      if (selected_app_tag_ != kAppTagNoneSelected)
+        PaintLabelButton(selected_app_tag_, SK_ColorWHITE);
       selected_app_tag_ = sender->tag();
+      PaintLabelButton(selected_app_tag_, SkColorSetRGB(0xeb, 0xeb, 0xeb));
       always_button_->SetState(views::Button::STATE_NORMAL);
       just_once_button_->SetState(views::Button::STATE_NORMAL);
   }
@@ -241,8 +296,15 @@ void IntentPickerBubbleView::ButtonPressed(views::Button* sender,
 gfx::Size IntentPickerBubbleView::GetPreferredSize() const {
   gfx::Size ps;
   ps.set_width(kMaxWidth);
-  ps.set_height((std::min(app_info_.size(), kMaxAppResults)) * kRowHeight +
-                kHeaderHeight + kFooterHeight);
+  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);
   return ps;
 }
 
@@ -256,3 +318,17 @@ void IntentPickerBubbleView::WebContentsDestroyed() {
   }
   GetWidget()->Close();
 }
+
+views::LabelButton* IntentPickerBubbleView::GetLabelButtonAt(size_t index) {
+  int scroll_index = static_cast<int>(ViewsId::SCROLL_VIEW);

[Luis Héctor Chávez, 2016/07/21 21:55:34]

Having the |selected_app_tag_| be a huge value is kind of scary. Can you add
bounds-checks in all the places it might be used (either directly or
indirectly)?

If you end up returning nullptr, then you also need nullchecks in all the places
where you call this.

[djacobo_, 2016/07/22 01:22:07]

Done.

+  views::ScrollView* temp_scroll =
+      static_cast<views::ScrollView*>(child_at(scroll_index));
+  views::View* temp_contents = temp_scroll->contents();
+  return static_cast<views::LabelButton*>(temp_contents->child_at(index));
+}
+
+void IntentPickerBubbleView::PaintLabelButton(size_t index, SkColor color) {
+  views::LabelButton* temp_lb = GetLabelButtonAt(index);
+  temp_lb->set_background(views::Background::CreateSolidBackground(color));
+  temp_lb->SchedulePaint();
+}