Add a Cancel button to remote copy notifications

This allows users to cancel downloading images.

Bug: 1059290
Change-Id: I5eeaf6ddecc22c28021161a15e61f6467b418bd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2096759
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749420}

chrome/browser/sharing/shared_clipboard/remote_copy_message_handler.cc

@@ -25,7 +25,10 @@
 #include "chrome/browser/sharing/proto/sharing_message.pb.h"
 #include "chrome/browser/sharing/shared_clipboard/feature_flags.h"
 #include "chrome/browser/sharing/sharing_metrics.h"
+#include "chrome/browser/sharing/sharing_service.h"
+#include "chrome/browser/sharing/sharing_service_factory.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/strings/grit/components_strings.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
 #include "net/base/load_flags.h"
@@ -154,10 +157,7 @@ void RemoteCopyMessageHandler::OnMessage(
 
   // First cancel any pending async tasks that might otherwise overwrite the
   // results of the more recent message.
-  url_loader_.reset();
-  ImageDecoder::Cancel(this);
-  resize_callback_.Cancel();
-  write_detection_timer_.AbandonAndStop();
+  CancelAsyncTasks();
 
   device_name_ = message.sender_device_name();
 
@@ -291,8 +291,17 @@ void RemoteCopyMessageHandler::OnImageDownloadProgress(uint64_t current) {
 void RemoteCopyMessageHandler::UpdateProgressNotification(
     const base::string16& context) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (image_notification_id_.empty())
+  if (image_notification_id_.empty()) {
     image_notification_id_ = base::GenerateGUID();
+    // base::Unretained is safe as the SharingService owns |this| via the
+    // SharingHandlerRegistry and also the passed callback.
+    SharingServiceFactory::GetForBrowserContext(profile_)
+        ->SetNotificationActionHandler(
+            image_notification_id_,
+            base::BindRepeating(
+                &RemoteCopyMessageHandler::OnProgressNotificationAction,
+                base::Unretained(this)));
+  }
 
   message_center::RichNotificationData rich_notification_data;
   rich_notification_data.vector_small_image = &kSendTabToSelfIcon;
@@ -307,7 +316,13 @@ void RemoteCopyMessageHandler::UpdateProgressNotification(
       /*display_source=*/base::string16(),
       /*origin_url=*/GURL(), message_center::NotifierId(),
       rich_notification_data,
-      base::MakeRefCounted<message_center::NotificationDelegate>());
+      /*delegate=*/nullptr);
+
+  std::vector<message_center::ButtonInfo> notification_actions;
+  message_center::ButtonInfo button_info =
+      message_center::ButtonInfo(l10n_util::GetStringUTF16(IDS_CANCEL));
+  notification_actions.push_back(button_info);
+  notification.set_buttons(notification_actions);
 
   if (image_content_length_ <= 0) {
     // TODO(knollr): Show transfer status if |image_content_progress_| is != 0.
@@ -323,7 +338,7 @@ void RemoteCopyMessageHandler::UpdateProgressNotification(
   }
 
   NotificationDisplayServiceFactory::GetForProfile(profile_)->Display(
-      NotificationHandler::Type::TRANSIENT, notification, /*metadata=*/nullptr);
+      NotificationHandler::Type::SHARING, notification, /*metadata=*/nullptr);
 
   // Unretained(this) is safe here because |this| owns
   // |image_download_update_progress_timer_|.
@@ -341,12 +356,41 @@ void RemoteCopyMessageHandler::CancelProgressNotification() {
   if (image_notification_id_.empty())
     return;
 
+  SharingServiceFactory::GetForBrowserContext(profile_)
+      ->SetNotificationActionHandler(image_notification_id_,
+                                     base::NullCallback());
   NotificationDisplayServiceFactory::GetForProfile(profile_)->Close(
       NotificationHandler::Type::SHARING, image_notification_id_);
 
   image_notification_id_.clear();
 }
 
+void RemoteCopyMessageHandler::OnProgressNotificationAction(
+    base::Optional<int> button,
+    bool closed) {
+  // Clicks on the progress notification body are ignored.
+  if (!closed && !button)
+    return;
+
+  // Stop updating the progress notification.
+  image_download_update_progress_timer_.AbandonAndStop();
+
+  // Let the download continue if the notification was dismissed.
+  if (closed) {
+    // Remove the handler as this notification is now closed.
+    SharingServiceFactory::GetForBrowserContext(profile_)
+        ->SetNotificationActionHandler(image_notification_id_,
+                                       base::NullCallback());
+    // The notification will be closed by the framework after this.
+    image_notification_id_.clear();
+    return;
+  }
+
+  // Cancel the download if the cancel button was pressed.
+  DCHECK_EQ(0, *button);
+  CancelAsyncTasks();
+}
+
 void RemoteCopyMessageHandler::OnURLLoadComplete(
     std::unique_ptr<std::string> content) {
   TRACE_EVENT0("sharing", "RemoteCopyMessageHandler::OnURLLoadComplete");
@@ -451,9 +495,14 @@ void RemoteCopyMessageHandler::WriteImageAndShowNotification(
                      base::TimeTicks::Now(), /*is_image=*/true));
 
   std::string notification_id = image_notification_id_;
-  if (notification_id.empty())
+  if (notification_id.empty()) {
     notification_id = base::GenerateGUID();
-  image_notification_id_.clear();
+  } else {
+    SharingServiceFactory::GetForBrowserContext(profile_)
+        ->SetNotificationActionHandler(image_notification_id_,
+                                       base::NullCallback());
+    image_notification_id_.clear();
+  }
 
   ShowNotification(GetImageNotificationTitle(device_name_), resized_image,
                    notification_id);
@@ -524,3 +573,12 @@ void RemoteCopyMessageHandler::Finish(RemoteCopyHandleMessageResult result) {
   LogRemoteCopyHandleMessageResult(result);
   device_name_.clear();
 }
+
+void RemoteCopyMessageHandler::CancelAsyncTasks() {
+  url_loader_.reset();
+  ImageDecoder::Cancel(this);
+  resize_callback_.Cancel();
+  write_detection_timer_.AbandonAndStop();
+  image_download_update_progress_timer_.AbandonAndStop();
+  CancelProgressNotification();
+}

chrome/browser/sharing/shared_clipboard/remote_copy_message_handler.h

@@ -10,6 +10,7 @@
 
 #include "base/cancelable_callback.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/timer/elapsed_timer.h"
 #include "base/timer/timer.h"
@@ -51,6 +52,7 @@ class RemoteCopyMessageHandler : public SharingMessageHandler,
   void OnImageDownloadProgress(uint64_t current);
   void UpdateProgressNotification(const base::string16& context);
   void CancelProgressNotification();
+  void OnProgressNotificationAction(base::Optional<int> button, bool closed);
   void OnURLLoadComplete(std::unique_ptr<std::string> content);
   void WriteImageAndShowNotification(const SkBitmap& original_image,
                                      const SkBitmap& resized_image);
@@ -61,6 +63,7 @@ class RemoteCopyMessageHandler : public SharingMessageHandler,
                    base::TimeTicks start_ticks,
                    bool is_image);
   void Finish(RemoteCopyHandleMessageResult result);
