Clean up SharingDialogView and avoid needing friend decls for testing.

Some of this cleanup will facilitate replacing ButtonListener with
callbacks in a future CL.

Bug: none
Change-Id: Ieefc3be0238bdaa95fc171b7ee7c55e4f8e41a90
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2472899
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818987}

chrome/browser/sharing/click_to_call/click_to_call_context_menu_observer.cc

@@ -146,8 +146,8 @@ void ClickToCallContextMenuObserver::SendClickToCallMessage(
   if (size_t{chosen_device_index} >= devices_.size())
     return;
 
-  LogSharingSelectedDeviceIndex(controller_->GetFeatureMetricsPrefix(),
-                                kSharingUiContextMenu, chosen_device_index);
+  LogSharingSelectedIndex(controller_->GetFeatureMetricsPrefix(),
+                          kSharingUiContextMenu, chosen_device_index);
 
   controller_->OnDeviceSelected(phone_number_, *devices_[chosen_device_index],
                                 *entry_point_);

chrome/browser/sharing/shared_clipboard/shared_clipboard_context_menu_observer.cc

@@ -141,8 +141,8 @@ void SharedClipboardContextMenuObserver::SendSharedClipboardMessage(
     int chosen_device_index) {
   if (size_t{chosen_device_index} >= devices_.size())
     return;
-  LogSharingSelectedDeviceIndex(controller_->GetFeatureMetricsPrefix(),
-                                nullptr /* No suffix */, chosen_device_index);
+  LogSharingSelectedIndex(controller_->GetFeatureMetricsPrefix(),
+                          nullptr /* No suffix */, chosen_device_index);
 
   controller_->OnDeviceSelected(text_, *devices_[chosen_device_index]);
   LogSharedClipboardSelectedTextSize(text_.size());

chrome/browser/sharing/sharing_metrics.cc

@@ -211,40 +211,22 @@ void LogSharingAppsToShow(SharingFeatureName feature,
       /*value_max=*/20);
 }
 
-void LogSharingSelectedDeviceIndex(SharingFeatureName feature,
-                                   const char* histogram_suffix,
-                                   int index) {
-  auto* feature_str = GetEnumStringValue(feature);
-  // Explicitly log both the base and the suffixed histogram because the base
-  // aggregation is not automatically generated.
-  base::UmaHistogramExactLinear(
-      base::StrCat({"Sharing.", feature_str, "SelectedDeviceIndex"}), index,
-      /*value_max=*/20);
-  if (!histogram_suffix)
-    return;
-  base::UmaHistogramExactLinear(
-      base::StrCat(
-          {"Sharing.", feature_str, "SelectedDeviceIndex.", histogram_suffix}),
-      index,
-      /*value_max=*/20);
-}
-
-void LogSharingSelectedAppIndex(SharingFeatureName feature,
-                                const char* histogram_suffix,
-                                int index) {
+void LogSharingSelectedIndex(SharingFeatureName feature,
+                             const char* histogram_suffix,
+                             int index,
+                             SharingIndexType index_type) {
   auto* feature_str = GetEnumStringValue(feature);
   // Explicitly log both the base and the suffixed histogram because the base
   // aggregation is not automatically generated.
-  base::UmaHistogramExactLinear(
-      base::StrCat({"Sharing.", feature_str, "SelectedAppIndex"}), index,
-      /*value_max=*/20);
+  std::string name = base::StrCat(
+      {"Sharing.", feature_str, "Selected",
+       (index_type == SharingIndexType::kDevice) ? "Device" : "App", "Index"});
+  base::UmaHistogramExactLinear(name, index, /*value_max=*/20);
   if (!histogram_suffix)
     return;
-  base::UmaHistogramExactLinear(
-      base::StrCat(
-          {"Sharing.", feature_str, "SelectedAppIndex.", histogram_suffix}),
-      index,
-      /*value_max=*/20);
+  base::UmaHistogramExactLinear(base::StrCat({name, ".", histogram_suffix}),
+                                index,
+                                /*value_max=*/20);
 }
 
 void LogSharingMessageAckTime(chrome_browser_sharing::MessageType message_type,

chrome/browser/sharing/sharing_metrics.h

@@ -79,21 +79,20 @@ void LogSharingAppsToShow(SharingFeatureName feature,
                           const char* histogram_suffix,
                           int count);
 
-// Logs the |index| of the device selected by the user for sharing feature. The
-// |histogram_suffix| indicates in which UI this event happened and must match
-// one from Sharing{feature}Ui defined in histograms.xml - use the
-// constants defined in this file for that.
-void LogSharingSelectedDeviceIndex(SharingFeatureName feature,
-                                   const char* histogram_suffix,
-                                   int index);
-
-// Logs the |index| of the app selected by the user for sharing feature. The
-// |histogram_suffix| indicates in which UI this event happened and must match
-// one from Sharing{feature}Ui defined in histograms.xml - use the
-// constants defined in this file for that.
-void LogSharingSelectedAppIndex(SharingFeatureName feature,
-                                const char* histogram_suffix,
-                                int index);
+// Logs the |index| of the user selection for sharing feature. |index_type| is
+// the type of selection made, either "Device" or "App". The |histogram_suffix|
+// indicates in which UI this event happened and must match one from
+// Sharing{feature}Ui defined in histograms.xml - use the constants defined in
+// this file for that.
+enum class SharingIndexType {
+  kDevice,
+  kApp,
+};
+void LogSharingSelectedIndex(
+    SharingFeatureName feature,
+    const char* histogram_suffix,
+    int index,
+    SharingIndexType index_type = SharingIndexType::kDevice);
 
 // Logs to UMA the time from sending a FCM message from the Sharing service
 // until an ack message is received for it.

chrome/browser/ui/views/sharing/click_to_call_browsertest.cc

@@ -40,6 +40,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/views/layout/grid_layout.h"
+#include "ui/views/test/button_test_api.h"
 #include "url/gurl.h"
 
 namespace {
@@ -402,15 +403,13 @@ IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest, LeftClick_ChooseDevice) {
       static_cast<SharingDialogView*>(controller->dialog());
   EXPECT_EQ(SharingDialogType::kDialogWithDevicesMaybeApps,
             dialog->GetDialogType());
-  EXPECT_EQ(1u, dialog->data_.devices.size());
-  EXPECT_EQ(dialog->data_.devices.size() + dialog->data_.apps.size(),
-            dialog->dialog_buttons_.size());
-
-  const ui::MouseEvent event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
-                             ui::EventTimeForNow(), 0, 0);
 
   // Choose first device.
-  dialog->ButtonPressed(dialog->dialog_buttons_[0], event);
+  const auto& buttons = dialog->button_list_for_testing()->children();
+  ASSERT_GT(buttons.size(), 0u);
+  views::test::ButtonTestApi(static_cast<views::Button*>(buttons[0]))
+      .NotifyClick(ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(),
+                                  gfx::Point(), ui::EventTimeForNow(), 0, 0));
 
   CheckLastReceiver(*devices[0]);
   // Defined in tel.html

chrome/browser/ui/views/sharing/sharing_dialog_view.cc

@@ -175,26 +175,21 @@ SharingDialogType SharingDialogView::GetDialogType() const {
 
 void SharingDialogView::ButtonPressed(views::Button* sender,
                                       const ui::Event& event) {
-  DCHECK(data_.device_callback);
-  DCHECK(data_.app_callback);
-  if (!sender || sender->tag() < 0)
-    return;
-  size_t index{sender->tag()};
-
-  if (index < data_.devices.size()) {
-    LogSharingSelectedDeviceIndex(data_.prefix, kSharingUiDialog, index);
+  DCHECK(sender);
+  size_t index = sender->tag();
+  DCHECK_LT(index, data_.devices.size() + data_.apps.size());
+
+  const bool is_device = index < data_.devices.size();
+  if (!is_device)
+    index -= data_.devices.size();
+  LogSharingSelectedIndex(
+      data_.prefix, kSharingUiDialog, index,
+      is_device ? SharingIndexType::kDevice : SharingIndexType::kApp);
+  if (is_device)
     std::move(data_.device_callback).Run(*data_.devices[index]);
-    CloseBubble();
-    return;
-  }
-
-  index -= data_.devices.size();
-
-  if (index < data_.apps.size()) {
-    LogSharingSelectedAppIndex(data_.prefix, kSharingUiDialog, index);
+  else
     std::move(data_.app_callback).Run(data_.apps[index]);
-    CloseBubble();
-  }
+  CloseBubble();
 }
 
 // static
@@ -271,14 +266,13 @@ void SharingDialogView::InitListView() {
             : kHardwareSmartphoneIcon,
         kPrimaryIconSize);
 
-    auto dialog_button = std::make_unique<HoverButton>(
-        this, std::move(icon), base::UTF8ToUTF16(device->client_name()),
-        GetLastUpdatedTimeInDays(device->last_updated_timestamp()));
+    auto* dialog_button =
+        button_list->AddChildView(std::make_unique<HoverButton>(
+            this, std::move(icon), base::UTF8ToUTF16(device->client_name()),
+            GetLastUpdatedTimeInDays(device->last_updated_timestamp())));
     dialog_button->SetEnabled(true);
     dialog_button->set_tag(tag++);
     dialog_button->SetBorder(views::CreateEmptyBorder(device_border));
-    dialog_buttons_.push_back(
-        button_list->AddChildView(std::move(dialog_button)));
   }
 
   // Apps:
@@ -293,32 +287,32 @@ void SharingDialogView::InitListView() {
       icon->SetImage(app.image.AsImageSkia());
     }
 
-    auto dialog_button =
+    auto* dialog_button = button_list->AddChildView(
         std::make_unique<HoverButton>(this, std::move(icon), app.name,
-                                      /* subtitle= */ base::string16());
+                                      /* subtitle= */ base::string16()));
     dialog_button->SetEnabled(true);
     dialog_button->set_tag(tag++);
     dialog_button->SetBorder(views::CreateEmptyBorder(app_border));
-    dialog_buttons_.push_back(
-        button_list->AddChildView(std::move(dialog_button)));
   }
 
   // Allow up to 5 buttons in the list and let the rest scroll.
   constexpr size_t kMaxDialogButtons = 5;
