Reland "Change ButtonPressed overrides to callbacks: c/b/ui/views/sharesheet/"

This reverts commit f524b7f0.

Reason for revert: I will add a simple fix.

Original change's description:
> Revert "Change ButtonPressed overrides to callbacks: c/b/ui/views/sharesheet/"
>
> This reverts commit 1ac9a1da.
>
> Reason for revert: Breaking UI
>
> Original change's description:
> > Change ButtonPressed overrides to callbacks: c/b/ui/views/sharesheet/
> >
> > This allows converting |targets_| from a member to a parameter that's
> > passed around.  I also did some other cleanup while there.
> >
> > Bug: 772945
> > Change-Id: Iaa7f70594ba109d1598da3566856e90c8b09629d
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2481246
> > Commit-Queue: Melissa Zhang <melzhang@chromium.org>
> > Reviewed-by: Melissa Zhang <melzhang@chromium.org>
> > Auto-Submit: Peter Kasting <pkasting@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#818324}
>
> TBR=pkasting@chromium.org,melzhang@chromium.org
>
> Change-Id: Ief930a27f44caef219bf782a7534ef8bc76d83ef
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 772945
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2483745
> Reviewed-by: Melissa Zhang <melzhang@chromium.org>
> Commit-Queue: Melissa Zhang <melzhang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#818357}

TBR=pkasting@chromium.org,melzhang@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 772945
Change-Id: I00d7acd3d7c812dfde8b584c2e53b6b2e1c237a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2483987Reviewed-by: Melissa Zhang <melzhang@chromium.org>
Commit-Queue: Melissa Zhang <melzhang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818379}

chrome/browser/ui/views/sharesheet/sharesheet_bubble_view.cc

