Adding a ScrollView for IntentPickerBubbleView

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 <algorithm>
-
 #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/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;
 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 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
 // which are always >= 0.
 enum class Option : int { ALWAYS = -2, JUST_ONCE };
 
 }  // namespace
 
 // static
 void IntentPickerBubbleView::ShowBubble(
@@ skipping 29 matching lines @@
   // 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();
 }
 
+// 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);
 
   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();
@@ skipping 19 matching lines @@
       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);
+  // 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));
   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(16, 16, 16, 16));
 
-  for (size_t i = 0; i < std::min(app_info_.size(), kMaxAppResults); ++i) {
+  // 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!

+
+  // 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);
     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);
-    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(
     const std::vector<NameAndIcon>& app_info,
     ThrottleCallback throttle_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),
       app_info_(app_info) {}
 
 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) {
   if (!was_callback_run_) {
     throttle_cb_.Run(
         kAppTagNoneSelected,
         arc::ArcNavigationThrottle::CloseReason::DIALOG_DEACTIVATED);
     was_callback_run_ = true;
   }
 }
 
 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):
-      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);
   }
 }
 
 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;
 }
 
 // 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_) {
     throttle_cb_.Run(kAppTagNoneSelected,
                      arc::ArcNavigationThrottle::CloseReason::ERROR);
     was_callback_run_ = true;
   }
   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();
+}