-  if (dialog_buttons_.size() > kMaxDialogButtons) {
+  if (button_list->children().size() > kMaxDialogButtons) {
     const int bubble_width = ChromeLayoutProvider::Get()->GetDistanceMetric(
         views::DISTANCE_BUBBLE_PREFERRED_WIDTH);
 
     int max_list_height = 0;
-    for (size_t i = 0; i < kMaxDialogButtons; ++i)
-      max_list_height += dialog_buttons_[i]->GetHeightForWidth(bubble_width);
+    for (size_t i = 0; i < kMaxDialogButtons; ++i) {
+      max_list_height +=
+          button_list->children()[i]->GetHeightForWidth(bubble_width);
+    }
     DCHECK_GT(max_list_height, 0);
 
     auto* scroll_view = AddChildView(std::make_unique<views::ScrollView>());
     scroll_view->ClipHeightTo(0, max_list_height);
-    scroll_view->SetContents(std::move(button_list));
+    button_list_ = scroll_view->SetContents(std::move(button_list));
   } else {
-    AddChildView(std::move(button_list));
+    button_list_ = AddChildView(std::move(button_list));
   }
 }
 

chrome/browser/ui/views/sharing/sharing_dialog_view.h

@@ -18,7 +18,6 @@ class StyledLabel;
 class View;
 }  // namespace views
 