@@ -105,7 +105,6 @@ void SharesheetBubbleView::ShowBubble(
     std::vector<TargetInfo> targets,
     apps::mojom::IntentPtr intent,
     sharesheet::CloseCallback close_callback) {
-  targets_ = std::move(targets);
   intent_ = std::move(intent);
   close_callback_ = std::move(close_callback);
 
@@ -132,7 +131,7 @@ void SharesheetBubbleView::ShowBubble(
   main_layout->AddPaddingRow(views::GridLayout::kFixedSize, kSpacing);
 
   auto scroll_view = std::make_unique<views::ScrollView>();
-  scroll_view->SetContents(MakeScrollableTargetView());
+  scroll_view->SetContents(MakeScrollableTargetView(std::move(targets)));
   scroll_view->ClipHeightTo(kTargetViewHeight, kTargetViewExpandedHeight);
 
   // TODO(crbug.com/1097623) Update grey border lines.
@@ -142,8 +141,9 @@ void SharesheetBubbleView::ShowBubble(
 
   main_layout->StartRow(views::GridLayout::kFixedSize, COLUMN_SET_ID_TITLE,
                         kShortSpacing);
-  expand_button_ =
-      main_layout->AddView(std::make_unique<SharesheetExpandButton>(this));
+  expand_button_ = main_layout->AddView(
+      std::make_unique<SharesheetExpandButton>(base::BindRepeating(
+          &SharesheetBubbleView::ExpandButtonPressed, base::Unretained(this))));
   main_layout->AddPaddingRow(views::GridLayout::kFixedSize, kShortSpacing);
 
   main_view_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
@@ -153,17 +153,18 @@ void SharesheetBubbleView::ShowBubble(
   GetWidget()->GetRootView()->Layout();
   widget->Show();
 
-  if (targets_.size() <= (kMaxRowsForDefaultView * kMaxTargetsPerRow)) {
+  if (expanded_view_->children().size() > 1) {
+    SetToDefaultBubbleSizing();
+  } else {
     width_ = kDefaultBubbleWidth;
     height_ = kNoExtensionBubbleHeight;
     expand_button_->SetVisible(false);
-  } else {
-    SetToDefaultBubbleSizing();
   }
   UpdateAnchorPosition();
 }
 
-std::unique_ptr<views::View> SharesheetBubbleView::MakeScrollableTargetView() {
+std::unique_ptr<views::View> SharesheetBubbleView::MakeScrollableTargetView(
+    std::vector<TargetInfo> targets) {
   // Set up default and expanded views.
   auto default_view = std::make_unique<views::View>();
   auto* default_layout =
@@ -197,7 +198,8 @@ std::unique_ptr<views::View> SharesheetBubbleView::MakeScrollableTargetView() {
   expanded_layout->AddPaddingRow(views::GridLayout::kFixedSize,
                                  kExpandViewPadding);
 
-  PopulateLayoutsWithTargets(default_layout, expanded_layout);
+  PopulateLayoutsWithTargets(std::move(targets), default_layout,
+                             expanded_layout);
   default_layout->AddPaddingRow(views::GridLayout::kFixedSize, kShortSpacing);
 
   auto scrollable_view = std::make_unique<views::View>();
@@ -215,14 +217,15 @@ std::unique_ptr<views::View> SharesheetBubbleView::MakeScrollableTargetView() {
 }
 
 void SharesheetBubbleView::PopulateLayoutsWithTargets(
+    std::vector<TargetInfo> targets,
     views::GridLayout* default_layout,
     views::GridLayout* expanded_layout) {
   // Add first kMaxRowsForDefaultView*kMaxTargetsPerRow targets to
   // |default_view| and subsequent targets to |expanded_view|.
-  size_t target_counter = 0;
   size_t row_count = 0;
+  size_t target_counter = 0;
   auto* layout_for_target = default_layout;
-  for (const auto& target : targets_) {
+  for (auto& target : targets) {
     if (target_counter % kMaxTargetsPerRow == 0) {
       // When we've reached kMaxRowsForDefaultView switch to populating
       // |expanded_layout|.
@@ -238,13 +241,16 @@ void SharesheetBubbleView::PopulateLayoutsWithTargets(
       layout_for_target->StartRow(views::GridLayout::kFixedSize,
                                   COLUMN_SET_ID_TARGETS);
     }
+    ++target_counter;
 
     base::string16 secondary_display_name =
         target.secondary_display_name.value_or(base::string16());
 
     auto target_view = std::make_unique<SharesheetTargetButton>(
-        this, target.display_name, secondary_display_name, &target.icon);
-    target_view->set_tag(target_counter++);
+        base::BindRepeating(&SharesheetBubbleView::TargetButtonPressed,
+                            base::Unretained(this),
+                            base::Passed(std::move(target))),
+        target.display_name, secondary_display_name, &target.icon);
 
     layout_for_target->AddView(std::move(target_view));
   }
@@ -265,7 +271,6 @@ void SharesheetBubbleView::CloseBubble() {
   views::Widget* widget = View::GetWidget();
   widget->CloseWithReason(views::Widget::ClosedReason::kAcceptButtonClicked);
   // Reset all bubble values.
-  targets_.clear();
   active_target_ = base::string16();
   intent_.reset();
   keyboard_highlighted_target_ = 0;
@@ -297,63 +302,25 @@ void SharesheetBubbleView::OnKeyEvent(ui::KeyEvent* event) {
       break;
   }
 
-  keyboard_highlighted_target_ += delta;
-
-  int default_view_max = kMaxTargetsPerRow * kMaxRowsForDefaultView - 1;
-  int max_target_index = targets_.size() - 1;
-  if ((!show_expanded_view_) && (max_target_index > default_view_max)) {
-    max_target_index = default_view_max;
-  }
-
-  if (keyboard_highlighted_target_ > max_target_index) {
-    keyboard_highlighted_target_ = max_target_index;
-  } else if (keyboard_highlighted_target_ < 0) {
-    keyboard_highlighted_target_ = 0;
-  }
+  const size_t default_views = default_view_->children().size();
+  // The -1 here and +1 below account for the app list label.
+  const size_t targets =
+      default_views +
+      (show_expanded_view_ ? (expanded_view_->children().size() - 1) : 0);
+  const int new_target = int{keyboard_highlighted_target_} + delta;
+  keyboard_highlighted_target_ =
+      size_t{base::ClampToRange(new_target, 0, int{targets} - 1)};
 
-  if (keyboard_highlighted_target_ <= default_view_max) {
+  if (keyboard_highlighted_target_ < default_views) {
     default_view_->children()[keyboard_highlighted_target_]->RequestFocus();
   } else {
-    DCHECK_LT(keyboard_highlighted_target_ - default_view_max,
-              expanded_view_->children().size());
-    DCHECK(show_expanded_view_);
-    expanded_view_->children()[keyboard_highlighted_target_ - default_view_max]
+    expanded_view_->children()[keyboard_highlighted_target_ + 1 - default_views]
         ->RequestFocus();
   }
 
   View::OnKeyEvent(event);
 }
 
-void SharesheetBubbleView::ButtonPressed(views::Button* sender,
-                                         const ui::Event& event) {
-  if (sender == expand_button_) {
-    if (show_expanded_view_) {
-      expand_button_->SetDefaultView();
-      expanded_view_->SetVisible(false);
-      ResizeBubble(kDefaultBubbleWidth, kDefaultBubbleHeight);
-    } else {
-      expand_button_->SetExpandedView();
-      expanded_view_->SetVisible(true);
-      ResizeBubble(kDefaultBubbleWidth, kExpandedBubbleHeight);
-    }
-    show_expanded_view_ = !show_expanded_view_;
-  } else {
-    auto type = targets_[sender->tag()].type;
-    if (type == sharesheet::TargetType::kAction) {
-      active_target_ = targets_[sender->tag()].launch_name;
-    } else {
-      intent_->activity_name = targets_[sender->tag()].activity_name;
-    }
-    delegate_->OnTargetSelected(targets_[sender->tag()].launch_name, type,
-                                std::move(intent_), share_action_view_);
-    intent_.reset();
-    user_cancelled_ = false;
-    if (close_callback_) {
-      std::move(close_callback_).Run(sharesheet::SharesheetResult::kSuccess);
-    }
-  }
-}
-
 std::unique_ptr<views::NonClientFrameView>
 SharesheetBubbleView::CreateNonClientFrameView(views::Widget* widget) {
   auto bubble_border =
@@ -413,6 +380,31 @@ void SharesheetBubbleView::CreateBubble() {
   share_action_view_->SetVisible(false);
 }
 
+void SharesheetBubbleView::ExpandButtonPressed() {
+  show_expanded_view_ = !show_expanded_view_;
+  if (show_expanded_view_)
+    expand_button_->SetExpandedView();
+  else
+    expand_button_->SetDefaultView();
+  expanded_view_->SetVisible(show_expanded_view_);
+  ResizeBubble(kDefaultBubbleWidth, show_expanded_view_ ? kExpandedBubbleHeight
+                                                        : kDefaultBubbleHeight);
+}
+
+void SharesheetBubbleView::TargetButtonPressed(TargetInfo target) {
+  auto type = target.type;
+  if (type == sharesheet::TargetType::kAction)
+    active_target_ = target.launch_name;
+  else
+    intent_->activity_name = target.activity_name;
+  delegate_->OnTargetSelected(target.launch_name, type, std::move(intent_),
+                              share_action_view_);
+  intent_.reset();
+  user_cancelled_ = false;
+  if (close_callback_)
+    std::move(close_callback_).Run(sharesheet::SharesheetResult::kSuccess);
+}
+
 void SharesheetBubbleView::UpdateAnchorPosition() {
   // If |width_| is not set, set to default value.
   if (width_ == 0) {

chrome/browser/ui/views/sharesheet/sharesheet_bubble_view.h

@@ -10,7 +10,6 @@
 #include "chrome/browser/sharesheet/sharesheet_types.h"
 #include "components/services/app_service/public/mojom/types.mojom.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
-#include "ui/views/controls/button/button.h"
 
 namespace views {
 class GridLayout;
@@ -26,8 +25,7 @@ class WebContents;
 
 class SharesheetExpandButton;
 
-class SharesheetBubbleView : public views::BubbleDialogDelegateView,
-                             public views::ButtonListener {
+class SharesheetBubbleView : public views::BubbleDialogDelegateView {
  public:
   using TargetInfo = sharesheet::TargetInfo;
 
@@ -51,9 +49,6 @@ class SharesheetBubbleView : public views::BubbleDialogDelegateView,
   // ui::EventHandler overrides:
   void OnKeyEvent(ui::KeyEvent* event) override;
 
-  // views::ButtonListener overrides
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // views::WidgetDelegate override
   std::unique_ptr<views::NonClientFrameView> CreateNonClientFrameView(
       views::Widget* widget) override;
@@ -63,16 +58,18 @@ class SharesheetBubbleView : public views::BubbleDialogDelegateView,
   void OnWidgetDestroyed(views::Widget* widget) override;
 
   void CreateBubble();
-  std::unique_ptr<views::View> MakeScrollableTargetView();
-  void PopulateLayoutsWithTargets(views::GridLayout* default_layout,
+  std::unique_ptr<views::View> MakeScrollableTargetView(
+      std::vector<TargetInfo> targets);
+  void PopulateLayoutsWithTargets(std::vector<TargetInfo> targets,
+                                  views::GridLayout* default_layout,
                                   views::GridLayout* expanded_layout);
-  void OnDialogClosed();
+  void ExpandButtonPressed();
+  void TargetButtonPressed(TargetInfo target);
   void UpdateAnchorPosition();
   void SetToDefaultBubbleSizing();
 
   // Owns this class.
   sharesheet::SharesheetServiceDelegate* delegate_;
-  std::vector<TargetInfo> targets_;
   base::string16 active_target_;
   apps::mojom::IntentPtr intent_;
   sharesheet::CloseCallback close_callback_;
@@ -82,7 +79,7 @@ class SharesheetBubbleView : public views::BubbleDialogDelegateView,
   bool user_cancelled_ = true;
   bool show_expanded_view_ = false;
 
-  int keyboard_highlighted_target_ = 0;
+  size_t keyboard_highlighted_target_ = 0;
 
   views::View* root_view_ = nullptr;
   views::View* main_view_ = nullptr;

chrome/browser/ui/views/sharesheet/sharesheet_expand_button.cc

@@ -27,8 +27,8 @@ constexpr SkColor kLabelColor = gfx::kGoogleBlue600;
 
 }  // namespace
 
-SharesheetExpandButton::SharesheetExpandButton(views::ButtonListener* listener)
-    : Button(listener) {
+SharesheetExpandButton::SharesheetExpandButton(PressedCallback callback)
+    : Button(std::move(callback)) {
   auto* layout = SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kHorizontal, gfx::Insets(6, 16, 6, 16),
       kBetweenChildSpacing, true));

chrome/browser/ui/views/sharesheet/sharesheet_expand_button.h

@@ -12,7 +12,7 @@
 
 class SharesheetExpandButton : public views::Button {
  public:
-  explicit SharesheetExpandButton(views::ButtonListener* listener);
+  explicit SharesheetExpandButton(PressedCallback callback);
   SharesheetExpandButton(const SharesheetExpandButton&) = delete;
   SharesheetExpandButton& operator=(const SharesheetExpandButton&) = delete;
 

chrome/browser/ui/views/sharesheet/sharesheet_target_button.cc

@@ -35,11 +35,11 @@ constexpr SkColor kShareTargetSecondaryTitleColor = gfx::kGoogleGrey600;
 
 // A button that represents a candidate share target.
 SharesheetTargetButton::SharesheetTargetButton(
-    views::ButtonListener* listener,
+    PressedCallback callback,
     const base::string16& display_name,
     const base::string16& secondary_display_name,
     const gfx::ImageSkia* icon)
-    : Button(listener) {
+    : Button(std::move(callback)) {
   // TODO(crbug.com/1097623) Margins shouldn't be within
   // SharesheetTargetButton as the margins are different in |expanded_view_|.
   auto* layout = SetLayoutManager(std::make_unique<views::BoxLayout>(

chrome/browser/ui/views/sharesheet/sharesheet_target_button.h

@@ -11,7 +11,7 @@
 
 class SharesheetTargetButton : public views::Button {
  public:
-  SharesheetTargetButton(views::ButtonListener* listener,
+  SharesheetTargetButton(PressedCallback callback,
                          const base::string16& display_name,
                          const base::string16& secondary_display_name,
                          const gfx::ImageSkia* icon);