Sharing dialog cleanup.

Follow-up CL with some cleanup after crrev.com/c/1789149.

Bug: None
Change-Id: I0301aba1e20df1fdc8681e8008e86e37ac703517
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1852425Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705492}

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc

@@ -382,15 +382,16 @@ void HandleDeviceSelection(
   if (!web_contents)
     return;
 
-  for (const auto& device : devices) {
-    if (device->guid() == device_guid) {
-      ClickToCallUiController::GetOrCreateFromWebContents(web_contents)
-          ->OnDeviceSelected(GetUnescapedURLContent(url), *device,
-                             SharingClickToCallEntryPoint::kLeftClickLink);
-      return;
-    }
-  }
-  NOTREACHED();
+  const auto it = std::find_if(devices.begin(), devices.end(),
+                               [&device_guid](const auto& device) {
+                                 return device->guid() == device_guid;
+                               });
+  DCHECK(it != devices.end());
+  auto* device = it->get();
+
+  ClickToCallUiController::GetOrCreateFromWebContents(web_contents)
+      ->OnDeviceSelected(GetUnescapedURLContent(url), *device,
+                         SharingClickToCallEntryPoint::kLeftClickLink);
 }
 
 // Handles |url| if possible. Returns true if it is actually handled.

chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog_unittest.cc

@@ -86,23 +86,19 @@ class MockSharingService : public SharingService {
  public:
   MockSharingService()
       : SharingService(
-            /*sync_prefs=*/nullptr,
-            /*vapid_key_manager=*/nullptr,
-            std::make_unique<SharingDeviceRegistration>(
-                /*pref_service=*/nullptr,
-                /*sync_preference=*/nullptr,
-                /*instance_id_driver=*/nullptr,
-                /*vapid_key_manager=*/nullptr),
-            /*fcm_sender=*/nullptr,
-            std::make_unique<SharingFCMHandler>(
-                /*gcm_driver=*/nullptr,
-                /*sharing_fcm_sender=*/nullptr,
-                /*sync_preference=*/nullptr),
-            /*gcm_driver=*/nullptr,
-            /*device_info_tracker=*/nullptr,
-            /*local_device_info_provider=*/nullptr,
-            /*sync_service=*/nullptr,
-            /*notification_display_service=*/nullptr) {}
+            nullptr,
+            nullptr,
+            std::make_unique<SharingDeviceRegistration>(nullptr,
+                                                        nullptr,
+                                                        nullptr,
+                                                        nullptr),
+            nullptr,
+            std::make_unique<SharingFCMHandler>(nullptr, nullptr, nullptr),
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr) {}
 
   ~MockSharingService() override = default;
 
