Improve Remote Copy notifications on Windows

Because of crbug.com/1064558 we can not update Windows native
notifications. Instead, we'll just show an initial indefinite
progress notification and replace it with the final image after
all processing is done.

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

chrome/browser/notifications/win/notification_template_builder.cc

@@ -224,7 +224,8 @@ void WriteProgressElement(XmlWriter* xml_writer,
   // valueStringOverride: A string that replaces the percentage on the right.
   xml_writer->StartElement(kProgress);
   // Status is mandatory, without it the progress bar is not shown.
-  xml_writer->AddAttribute(kStatus, std::string());
+  xml_writer->AddAttribute(kStatus,
+                           base::UTF16ToUTF8(notification.progress_status()));
 
   // Show indeterminate spinner for values outside the [0-100] range.
   if (notification.progress() < 0 || notification.progress() > 100) {

chrome/browser/sharing/shared_clipboard/remote_copy_message_handler.cc

@@ -53,6 +53,10 @@
 #include "ui/message_center/public/cpp/notification_types.h"
 #include "ui/message_center/public/cpp/notifier_id.h"
 
+#if defined(OS_WIN)
+#include "chrome/browser/notifications/notification_platform_bridge_win.h"
+#endif  // defined(OS_WIN)
+
 namespace {
 constexpr size_t kMaxImageDownloadSize = 5 * 1024 * 1024;
 
@@ -146,6 +150,17 @@ base::string16 GetProgressString(int64_t current, int64_t total) {
                                     total_string);
 }
 
+bool CanUpdateProgressNotification() {
+#if defined(OS_WIN)
+  // TODO(crbug.com/1064558): Windows native notifications don't support updates
+  // so only show the initial progress notification and replace it with the
+  // final image notification at the end.
+  if (NotificationPlatformBridgeWin::NativeNotificationEnabled())
+    return false;
+#endif  // defined(OS_WIN)
+  return true;
+}
+
 }  // namespace
 
 RemoteCopyMessageHandler::RemoteCopyMessageHandler(Profile* profile)
@@ -228,11 +243,14 @@ void RemoteCopyMessageHandler::HandleImage(const std::string& image_url) {
 
   bool should_show_progress =
       base::FeatureList::IsEnabled(kRemoteCopyProgressNotification);
+  bool can_update_notification = CanUpdateProgressNotification();
 
   if (should_show_progress) {
     CancelProgressNotification();
     UpdateProgressNotification(l10n_util::GetStringUTF16(
-        IDS_SHARING_REMOTE_COPY_NOTIFICATION_PREPARING_DOWNLOAD));
+        can_update_notification
+            ? IDS_SHARING_REMOTE_COPY_NOTIFICATION_PREPARING_DOWNLOAD
+            : IDS_SHARING_REMOTE_COPY_NOTIFICATION_PROCESSING_IMAGE));
   }
 
   auto request = std::make_unique<network::ResourceRequest>();