+  void CancelAsyncTasks();
 
   Profile* profile_ = nullptr;
   std::unique_ptr<network::SimpleURLLoader> url_loader_;

chrome/browser/sharing/shared_clipboard/remote_copy_message_handler_unittest.cc

@@ -13,11 +13,13 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/notifications/notification_display_service_tester.h"
+#include "chrome/browser/sharing/mock_sharing_service.h"
 #include "chrome/browser/sharing/proto/remote_copy_message.pb.h"
 #include "chrome/browser/sharing/proto/sharing_message.pb.h"
 #include "chrome/browser/sharing/shared_clipboard/feature_flags.h"
 #include "chrome/browser/sharing/shared_clipboard/remote_copy_handle_message_result.h"
 #include "chrome/browser/sharing/shared_clipboard/shared_clipboard_test_base.h"
+#include "chrome/browser/sharing/sharing_service_factory.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/sync/protocol/sync_enums.pb.h"
@@ -48,6 +50,11 @@ class RemoteCopyMessageHandlerTest : public SharedClipboardTestBase {
 
   void SetUp() override {
     SharedClipboardTestBase::SetUp();
+    SharingServiceFactory::GetInstance()->SetTestingFactory(
+        &profile_, base::BindRepeating([](content::BrowserContext* context)
+                                           -> std::unique_ptr<KeyedService> {
+          return std::make_unique<testing::NiceMock<MockSharingService>>();
+        }));
     message_handler_ = std::make_unique<RemoteCopyMessageHandler>(&profile_);
   }
 
@@ -243,6 +250,9 @@ TEST_F(RemoteCopyMessageHandlerTest, ImageNotificationWithProgressFlag) {
   // download.
   task_environment_.RunUntilIdle();
 
+  // After finishing the transfer there should be no progress notification.
+  EXPECT_FALSE(HasProgressNotification());
+
   // Expect the image to be in the clipboard now.
   SkBitmap image = GetClipboardImage();
   EXPECT_TRUE(gfx::BitmapsAreEqual(*image_, image));
@@ -251,3 +261,54 @@ TEST_F(RemoteCopyMessageHandlerTest, ImageNotificationWithProgressFlag) {
   auto notification = GetImageNotification();
   EXPECT_FALSE(notification.image().IsEmpty());
 }
+
+TEST_F(RemoteCopyMessageHandlerTest, CancelProgressNotification) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeaturesAndParameters(
+      {{kRemoteCopyReceiver, {{kRemoteCopyAllowedOrigins.name, kTestImageUrl}}},
+       {kRemoteCopyProgressNotification, {}}},
+      {});
+
+  message_handler_->OnMessage(CreateMessageWithImage(kTestImageUrl),
+                              base::DoNothing());
+  auto notification = GetProgressNotification();
+
+  // Simulate a click on the cancel button at index 0.
+  notification_tester_->SimulateClick(NotificationHandler::Type::SHARING,
+                                      notification.id(), /*action_index=*/0,
+                                      /*reply=*/base::nullopt);
+
+  // The progress notification should now be closed.
+  EXPECT_FALSE(HasProgressNotification());
+
+  // Run remaining tasks to ensure no notification is shown at the end.
+  task_environment_.RunUntilIdle();
+
+  EXPECT_FALSE(HasProgressNotification());
+  EXPECT_FALSE(HasImageNotification());
+}
+
+TEST_F(RemoteCopyMessageHandlerTest, DismissProgressNotification) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitWithFeaturesAndParameters(
+      {{kRemoteCopyReceiver, {{kRemoteCopyAllowedOrigins.name, kTestImageUrl}}},
+       {kRemoteCopyProgressNotification, {}}},
+      {});
+
+  message_handler_->OnMessage(CreateMessageWithImage(kTestImageUrl),
+                              base::DoNothing());
+  auto notification = GetProgressNotification();
+
+  // Simulate closing the notification by the user.
+  notification_tester_->RemoveNotification(NotificationHandler::Type::SHARING,
+                                           notification.id(), /*by_user=*/true,
+                                           /*silent=*/false);
+
+  // The progress notification should now be closed.
+  EXPECT_FALSE(HasProgressNotification());
+
+  // Let tasks run until the image is decoded and written to the clipboard.
+  task_environment_.RunUntilIdle();
+
+  EXPECT_TRUE(HasImageNotification());
+}