@@ -1018,7 +1014,7 @@ TEST_F(ArcExternalProtocolDialogTestUtils, TestSelectDeviceForTelLink) {
   std::vector<mojom::IntentHandlerInfoPtr> handlers;
   std::vector<std::unique_ptr<syncer::DeviceInfo>> devices;
 
-  devices.emplace_back(std::make_unique<syncer::DeviceInfo>(
+  devices.push_back(std::make_unique<syncer::DeviceInfo>(
       device_guid, "device_name", "chrome_version", "user_agent",
       sync_pb::SyncEnums_DeviceType_TYPE_PHONE, "device_id",
       base::SysInfo::HardwareInfo(),

chrome/browser/sharing/click_to_call/click_to_call_context_menu_observer.cc

@@ -130,7 +130,7 @@ bool ClickToCallContextMenuObserver::IsCommandIdEnabled(int command_id) {
 
 void ClickToCallContextMenuObserver::ExecuteCommand(int command_id) {
   if (command_id == IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_SINGLE_DEVICE) {
-    DCHECK(devices_.size() == 1);
+    DCHECK_EQ(1u, devices_.size());
     SendClickToCallMessage(0);
   }
 }
@@ -138,7 +138,7 @@ void ClickToCallContextMenuObserver::ExecuteCommand(int command_id) {
 void ClickToCallContextMenuObserver::SendClickToCallMessage(
     int chosen_device_index) {
   DCHECK(entry_point_);
-  if (chosen_device_index >= static_cast<int>(devices_.size()))
+  if (size_t{chosen_device_index} >= devices_.size())
     return;
 
   LogSharingSelectedDeviceIndex(controller_->GetFeatureMetricsPrefix(),

chrome/browser/sharing/shared_clipboard/shared_clipboard_context_menu_observer.cc

@@ -127,14 +127,14 @@ bool SharedClipboardContextMenuObserver::IsCommandIdEnabled(int command_id) {
 void SharedClipboardContextMenuObserver::ExecuteCommand(int command_id) {
   if (command_id ==
       IDC_CONTENT_CONTEXT_SHARING_SHARED_CLIPBOARD_SINGLE_DEVICE) {
-    DCHECK(devices_.size() == 1);
+    DCHECK_EQ(1u, devices_.size());
     SendSharedClipboardMessage(0);
   }
 }
 
 void SharedClipboardContextMenuObserver::SendSharedClipboardMessage(
     int chosen_device_index) {
-  if (chosen_device_index >= static_cast<int>(devices_.size()))
+  if (size_t{chosen_device_index} >= devices_.size())
     return;
   LogSharingSelectedDeviceIndex(controller_->GetFeatureMetricsPrefix(),
                                 nullptr /* No suffix */, chosen_device_index);

chrome/browser/sharing/sharing_dialog_data.cc

@@ -9,3 +9,6 @@ SharingDialogData::SharingDialogData() = default;
 SharingDialogData::~SharingDialogData() = default;
 
 SharingDialogData::SharingDialogData(SharingDialogData&& other) = default;
+
+SharingDialogData& SharingDialogData::operator=(SharingDialogData&& other) =
+    default;

chrome/browser/sharing/sharing_dialog_data.h

@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "base/callback_forward.h"
-#include "base/macros.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/sharing/sharing_app.h"
 #include "chrome/browser/sharing/sharing_metrics.h"
@@ -23,6 +22,7 @@ struct SharingDialogData {
   SharingDialogData();
   ~SharingDialogData();
   SharingDialogData(SharingDialogData&& other);
+  SharingDialogData& operator=(SharingDialogData&& other);
 
   SharingDialogType type = SharingDialogType::kErrorDialog;
   SharingFeatureName prefix = SharingFeatureName::kUnknown;
@@ -41,9 +41,6 @@ struct SharingDialogData {
   base::OnceCallback<void(const syncer::DeviceInfo&)> device_callback;
   base::OnceCallback<void(const SharingApp&)> app_callback;
   base::OnceCallback<void(SharingDialog*)> close_callback;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SharingDialogData);
 };
 
 #endif  // CHROME_BROWSER_SHARING_SHARING_DIALOG_DATA_H_

chrome/browser/sharing/sharing_ui_controller.cc

@@ -49,19 +49,99 @@ SharingUiController::SharingUiController(content::WebContents* web_contents)
 
 SharingUiController::~SharingUiController() = default;
 
-void SharingUiController::CloseDialog() {
-  if (!dialog_)
+base::string16 SharingUiController::GetTitle(SharingDialogType dialog_type) {
+  // We only handle error messages generically.
+  DCHECK_EQ(SharingDialogType::kErrorDialog, dialog_type);
+  switch (send_result()) {
+    case SharingSendMessageResult::kDeviceNotFound:
+    case SharingSendMessageResult::kNetworkError:
+    case SharingSendMessageResult::kAckTimeout:
+      return l10n_util::GetStringFUTF16(
+          IDS_BROWSER_SHARING_ERROR_DIALOG_TITLE_GENERIC_ERROR,
+          base::ToLowerASCII(GetContentType()));
+
+    case SharingSendMessageResult::kSuccessful:
+      NOTREACHED();
+      FALLTHROUGH;
+
+    case SharingSendMessageResult::kPayloadTooLarge:
+    case SharingSendMessageResult::kInternalError:
+      return l10n_util::GetStringFUTF16(
+          IDS_BROWSER_SHARING_ERROR_DIALOG_TITLE_INTERNAL_ERROR,
+          base::ToLowerASCII(GetContentType()));
+  }
+}
+
+base::string16 SharingUiController::GetErrorDialogText() const {
+  switch (send_result()) {
+    case SharingSendMessageResult::kDeviceNotFound:
+      return l10n_util::GetStringFUTF16(
+          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_DEVICE_NOT_FOUND,
+          GetTargetDeviceName());
+
+    case SharingSendMessageResult::kNetworkError:
+      return l10n_util::GetStringUTF16(
+          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_NETWORK_ERROR);
+
+    case SharingSendMessageResult::kAckTimeout:
+      return l10n_util::GetStringFUTF16(
+          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_DEVICE_ACK_TIMEOUT,
+          GetTargetDeviceName());
+
+    case SharingSendMessageResult::kSuccessful:
+      return base::string16();
+
+    case SharingSendMessageResult::kPayloadTooLarge:
+    case SharingSendMessageResult::kInternalError:
+      return l10n_util::GetStringUTF16(
+          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_INTERNAL_ERROR);
+  }
+}
+
+void SharingUiController::OnDialogClosed(SharingDialog* dialog) {
+  // Ignore already replaced dialogs.
+  if (dialog != dialog_)
     return;
 
-  dialog_->Hide();
+  dialog_ = nullptr;
+  UpdateIcon();
+}
 
-  // SharingDialog::Hide may close the dialog asynchronously, and therefore not
-  // call OnDialogClosed immediately. If that is the case, call OnDialogClosed
-  // now to notify subclasses and clear |dialog_|.
-  if (dialog_)
-    OnDialogClosed(dialog_);
+void SharingUiController::OnHelpTextClicked(SharingDialogType dialog_type) {
+  ShowSingletonTab(chrome::FindBrowserWithWebContents(web_contents()),
+                   GURL(chrome::kSyncLearnMoreURL));
+}
 
-  DCHECK(!dialog_);
+void SharingUiController::OnDialogShown(bool has_devices, bool has_apps) {
+  if (on_dialog_shown_closure_for_testing_)
+    std::move(on_dialog_shown_closure_for_testing_).Run();
+}
+
+void SharingUiController::ClearLastDialog() {
+  last_dialog_id_++;
+  is_loading_ = false;
+  send_result_ = SharingSendMessageResult::kSuccessful;
+  CloseDialog();
+}
+
+void SharingUiController::UpdateAndShowDialog() {
+  ClearLastDialog();
+  DoUpdateApps(base::BindOnce(&SharingUiController::OnAppsReceived,
+                              weak_ptr_factory_.GetWeakPtr(), last_dialog_id_));
+}
+
+std::vector<std::unique_ptr<syncer::DeviceInfo>>
+SharingUiController::GetDevices() {
+  return sharing_service_->GetDeviceCandidates(GetRequiredFeature());
+}
+
+bool SharingUiController::HasSendFailed() const {
+  return send_result_ != SharingSendMessageResult::kSuccessful;
+}
+
+void SharingUiController::MaybeShowErrorDialog() {
+  if (HasSendFailed() && web_contents_ == GetCurrentWebContents(web_contents_))
+    ShowNewDialog(CreateDialogData(SharingDialogType::kErrorDialog));
 }
 
 SharingDialogData SharingUiController::CreateDialogData(
@@ -85,40 +165,6 @@ SharingDialogData SharingUiController::CreateDialogData(
   return data;
 }
 
-void SharingUiController::ShowNewDialog(SharingDialogData dialog_data) {
-  CloseDialog();
-  BrowserWindow* window = GetWindowFromWebContents(web_contents_);
-  if (!window)
-    return;
-  bool has_devices = !dialog_data.devices.empty();
-  bool has_apps = !dialog_data.apps.empty();
-  dialog_ = window->ShowSharingDialog(web_contents(), std::move(dialog_data));
-  UpdateIcon();
-  OnDialogShown(has_devices, has_apps);
-}
-
-void SharingUiController::UpdateIcon() {
-  BrowserWindow* window = GetWindowFromWebContents(web_contents_);
-  if (!window)
-    return;
-
-  window->UpdatePageActionIcon(GetIconType());
-}
-
-void SharingUiController::OnDialogClosed(SharingDialog* dialog) {
-  // Ignore already replaced dialogs.
-  if (dialog != dialog_)
-    return;
-
-  dialog_ = nullptr;
-  UpdateIcon();
-}
-
-void SharingUiController::MaybeShowErrorDialog() {
-  if (HasSendFailed() && web_contents_ == GetCurrentWebContents(web_contents_))
-    ShowNewDialog(CreateDialogData(SharingDialogType::kErrorDialog));
-}
-
 void SharingUiController::SendMessageToDevice(
     const syncer::DeviceInfo& device,
     chrome_browser_sharing::SharingMessage sharing_message) {
@@ -134,86 +180,54 @@ void SharingUiController::SendMessageToDevice(
                  weak_ptr_factory_.GetWeakPtr(), last_dialog_id_));
 }
 
-void SharingUiController::OnMessageSentToDevice(
-    int dialog_id,
-    SharingSendMessageResult result) {
-  if (dialog_id != last_dialog_id_)
+void SharingUiController::UpdateIcon() {
+  BrowserWindow* window = GetWindowFromWebContents(web_contents_);
+  if (!window)
     return;
 
-  is_loading_ = false;
-  send_result_ = result;
-  UpdateIcon();
+  window->UpdatePageActionIcon(GetIconType());
 }
 
-void SharingUiController::ClearLastDialog() {
-  last_dialog_id_++;
-  is_loading_ = false;
-  send_result_ = SharingSendMessageResult::kSuccessful;
-  CloseDialog();
-}
+void SharingUiController::CloseDialog() {
+  if (!dialog_)
+    return;
 
-void SharingUiController::UpdateAndShowDialog() {
-  ClearLastDialog();
-  DoUpdateApps(base::BindOnce(&SharingUiController::OnAppsReceived,
-                              weak_ptr_factory_.GetWeakPtr(), last_dialog_id_));
+  dialog_->Hide();
+
+  // SharingDialog::Hide may close the dialog asynchronously, and therefore not
+  // call OnDialogClosed immediately. If that is the case, call OnDialogClosed
+  // now to notify subclasses and clear |dialog_|.
+  if (dialog_)
+    OnDialogClosed(dialog_);
+
+  DCHECK(!dialog_);
 }
 
-std::vector<std::unique_ptr<syncer::DeviceInfo>>
-SharingUiController::GetDevices() {
-  return sharing_service_->GetDeviceCandidates(GetRequiredFeature());
+void SharingUiController::ShowNewDialog(SharingDialogData dialog_data) {
+  CloseDialog();
+  BrowserWindow* window = GetWindowFromWebContents(web_contents_);
+  if (!window)
+    return;
+  bool has_devices = !dialog_data.devices.empty();
+  bool has_apps = !dialog_data.apps.empty();
+  dialog_ = window->ShowSharingDialog(web_contents(), std::move(dialog_data));
+  UpdateIcon();
+  OnDialogShown(has_devices, has_apps);
 }
 
 base::string16 SharingUiController::GetTargetDeviceName() const {
   return base::UTF8ToUTF16(target_device_name_);
 }
 
-base::string16 SharingUiController::GetTitle(SharingDialogType dialog_type) {
-  // We only handle error messages generically.
-  DCHECK_EQ(SharingDialogType::kErrorDialog, dialog_type);
-  switch (send_result()) {
-    case SharingSendMessageResult::kDeviceNotFound:
-    case SharingSendMessageResult::kNetworkError:
-    case SharingSendMessageResult::kAckTimeout:
-      return l10n_util::GetStringFUTF16(
-          IDS_BROWSER_SHARING_ERROR_DIALOG_TITLE_GENERIC_ERROR,
-          base::ToLowerASCII(GetContentType()));
-
-    case SharingSendMessageResult::kSuccessful:
-      NOTREACHED();
-      FALLTHROUGH;
-
-    case SharingSendMessageResult::kPayloadTooLarge:
-    case SharingSendMessageResult::kInternalError:
-      return l10n_util::GetStringFUTF16(
-          IDS_BROWSER_SHARING_ERROR_DIALOG_TITLE_INTERNAL_ERROR,
-          base::ToLowerASCII(GetContentType()));
-  }
-}
-
-base::string16 SharingUiController::GetErrorDialogText() const {
-  switch (send_result()) {
-    case SharingSendMessageResult::kDeviceNotFound:
-      return l10n_util::GetStringFUTF16(
-          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_DEVICE_NOT_FOUND,
-          GetTargetDeviceName());
-
-    case SharingSendMessageResult::kNetworkError:
-      return l10n_util::GetStringUTF16(
-          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_NETWORK_ERROR);
-
-    case SharingSendMessageResult::kAckTimeout:
-      return l10n_util::GetStringFUTF16(
-          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_DEVICE_ACK_TIMEOUT,
-          GetTargetDeviceName());
-
-    case SharingSendMessageResult::kSuccessful:
-      return base::string16();
+void SharingUiController::OnMessageSentToDevice(
+    int dialog_id,
+    SharingSendMessageResult result) {
+  if (dialog_id != last_dialog_id_)
+    return;
 
-    case SharingSendMessageResult::kPayloadTooLarge:
-    case SharingSendMessageResult::kInternalError:
-      return l10n_util::GetStringUTF16(
-          IDS_BROWSER_SHARING_ERROR_DIALOG_TEXT_INTERNAL_ERROR);
-  }
+  is_loading_ = false;
+  send_result_ = result;
+  UpdateIcon();
 }
 
 void SharingUiController::OnAppsReceived(int dialog_id,
@@ -230,17 +244,3 @@ void SharingUiController::OnAppsReceived(int dialog_id,
 
   ShowNewDialog(std::move(dialog_data));
 }
-
-bool SharingUiController::HasSendFailed() const {
-  return send_result_ != SharingSendMessageResult::kSuccessful;
-}
-
-void SharingUiController::OnHelpTextClicked(SharingDialogType dialog_type) {
-  ShowSingletonTab(chrome::FindBrowserWithWebContents(web_contents()),
-                   GURL(chrome::kSyncLearnMoreURL));
-}
-
-void SharingUiController::OnDialogShown(bool has_devices, bool has_apps) {
-  if (on_dialog_shown_closure_for_testing_)
-    std::move(on_dialog_shown_closure_for_testing_).Run();
-}

chrome/browser/sharing/sharing_ui_controller.h

@@ -57,9 +57,18 @@ class SharingUiController {
   virtual base::string16 GetTextForTooltipAndAccessibleName() const = 0;
   // Get the name of the feature to be used as a prefix for the metric name.
   virtual SharingFeatureName GetFeatureMetricsPrefix() const = 0;
+  // Describes the content type of shared data.
+  virtual base::string16 GetContentType() const = 0;
+  // Returns the message to be shown in the body of error dialog based on
+  // |send_result_|.
+  virtual base::string16 GetErrorDialogText() const;
 
   // Called by the SharingDialog when it is being closed.
   virtual void OnDialogClosed(SharingDialog* dialog);
+  // Called by the SharingDialogView when the help text got clicked.
+  virtual void OnHelpTextClicked(SharingDialogType dialog_type);
+  // Called when a new dialog is shown.
+  virtual void OnDialogShown(bool has_devices, bool has_apps);
 
   // Closes the current dialog and resets all state.
   void ClearLastDialog();
@@ -70,12 +79,9 @@ class SharingUiController {
   // Gets the current list of devices that support the required feature.
   std::vector<std::unique_ptr<syncer::DeviceInfo>> GetDevices();
 
-  // Describes the content type of shared data.
-  virtual base::string16 GetContentType() const = 0;
+  bool HasSendFailed() const;
 
-  // Returns the message to be shown in the body of error dialog based on
-  // |send_result_|.
-  virtual base::string16 GetErrorDialogText() const;
+  void MaybeShowErrorDialog();
 
   // Returns the currently open SharingDialog or nullptr if there is no
   // dialog open.
@@ -85,33 +91,21 @@ class SharingUiController {
   void set_send_result_for_testing(SharingSendMessageResult result) {
     send_result_ = result;
   }
-
-  bool HasSendFailed() const;
-
   content::WebContents* web_contents() const { return web_contents_; }
 
-  void MaybeShowErrorDialog();
-
-  // Called by the SharingDialogView when the help text got clicked.
-  virtual void OnHelpTextClicked(SharingDialogType dialog_type);
-
-  // Called when a new dialog is shown.
-  virtual void OnDialogShown(bool has_devices, bool has_apps);
-
   void set_on_dialog_shown_closure_for_testing(base::OnceClosure closure) {
     on_dialog_shown_closure_for_testing_ = std::move(closure);
   }
 
  protected:
   virtual void DoUpdateApps(UpdateAppsCallback callback) = 0;
+  // Prepares a new dialog data.
+  virtual SharingDialogData CreateDialogData(SharingDialogType dialog_type);
 
   void SendMessageToDevice(
       const syncer::DeviceInfo& device,
       chrome_browser_sharing::SharingMessage sharing_message);
 
-  // Prepares a new dialog data.
-  virtual SharingDialogData CreateDialogData(SharingDialogType dialog_type);
-
  private:
   // Updates the omnibox icon if available.
   void UpdateIcon();

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

@@ -19,6 +19,7 @@
 #include "components/sync_device_info/device_info.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/border.h"
@@ -171,8 +172,7 @@ void SharingDialogView::ButtonPressed(views::Button* sender,
   DCHECK(data_.app_callback);
   if (!sender || sender->tag() < 0)
     return;
-  size_t index = static_cast<size_t>(sender->tag());
-  DCHECK(index < data_.devices.size() + data_.apps.size());
+  size_t index{sender->tag()};
 
   if (index < data_.devices.size()) {
     LogSharingSelectedDeviceIndex(data_.prefix, kSharingUiDialog, index);
@@ -195,7 +195,7 @@ void SharingDialogView::MaybeShowHeaderImage() {
   if (!frame_view)
     return;
 
-  int image_id = GetNativeTheme()->ShouldUseDarkColors()
+  int image_id = color_utils::IsDark(frame_view->GetBackgroundColor())
                      ? data_.header_image_dark
                      : data_.header_image_light;
   if (!image_id) {

ui/views/bubble/bubble_frame_view.cc

@@ -511,6 +511,10 @@ void BubbleFrameView::SetBackgroundColor(SkColor color) {
   SchedulePaint();
 }
 
+SkColor BubbleFrameView::GetBackgroundColor() const {
+  return bubble_border_->background_color();
+}
+
 gfx::Rect BubbleFrameView::GetUpdatedWindowBounds(
     const gfx::Rect& anchor_rect,
     const BubbleBorder::Arrow delegate_arrow,

ui/views/bubble/bubble_frame_view.h

@@ -135,6 +135,7 @@ class VIEWS_EXPORT BubbleFrameView : public NonClientFrameView,
 
   // Set the background color of the bubble border.
   void SetBackgroundColor(SkColor color);
+  SkColor GetBackgroundColor() const;
 
   // Given the size of the contents and the rect to point at, returns the bounds
   // of the bubble window. The bubble's arrow location may change if the bubble