Change ButtonPressed overrides to callbacks: .../send_tab_to_self/

This also reworks how the class is tested so only the public API is
tested, and the actual construction/teardown are more like production.

Bug: 772945
Change-Id: I8cd6970d314359296398104484afc7551441808f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2469666
Commit-Queue: Travis Skare <skare@chromium.org>
Reviewed-by: Travis Skare <skare@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818678}

chrome/browser/ui/send_tab_to_self/send_tab_to_self_bubble_controller.cc

@@ -61,9 +61,14 @@ base::string16 SendTabToSelfBubbleController::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_CONTEXT_MENU_SEND_TAB_TO_SELF);
 }
 
-const std::vector<TargetDeviceInfo>&
-SendTabToSelfBubbleController::GetValidDevices() const {
-  return valid_devices_;
+std::vector<TargetDeviceInfo> SendTabToSelfBubbleController::GetValidDevices()
+    const {
+  SendTabToSelfSyncService* const service =
+      SendTabToSelfSyncServiceFactory::GetForProfile(GetProfile());
+  SendTabToSelfModel* const model =
+      service ? service->GetSendTabToSelfModel() : nullptr;
+  return model ? model->GetTargetDeviceInfoSortedList()
+               : std::vector<TargetDeviceInfo>();
 }
 
 Profile* SendTabToSelfBubbleController::GetProfile() const {
@@ -111,21 +116,6 @@ SendTabToSelfBubbleController::SendTabToSelfBubbleController(
     content::WebContents* web_contents)
     : web_contents_(web_contents) {
   DCHECK(web_contents);
-  FetchDeviceInfo();
-}
-
-void SendTabToSelfBubbleController::FetchDeviceInfo() {
-  valid_devices_.clear();
-  SendTabToSelfSyncService* service =
-      SendTabToSelfSyncServiceFactory::GetForProfile(GetProfile());
-  if (!service) {
-    return;
-  }
-  SendTabToSelfModel* model = service->GetSendTabToSelfModel();
-  if (!model) {
-    return;
-  }
-  valid_devices_ = model->GetTargetDeviceInfoSortedList();
 }
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(SendTabToSelfBubbleController)

chrome/browser/ui/send_tab_to_self/send_tab_to_self_bubble_controller.h

@@ -44,7 +44,7 @@ class SendTabToSelfBubbleController
   // Returns the title of send tab to self bubble.
   base::string16 GetWindowTitle() const;
   // Returns the valid devices info map.
-  const std::vector<TargetDeviceInfo>& GetValidDevices() const;
+  virtual std::vector<TargetDeviceInfo> GetValidDevices() const;
   // Returns current profile.
   Profile* GetProfile() const;
 
@@ -80,15 +80,10 @@ class SendTabToSelfBubbleController
   FRIEND_TEST_ALL_PREFIXES(SendTabToSelfBubbleViewImplTest, PopulateScrollView);
   FRIEND_TEST_ALL_PREFIXES(SendTabToSelfBubbleViewImplTest, DevicePressed);
 
-  // Get information of valid devices.
-  void FetchDeviceInfo();
-
   // The web_contents associated with this controller.
   content::WebContents* web_contents_;
   // Weak reference. Will be nullptr if no bubble is currently shown.
   SendTabToSelfBubbleView* send_tab_to_self_bubble_view_ = nullptr;
-  // Valid devices data.
-  std::vector<TargetDeviceInfo> valid_devices_;
   // True if a confirmation message should be shown in the omnibox.
   bool show_message_ = false;
 

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_device_button.cc

@@ -11,6 +11,7 @@
 #include "base/time/time.h"
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/ui/views/hover_button.h"
+#include "chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_view_impl.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/send_tab_to_self/target_device_info.h"
 #include "components/sync/protocol/sync.pb.h"
@@ -53,17 +54,18 @@ base::string16 GetLastUpdatedTime(const TargetDeviceInfo& device_info) {
 }  // namespace
 
 SendTabToSelfBubbleDeviceButton::SendTabToSelfBubbleDeviceButton(
-    views::ButtonListener* button_listener,
-    const TargetDeviceInfo& device_info,
-    int button_tag)
-    : HoverButton(button_listener,
-                  CreateIcon(device_info.device_type),
-                  base::UTF8ToUTF16(device_info.device_name),
-                  GetLastUpdatedTime(device_info)) {
+    SendTabToSelfBubbleViewImpl* bubble,
+    const TargetDeviceInfo& device_info)
+    : HoverButton(
+          base::BindRepeating(&SendTabToSelfBubbleViewImpl::DeviceButtonPressed,
+                              base::Unretained(bubble),
+                              base::Unretained(this)),
+          CreateIcon(device_info.device_type),
+          base::UTF8ToUTF16(device_info.device_name),
+          GetLastUpdatedTime(device_info)) {
   device_name_ = device_info.device_name;
   device_guid_ = device_info.cache_guid;
   device_type_ = device_info.device_type;
-  set_tag(button_tag);
   SetEnabled(true);
 }
 

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_device_button.h

@@ -13,15 +13,15 @@
 
 namespace send_tab_to_self {
 
+class SendTabToSelfBubbleViewImpl;
 struct TargetDeviceInfo;
 
 // A button representing a device in share bubble. It is highlighted when
 // hovered.
 class SendTabToSelfBubbleDeviceButton : public HoverButton {
  public:
-  SendTabToSelfBubbleDeviceButton(views::ButtonListener* button_listener,
-                                  const TargetDeviceInfo& device_info,
-                                  int button_tag);
+  SendTabToSelfBubbleDeviceButton(SendTabToSelfBubbleViewImpl* bubble,
+                                  const TargetDeviceInfo& device_info);
   ~SendTabToSelfBubbleDeviceButton() override;
 
   const std::string& device_name() const { return device_name_; }

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_view_impl.cc

@@ -40,7 +40,6 @@ SendTabToSelfBubbleViewImpl::SendTabToSelfBubbleViewImpl(
     content::WebContents* web_contents,
     SendTabToSelfBubbleController* controller)
     : LocationBarBubbleDelegateView(anchor_view, web_contents),
-      web_contents_(web_contents),
       controller_(controller) {
   SetButtons(ui::DIALOG_BUTTON_NONE);
   set_fixed_width(views::LayoutProvider::Get()->GetDistanceMetric(
@@ -73,11 +72,6 @@ void SendTabToSelfBubbleViewImpl::WindowClosing() {
   }
 }
 
-void SendTabToSelfBubbleViewImpl::ButtonPressed(views::Button* sender,
-                                                const ui::Event& event) {
-  DevicePressed(sender->tag());
-}
-
 void SendTabToSelfBubbleViewImpl::OnPaint(gfx::Canvas* canvas) {
   views::BubbleDialogDelegateView::OnPaint(canvas);
 }
@@ -86,9 +80,19 @@ void SendTabToSelfBubbleViewImpl::Show(DisplayReason reason) {
   ShowForReason(reason);
 }
 
-const std::vector<SendTabToSelfBubbleDeviceButton*>&
-SendTabToSelfBubbleViewImpl::GetDeviceButtonsForTest() {
-  return device_buttons_;
+void SendTabToSelfBubbleViewImpl::DeviceButtonPressed(
+    SendTabToSelfBubbleDeviceButton* device_button) {
+  if (!controller_)
+    return;
+
+  controller_->OnDeviceSelected(device_button->device_name(),
+                                device_button->device_guid());
+  Hide();
+}
+
+const views::View* SendTabToSelfBubbleViewImpl::GetButtonContainerForTesting()
+    const {
+  return scroll_view_->contents();
 }
 
 void SendTabToSelfBubbleViewImpl::Init() {
@@ -108,49 +112,29 @@ void SendTabToSelfBubbleViewImpl::Init() {
 }
 
 void SendTabToSelfBubbleViewImpl::CreateScrollView() {
-  scroll_view_ = new views::ScrollView();
-  AddChildView(scroll_view_);
+  scroll_view_ = AddChildView(std::make_unique<views::ScrollView>());
   scroll_view_->ClipHeightTo(0, kDeviceButtonHeight * kMaximumButtons);
 }
 
 void SendTabToSelfBubbleViewImpl::PopulateScrollView(
     const std::vector<TargetDeviceInfo>& devices) {
-  DCHECK(device_buttons_.empty());
-  auto device_list_view = std::make_unique<views::View>();
+  auto* device_list_view =
+      scroll_view_->SetContents(std::make_unique<views::View>());
   device_list_view->SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kVertical));
-  int tag = 0;
   for (const auto& device : devices) {
-    auto device_button = std::make_unique<SendTabToSelfBubbleDeviceButton>(
-        this, device,
-        /** button_tag */ tag++);
-    device_buttons_.push_back(device_button.get());
-    device_list_view->AddChildView(std::move(device_button));
+    device_list_view->AddChildView(
+        std::make_unique<SendTabToSelfBubbleDeviceButton>(this, device));
   }
-  scroll_view_->SetContents(std::move(device_list_view));
 
   MaybeSizeToContents();
   Layout();
 }
 
-void SendTabToSelfBubbleViewImpl::DevicePressed(size_t index) {
-  if (!controller_) {
-    return;
-  }
-
-  DCHECK_LT(index, device_buttons_.size());
-
-  SendTabToSelfBubbleDeviceButton* device_button = device_buttons_[index];
-  controller_->OnDeviceSelected(device_button->device_name(),
-                                device_button->device_guid());
-  Hide();
-}
-
 void SendTabToSelfBubbleViewImpl::MaybeSizeToContents() {
   // The widget may be null if this is called while the dialog is opening.
-  if (GetWidget()) {
+  if (GetWidget())
     SizeToContents();
-  }
 }
 
 }  // namespace send_tab_to_self

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_view_impl.h

@@ -15,7 +15,6 @@
 #include "chrome/browser/ui/media_router/cast_dialog_controller.h"
 #include "chrome/browser/ui/send_tab_to_self/send_tab_to_self_bubble_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h"
-#include "ui/views/controls/button/button.h"
 
 namespace gfx {
 class Canvas;
@@ -34,7 +33,6 @@ struct TargetDeviceInfo;
 // View component of the send tab to self bubble that allows users to choose
 // target device to send tab to.
 class SendTabToSelfBubbleViewImpl : public SendTabToSelfBubbleView,
-                                    public views::ButtonListener,
                                     public LocationBarBubbleDelegateView {
  public:
   // Bubble will be anchored to |anchor_view|.
@@ -52,24 +50,17 @@ class SendTabToSelfBubbleViewImpl : public SendTabToSelfBubbleView,
   base::string16 GetWindowTitle() const override;
   void WindowClosing() override;
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // LocationBarBubbleDelegateView:
   void OnPaint(gfx::Canvas* canvas) override;
 
   // Shows the bubble view.
   void Show(DisplayReason reason);
 
-  // Called by tests.
-  const std::vector<SendTabToSelfBubbleDeviceButton*>&
-  GetDeviceButtonsForTest();
+  void DeviceButtonPressed(SendTabToSelfBubbleDeviceButton* device_button);
 
- private:
-  friend class SendTabToSelfBubbleViewImplTest;
-  FRIEND_TEST_ALL_PREFIXES(SendTabToSelfBubbleViewImplTest, PopulateScrollView);
-  FRIEND_TEST_ALL_PREFIXES(SendTabToSelfBubbleViewImplTest, DevicePressed);
+  const views::View* GetButtonContainerForTesting() const;
 
+ private:
   // views::BubbleDialogDelegateView:
   void Init() override;
 
@@ -79,22 +70,15 @@ class SendTabToSelfBubbleViewImpl : public SendTabToSelfBubbleView,
   // Populates the scroll view containing valid devices.
   void PopulateScrollView(const std::vector<TargetDeviceInfo>& devices);
 
-  // Handles the action when a target device has been pressed.
-  void DevicePressed(size_t index);
-
   // Resizes and potentially moves the bubble to fit the content's preferred
   // size.
   void MaybeSizeToContents();
 
-  content::WebContents* web_contents_;         // Weak reference.
   SendTabToSelfBubbleController* controller_;  // Weak reference.
 
   // Title shown at the top of the bubble.
   base::string16 bubble_title_;
 
-  // Contains references to device buttons in the order they appear.
-  std::vector<SendTabToSelfBubbleDeviceButton*> device_buttons_;
-
   // ScrollView containing the list of device buttons.
   views::ScrollView* scroll_view_ = nullptr;
 

chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_view_impl_unittest.cc

@@ -25,27 +25,24 @@ class SendTabToSelfBubbleControllerMock : public SendTabToSelfBubbleController {
   SendTabToSelfBubbleControllerMock() = default;
   ~SendTabToSelfBubbleControllerMock() override = default;
 
+  std::vector<TargetDeviceInfo> GetValidDevices() const override {
+    base::SimpleTestClock clock;
+    return {{"Device_1", "Device_1", "device_guid_1",
+             sync_pb::SyncEnums_DeviceType_TYPE_LINUX,
+             clock.Now() - base::TimeDelta::FromDays(0)},
+            {"Device_2", "Device_2", "device_guid_2",
+             sync_pb::SyncEnums_DeviceType_TYPE_WIN,
+             clock.Now() - base::TimeDelta::FromDays(1)},
+            {"Device_3", "Device_3", "device_guid_3",
+             sync_pb::SyncEnums_DeviceType_TYPE_PHONE,
+             clock.Now() - base::TimeDelta::FromDays(5)}};
+  }
+
   MOCK_METHOD2(OnDeviceSelected,
                void(const std::string& target_device_name,
                     const std::string& target_device_guid));
 };
 
-class SendTabToSelfBubbleViewImplMock : public SendTabToSelfBubbleViewImpl {
- public:
-  SendTabToSelfBubbleViewImplMock(views::View* anchor_view,
-                                  content::WebContents* web_contents,
-                                  SendTabToSelfBubbleController* controller)
-      : SendTabToSelfBubbleViewImpl(anchor_view,
-                                    web_contents,
-                                    controller) {}
-  ~SendTabToSelfBubbleViewImplMock() override = default;
-
-  // The delegate cannot find widget since it is created from a null profile.
-  // This method will be called inside DevicePressed(). Unit tests will
-  // chrash without mocking.
-  MOCK_METHOD0(CloseBubble, void());
-};
-
 }  // namespace
 
 class SendTabToSelfBubbleViewImplTest : public ChromeViewsTestBase {
@@ -58,54 +55,34 @@ class SendTabToSelfBubbleViewImplTest : public ChromeViewsTestBase {
 
     profile_ = std::make_unique<TestingProfile>();
     controller_ = std::make_unique<SendTabToSelfBubbleControllerMock>();
-    bubble_ = std::make_unique<SendTabToSelfBubbleViewImplMock>(
-        anchor_widget_->GetContentsView(), nullptr, controller_.get());
+    bubble_ = new SendTabToSelfBubbleViewImpl(anchor_widget_->GetContentsView(),
+                                              nullptr, controller_.get());
+    views::BubbleDialogDelegateView::CreateBubble(bubble_);
   }
 
   void TearDown() override {
+    bubble_->GetWidget()->CloseNow();
     anchor_widget_.reset();
     ChromeViewsTestBase::TearDown();
   }
 
-  const std::vector<TargetDeviceInfo> SetUpDeviceList() {
-    base::SimpleTestClock clock;
-    std::vector<TargetDeviceInfo> list;
-    TargetDeviceInfo valid_device_1(
-        "Device_1", "Device_1", "device_guid_1",
-        sync_pb::SyncEnums_DeviceType_TYPE_LINUX,
-        /*last_updated_timestamp=*/clock.Now() - base::TimeDelta::FromDays(0));
-    TargetDeviceInfo valid_device_2(
-        "Device_2", "Device_2", "device_guid_2",
-        sync_pb::SyncEnums_DeviceType_TYPE_WIN,
-        /*last_updated_timestamp=*/clock.Now() - base::TimeDelta::FromDays(1));
-    TargetDeviceInfo valid_device_3(
-        "Device_3", "Device_3", "device_guid_3",
-        sync_pb::SyncEnums_DeviceType_TYPE_PHONE,
-        /*last_updated_timestamp=*/clock.Now() - base::TimeDelta::FromDays(5));
-    list.push_back(valid_device_1);
-    list.push_back(valid_device_2);
-    list.push_back(valid_device_3);
-    return list;
-  }
-
   std::unique_ptr<TestingProfile> profile_;
   std::unique_ptr<views::Widget> anchor_widget_;
   std::unique_ptr<SendTabToSelfBubbleControllerMock> controller_;
-  std::unique_ptr<SendTabToSelfBubbleViewImpl> bubble_;
+  SendTabToSelfBubbleViewImpl* bubble_;
 };
 
-TEST_F(SendTabToSelfBubbleViewImplTest, PopulateScrollView) {
-  bubble_->CreateScrollView();
-  bubble_->PopulateScrollView(SetUpDeviceList());
-  EXPECT_EQ(3UL, bubble_->GetDeviceButtonsForTest().size());
+TEST_F(SendTabToSelfBubbleViewImplTest, Init) {
+  EXPECT_EQ(3U, bubble_->GetButtonContainerForTesting()->children().size());
 }
 
-TEST_F(SendTabToSelfBubbleViewImplTest, DevicePressed) {
-  bubble_->Init();
-  bubble_->PopulateScrollView(SetUpDeviceList());
+TEST_F(SendTabToSelfBubbleViewImplTest, ButtonPressed) {
   EXPECT_CALL(*controller_.get(),
               OnDeviceSelected("Device_3", "device_guid_3"));
-  bubble_->DevicePressed(2);
+  const views::View* button_container = bubble_->GetButtonContainerForTesting();
+  ASSERT_EQ(3U, button_container->children().size());
+  bubble_->DeviceButtonPressed(static_cast<SendTabToSelfBubbleDeviceButton*>(
+      button_container->children()[2]));
 }
 
 }  // namespace send_tab_to_self