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 {
 
-// TODO(djacobo): Add a scroll bar and remove kMaxAppResults.
-// Discriminating app results when the list is longer than |kMaxAppResults|
+// Using |kMaxAppResults| as a measure of how many apps we want to show.
 constexpr size_t kMaxAppResults = 3;

[Yusuke Sato, 2016/07/20 08:05:54]

Please rebase this CL and use the throttle's.

[djacobo_, 2016/07/20 21:18:30]

Done.

 // 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;
 
 // 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(
     content::NavigationHandle* handle,
     const std::vector<NameAndIcon>& app_info,
     const ThrottleCallback& throttle_cb) {
   Browser* browser =
       chrome::FindBrowserWithWebContents(handle->GetWebContents());
   if (!browser) {
     throttle_cb.Run(kAppTagNoneSelected,
                     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, handle->GetWebContents());
+      app_info, throttle_cb, handle->GetWebContents(), false);
   delegate->set_margins(gfx::Insets());
   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
@@ skipping 39 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.
+  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.
+  header_layout->StartRow(0, 0);
+  AddChildView(header);
+
+  // Setting a view and a layout for a ScrollView for the apps' list.
+  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));
+    app_label_ptr_.emplace_back(tmp_label);
+    scrollable_view->AddChildView(tmp_label);
   }
 
-  // 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));
+  // 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.
+  if (app_info_.size() <= kMaxAppResults) {
+    scroll_view->ClipHeightTo(kMaxWidth, app_info_.size() * kRowHeight);
+  } else {
+    scroll_view->ClipHeightTo(kMaxWidth, (kMaxAppResults + 0.5) * kRowHeight);

[Yusuke Sato, 2016/07/19 10:23:58]

Can you add a code comment to document why we need '+ 0.5'?

[djacobo_, 2016/07/20 21:18:30]

Done.

+  }
+  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, 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);
 }
 
 IntentPickerBubbleView::IntentPickerBubbleView(
     const std::vector<NameAndIcon>& app_info,
     ThrottleCallback throttle_cb,
-    content::WebContents* web_contents)
+    content::WebContents* web_contents,
+    bool was_callback_run)
     : views::BubbleDialogDelegateView(nullptr /* anchor_view */,
                                       views::BubbleBorder::TOP_CENTER),
       WebContentsObserver(web_contents),
-      was_callback_run_(false),
+      was_callback_run_(was_callback_run),
       throttle_cb_(throttle_cb),
       selected_app_tag_(kAppTagNoneSelected),
       always_button_(nullptr),
       just_once_button_(nullptr),
-      app_info_(app_info) {}
+      app_info_(app_info) {
+  app_label_ptr_.clear();

[Yusuke Sato, 2016/07/19 10:23:58]

std::vector is initially empty. please remove.

[djacobo_, 2016/07/20 21:18:30]

Done.

+}
 
 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_) {
     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_) {

[Yusuke Sato, 2016/07/19 10:23:58]

What's wrong with running a cb for the unit tests? Can't the test define an
empty callback function?

[djacobo_, 2016/07/20 21:18:30]

Yes, I will have a dummy method for receiving this call. Done

+        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) {
+        app_label_ptr_[selected_app_tag_]->set_background(
+            views::Background::CreateSolidBackground(SK_ColorWHITE));
+        app_label_ptr_[selected_app_tag_]->SchedulePaint();
+      }
       selected_app_tag_ = sender->tag();
+      app_label_ptr_[selected_app_tag_]->set_background(
+          views::Background::CreateSolidBackground(
+              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();
 }