chrome/browser/sharing/shared_clipboard/shared_clipboard_test_base.cc

@@ -51,9 +51,18 @@ SkBitmap SharedClipboardTestBase::GetClipboardImage() {
       ui::ClipboardBuffer::kCopyPaste);
 }
 
+bool SharedClipboardTestBase::HasImageNotification() {
+  auto notifications = notification_tester_->GetDisplayedNotificationsForType(
+      NotificationHandler::Type::SHARING);
+  if (notifications.size() != 1u)
+    return false;
+
+  return notifications[0].type() == message_center::NOTIFICATION_TYPE_IMAGE;
+}
+
 bool SharedClipboardTestBase::HasProgressNotification() {
   auto notifications = notification_tester_->GetDisplayedNotificationsForType(
-      NotificationHandler::Type::TRANSIENT);
+      NotificationHandler::Type::SHARING);
   if (notifications.size() != 1u)
     return false;
 
@@ -74,7 +83,7 @@ message_center::Notification SharedClipboardTestBase::GetNotification() {
 message_center::Notification
 SharedClipboardTestBase::GetProgressNotification() {
   auto notifications = notification_tester_->GetDisplayedNotificationsForType(
-      NotificationHandler::Type::TRANSIENT);
+      NotificationHandler::Type::SHARING);
   EXPECT_EQ(notifications.size(), 1u);
 
   const message_center::Notification& notification = notifications[0];

chrome/browser/sharing/shared_clipboard/shared_clipboard_test_base.h

@@ -41,6 +41,7 @@ class SharedClipboardTestBase : public testing::Test {
   std::string GetClipboardText();
   SkBitmap GetClipboardImage();
 
+  bool HasImageNotification();
   bool HasProgressNotification();
 
   message_center::Notification GetNotification();

chrome/browser/sharing/sharing_notification_handler.cc

@@ -8,6 +8,9 @@
 
 #include "chrome/browser/notifications/notification_display_service.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sharing/sharing_service.h"
+#include "chrome/browser/sharing/sharing_service_factory.h"
 
 SharingNotificationHandler::SharingNotificationHandler() = default;
 SharingNotificationHandler::~SharingNotificationHandler() = default;
@@ -19,8 +22,27 @@ void SharingNotificationHandler::OnClick(
     const base::Optional<int>& action_index,
     const base::Optional<base::string16>& reply,
     base::OnceClosure completed_closure) {
-  NotificationDisplayServiceFactory::GetForProfile(profile)->Close(
-      NotificationHandler::Type::SHARING, notification_id);
+  auto handler = SharingServiceFactory::GetForBrowserContext(profile)
+                     ->GetNotificationActionHandler(notification_id);
+  if (handler) {
+    handler.Run(action_index, /*closed=*/false);
+  } else {
+    // Close the notification by default.
+    NotificationDisplayServiceFactory::GetForProfile(profile)->Close(
+        NotificationHandler::Type::SHARING, notification_id);
+  }
+  std::move(completed_closure).Run();
+}
+
+void SharingNotificationHandler::OnClose(Profile* profile,
+                                         const GURL& origin,
+                                         const std::string& notification_id,
+                                         bool by_user,
+                                         base::OnceClosure completed_closure) {
+  auto handler = SharingServiceFactory::GetForBrowserContext(profile)
+                     ->GetNotificationActionHandler(notification_id);
+  if (handler)
+    handler.Run(/*button=*/base::nullopt, /*closed=*/true);
   std::move(completed_closure).Run();
 }
 

chrome/browser/sharing/sharing_notification_handler.h

@@ -5,15 +5,23 @@
 #ifndef CHROME_BROWSER_SHARING_SHARING_NOTIFICATION_HANDLER_H_
 #define CHROME_BROWSER_SHARING_SHARING_NOTIFICATION_HANDLER_H_
 
-#include <map>
 #include <string>
 
+#include "base/callback_forward.h"
+#include "base/optional.h"
+#include "base/strings/string16.h"
 #include "chrome/browser/notifications/notification_handler.h"
+#include "url/gurl.h"
+
+class Profile;
 
 // Handles SHARING nofication actions.
 class SharingNotificationHandler : public NotificationHandler {
  public:
   SharingNotificationHandler();
+  SharingNotificationHandler(const SharingNotificationHandler&) = delete;
+  SharingNotificationHandler& operator=(const SharingNotificationHandler&) =
+      delete;
   ~SharingNotificationHandler() override;
 
   // NotificationHandler implementation:
@@ -23,10 +31,12 @@ class SharingNotificationHandler : public NotificationHandler {
                const base::Optional<int>& action_index,
                const base::Optional<base::string16>& reply,
                base::OnceClosure completed_closure) override;
+  void OnClose(Profile* profile,
+               const GURL& origin,
+               const std::string& notification_id,
+               bool by_user,
+               base::OnceClosure completed_closure) override;
   void OpenSettings(Profile* profile, const GURL& origin) override;
-
- protected:
-  DISALLOW_COPY_AND_ASSIGN(SharingNotificationHandler);
 };
 
-#endif  // CHROME_BROWSER_SHARING_SHARING_NOTIFICATION_HANDLER_H_
+#endif  // CHROME_BROWSER_SHARING_SHARING_NOTIFICATION_HANDLER_H_
\ No newline at end of file

chrome/browser/sharing/sharing_service.cc

@@ -130,6 +130,24 @@ void SharingService::UnregisterSharingHandler(
   handler_registry_->UnregisterSharingHandler(payload_case);
 }
 
+void SharingService::SetNotificationActionHandler(
+    const std::string& notification_id,
+    NotificationActionCallback callback) {
+  if (callback)
+    notification_action_handlers_[notification_id] = callback;
+  else
+    notification_action_handlers_.erase(notification_id);
+}
+
+SharingService::NotificationActionCallback
+SharingService::GetNotificationActionHandler(
+    const std::string& notification_id) const {
+  auto iter = notification_action_handlers_.find(notification_id);
+  return iter == notification_action_handlers_.end()
+             ? NotificationActionCallback()
+             : iter->second;
+}
+
 SharingDeviceSource* SharingService::GetDeviceSource() const {
   return device_source_.get();
 }

chrome/browser/sharing/sharing_service.h

@@ -44,6 +44,8 @@ enum class SharingDeviceRegistrationResult;
 class SharingService : public KeyedService, public syncer::SyncServiceObserver {
  public:
   using SharingDeviceList = std::vector<std::unique_ptr<syncer::DeviceInfo>>;
+  using NotificationActionCallback =
+      base::RepeatingCallback<void(base::Optional<int> button, bool closed)>;
 
   enum class State {
     // Device is unregistered with FCM and Sharing is unavailable.
@@ -100,6 +102,18 @@ class SharingService : public KeyedService, public syncer::SyncServiceObserver {
   void UnregisterSharingHandler(
       chrome_browser_sharing::SharingMessage::PayloadCase payload_case);
 
+  // Sets a notification action handler for |notification_id|. Replaces any
+  // previously set handlers for |notification_id|. |callback| may be a null
+  // callback which clears the handler for |notification_id|.
+  void SetNotificationActionHandler(const std::string& notification_id,
+                                    NotificationActionCallback callback);
+
+  // Returns the notification action handler for |notification_id| set by
+  // SetNotificationActionHandler(). The returned callback may be null if no
+  // handler has been set before for |notification_id|.
+  NotificationActionCallback GetNotificationActionHandler(
+      const std::string& notification_id) const;
+
   // Used to register devices with required capabilities in tests.
   void RegisterDeviceInTesting(
       std::set<sync_pb::SharingSpecificFields_EnabledFeatures> enabled_features,
@@ -149,6 +163,10 @@ class SharingService : public KeyedService, public syncer::SyncServiceObserver {
   SharingServiceProxyAndroid sharing_service_proxy_android_{this};
 #endif  // defined(OS_ANDROID)
 
+  // Map of notification id to notification handler callback.
+  std::map<std::string, NotificationActionCallback>
+      notification_action_handlers_;
+
   base::WeakPtrFactory<SharingService> weak_ptr_factory_{this};
 };