@@ -246,7 +264,7 @@ void RemoteCopyMessageHandler::HandleImage(const std::string& image_url) {
       network::SimpleURLLoader::Create(std::move(request), kTrafficAnnotation);
   timer_ = base::ElapsedTimer();
   // Unretained(this) is safe here because |this| owns |url_loader_|.
-  if (should_show_progress) {
+  if (should_show_progress && can_update_notification) {
     url_loader_->SetOnResponseStartedCallback(
         base::BindOnce(&RemoteCopyMessageHandler::OnImageResponseStarted,
                        base::Unretained(this)));
@@ -357,6 +375,9 @@ void RemoteCopyMessageHandler::UpdateProgressNotification(
   NotificationDisplayServiceFactory::GetForProfile(profile_)->Display(
       NotificationHandler::Type::SHARING, notification, /*metadata=*/nullptr);
 
+  if (!CanUpdateProgressNotification())
+    return;
+
   // Unretained(this) is safe here because |this| owns
   // |image_download_update_progress_timer_|.
   image_download_update_progress_timer_.Start(
@@ -369,6 +390,7 @@ void RemoteCopyMessageHandler::CancelProgressNotification() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   image_content_length_ = -1;
   image_content_progress_ = 0;
+  progress_notification_closed_ = false;
 
   if (image_notification_id_.empty())
     return;
@@ -398,8 +420,7 @@ void RemoteCopyMessageHandler::OnProgressNotificationAction(
     SharingServiceFactory::GetForBrowserContext(profile_)
         ->SetNotificationActionHandler(image_notification_id_,
                                        base::NullCallback());
-    // The notification will be closed by the framework after this.
-    image_notification_id_.clear();
+    progress_notification_closed_ = true;
     return;
   }
 
@@ -412,13 +433,14 @@ void RemoteCopyMessageHandler::OnURLLoadComplete(
     std::unique_ptr<std::string> content) {
   TRACE_EVENT0("sharing", "RemoteCopyMessageHandler::OnURLLoadComplete");
 
-  if (!image_notification_id_.empty()) {
+  if (!progress_notification_closed_ && CanUpdateProgressNotification()) {
     image_content_length_ = -1;
     UpdateProgressNotification(l10n_util::GetStringUTF16(
         IDS_SHARING_REMOTE_COPY_NOTIFICATION_PROCESSING_IMAGE));
-    image_download_update_progress_timer_.AbandonAndStop();
   }
 
+  image_download_update_progress_timer_.AbandonAndStop();
+
   int code;
   if (url_loader_->NetError() != net::OK) {
     code = url_loader_->NetError();
@@ -520,7 +542,8 @@ void RemoteCopyMessageHandler::WriteImageAndShowNotification(
   // On macOS we can't replace a persistent notification with a non-persistent
   // one because they are posted from different sources (app vs xpc). To avoid
   // having both notifications on screen, remove the progress one first.
-  CancelProgressNotification();
+  if (!progress_notification_closed_)
+    CancelProgressNotification();
 #endif  // defined(OS_MACOSX)
 
   std::string notification_id = image_notification_id_;
@@ -580,6 +603,15 @@ void RemoteCopyMessageHandler::ShowNotification(
       rich_notification_data,
       /*delegate=*/nullptr);
 
+  if (!CanUpdateProgressNotification())
+    notification.set_renotify(true);
+
+  // Make the notification silent if we're replacing a progress notification.
+  bool should_show_progress =
+      base::FeatureList::IsEnabled(kRemoteCopyProgressNotification);
+  if (should_show_progress && !progress_notification_closed_)
+    notification.set_silent(true);
+
   NotificationDisplayServiceFactory::GetForProfile(profile_)->Display(
       NotificationHandler::Type::SHARING, notification, /*metadata=*/nullptr);
 }

chrome/browser/sharing/shared_clipboard/remote_copy_message_handler.h

@@ -74,6 +74,7 @@ class RemoteCopyMessageHandler : public SharingMessageHandler,
   int64_t image_content_length_ = -1;
   int64_t image_content_progress_ = 0;
   std::string image_notification_id_;
+  bool progress_notification_closed_ = false;
   base::OneShotTimer image_download_update_progress_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteCopyMessageHandler);

chrome/browser/sharing/shared_clipboard/remote_copy_message_handler_unittest.cc

@@ -34,6 +34,10 @@
 #include "ui/gfx/skia_util.h"
 #include "ui/message_center/public/cpp/notification.h"
 
+#if defined(OS_WIN)
+#include "chrome/browser/notifications/notification_platform_bridge_win.h"
+#endif  // defined(OS_WIN)
+
 namespace {
 
 const char kText[] = "clipboard text";
@@ -231,9 +235,17 @@ TEST_F(RemoteCopyMessageHandlerTest, ProgressNotificationWithProgressFlag) {
   base::string16 progress_status = notification.progress_status();
 #endif  // defined(OS_MACOSX)
 
-  EXPECT_EQ(l10n_util::GetStringUTF16(
-                IDS_SHARING_REMOTE_COPY_NOTIFICATION_PREPARING_DOWNLOAD),
-            progress_status);
+#if defined(OS_WIN)
+  base::string16 expected_status = l10n_util::GetStringUTF16(
+      NotificationPlatformBridgeWin::NativeNotificationEnabled()
+          ? IDS_SHARING_REMOTE_COPY_NOTIFICATION_PROCESSING_IMAGE
+          : IDS_SHARING_REMOTE_COPY_NOTIFICATION_PREPARING_DOWNLOAD);
+#else
+  base::string16 expected_status = l10n_util::GetStringUTF16(
+      IDS_SHARING_REMOTE_COPY_NOTIFICATION_PREPARING_DOWNLOAD);
+#endif  // defined(OS_WIN)
+
+  EXPECT_EQ(expected_status, progress_status);
   EXPECT_EQ(-1, notification.progress());
 
   // Calling GetDefaultStoragePartition creates tasks that need to run before