-class HoverButton;
 enum class SharingDialogType;
 
 class SharingDialogView : public SharingDialog,
@@ -46,22 +45,15 @@ class SharingDialogView : public SharingDialog,
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
   static views::BubbleDialogDelegateView* GetAsBubble(SharingDialog* dialog);
-
   static views::BubbleDialogDelegateView* GetAsBubbleForClickToCall(
       SharingDialog* dialog);
 
- private:
-  friend class SharingDialogViewTest;
-  FRIEND_TEST_ALL_PREFIXES(SharingDialogViewTest, PopulateDialogView);
-  FRIEND_TEST_ALL_PREFIXES(SharingDialogViewTest, DevicePressed);
-  FRIEND_TEST_ALL_PREFIXES(SharingDialogViewTest, AppPressed);
-  FRIEND_TEST_ALL_PREFIXES(SharingDialogViewTest, HelpTextClickedEmpty);
-  FRIEND_TEST_ALL_PREFIXES(SharingDialogViewTest, HelpTextClickedOnlyApps);
-  FRIEND_TEST_ALL_PREFIXES(SharingDialogViewTest, ThemeChangedEmptyList);
+  SharingDialogType GetDialogType() const;
 
-  FRIEND_TEST_ALL_PREFIXES(ClickToCallBrowserTest, LeftClick_ChooseDevice);
+  const View* button_list_for_testing() const { return button_list_; }
 
-  SharingDialogType GetDialogType() const;
+ private:
+  friend class SharingDialogViewTest;
 
   // LocationBarBubbleDelegateView:
   void Init() override;
@@ -76,7 +68,7 @@ class SharingDialogView : public SharingDialog,
   SharingDialogData data_;
 
   // References to device and app buttons views.
-  std::vector<HoverButton*> dialog_buttons_;
+  View* button_list_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(SharingDialogView);
 };