fix: forward close event if we show a close button explicitely

This makes sure that we forward the close event when a user clicks on
the close button we show in native linux notifications for cinnamon.

Bug: 924478
Change-Id: I490d9250c85dbaaca1132bc75a273e2c070c2ad8
Reviewed-on: https://chromium-review.googlesource.com/c/1429044
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626077}

chrome/browser/notifications/notification_platform_bridge_linux.cc

@@ -808,6 +808,9 @@ class NotificationPlatformBridgeLinuxImpl
           FROM_HERE, data, NotificationCommon::OPERATION_SETTINGS,
           base::nullopt /* action_index */, base::nullopt /* by_user */);
     } else if (action == kCloseButtonId) {
+      ForwardNotificationOperation(
+          FROM_HERE, data, NotificationCommon::OPERATION_CLOSE,
+          base::nullopt /* action_index */, true /* by_user */);
       CloseOnTaskRunner(data->profile_id, data->notification_id);
     } else {
       size_t id;

chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc

@@ -15,6 +15,8 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/dbus/dbus_thread_linux.h"
+#include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/browser/notifications/notification_test_util.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "content/public/test/test_utils.h"
@@ -26,6 +28,7 @@
 #include "ui/message_center/public/cpp/notification.h"
 #include "ui/message_center/public/cpp/notification_delegate.h"
 
+using message_center::ButtonInfo;
 using message_center::Notification;
 using testing::_;
 using testing::ByMove;
@@ -99,6 +102,13 @@ class NotificationBuilder {
     return *this;
   }
 
+  NotificationBuilder& AddButton(const ButtonInfo& button) {
+    auto buttons = notification_.buttons();
+    buttons.push_back(button);
+    notification_.set_buttons(buttons);
+    return *this;
+  }
+
  private:
   Notification notification_;
 };
@@ -278,6 +288,12 @@ class NotificationPlatformBridgeLinuxTest : public BrowserWithTestWindowTest {
 
   void SetUp() override {
     BrowserWithTestWindowTest::SetUp();
+    display_service_tester_ =
+        std::make_unique<NotificationDisplayServiceTester>(profile());
+    display_service_tester_->SetProcessNotificationOperationDelegate(
+        base::BindRepeating(
+            &NotificationPlatformBridgeLinuxTest::HandleOperation,
+            base::Unretained(this)));
     mock_bus_ = new dbus::MockBus(dbus::Bus::Options());
     mock_notification_proxy_ = new StrictMock<dbus::MockObjectProxy>(
         mock_bus_.get(), kFreedesktopNotificationsName,
@@ -288,11 +304,23 @@ class NotificationPlatformBridgeLinuxTest : public BrowserWithTestWindowTest {
     notification_bridge_linux_->CleanUp();
     content::RunAllTasksUntilIdle();
     notification_bridge_linux_.reset();
+    display_service_tester_.reset();
     mock_notification_proxy_ = nullptr;
     mock_bus_ = nullptr;
     BrowserWithTestWindowTest::TearDown();
   }
 
+  void HandleOperation(NotificationCommon::Operation operation,
+                       NotificationHandler::Type notification_type,
+                       const GURL& origin,
+                       const std::string& notification_id,
+                       const base::Optional<int>& action_index,
+                       const base::Optional<base::string16>& reply,
+                       const base::Optional<bool>& by_user) {
+    last_operation_ = operation;
+    last_action_index_ = action_index;
+  }
+
  protected:
   void CreateNotificationBridgeLinux(const TestParams& test_params) {
     EXPECT_CALL(*mock_bus_.get(),
@@ -340,6 +368,13 @@ class NotificationPlatformBridgeLinuxTest : public BrowserWithTestWindowTest {
     content::RunAllTasksUntilIdle();
   }
 
+  void InvokeAction(uint32_t dbus_id, const std::string& action) {
+    chrome::GetDBusTaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&NotificationPlatformBridgeLinuxTest::DoInvokeAction,
+                       base::Unretained(this), dbus_id, action));
+  }
+
   MOCK_METHOD1(MockableNotificationBridgeReadyCallback, void(bool));
 
   scoped_refptr<dbus::MockBus> mock_bus_;
@@ -349,8 +384,20 @@ class NotificationPlatformBridgeLinuxTest : public BrowserWithTestWindowTest {
   base::Callback<void(dbus::Signal*)> notification_closed_callback_;
 
   std::unique_ptr<NotificationPlatformBridgeLinux> notification_bridge_linux_;
+  std::unique_ptr<NotificationDisplayServiceTester> display_service_tester_;
+
+  base::Optional<NotificationCommon::Operation> last_operation_;
+  base::Optional<int> last_action_index_;
 
  private:
+  void DoInvokeAction(uint32_t dbus_id, const std::string& action) {
+    dbus::Signal signal(kFreedesktopNotificationsName, "ActionInvoked");
+    dbus::MessageWriter writer(&signal);
+    writer.AppendUint32(dbus_id);
+    writer.AppendString(action);
+    action_invoked_callback_.Run(&signal);
+  }
+
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeLinuxTest);
 };
 
@@ -667,3 +714,91 @@ TEST_F(NotificationPlatformBridgeLinuxTest,
       NotificationHandler::Type::WEB_PERSISTENT, profile(),
       NotificationBuilder("").GetResult(), nullptr);
 }
+
+TEST_F(NotificationPlatformBridgeLinuxTest, DefaultButtonForwards) {
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("Notify"), _))
+      .WillOnce(OnNotify([](const NotificationRequest&) {}, 1));
+
+  CreateNotificationBridgeLinux(TestParams());
+  notification_bridge_linux_->Display(
+      NotificationHandler::Type::WEB_PERSISTENT, profile(),
+      NotificationBuilder("1")
+          .SetOriginUrl(GURL("https://google.com"))
+          .GetResult(),
+      nullptr);
+
+  InvokeAction(1, "default");
+
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_EQ(NotificationCommon::OPERATION_CLICK, last_operation_);
+  EXPECT_EQ(false, last_action_index_.has_value());
+}
+
+TEST_F(NotificationPlatformBridgeLinuxTest, SettingsButtonForwards) {
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("Notify"), _))
+      .WillOnce(OnNotify([](const NotificationRequest&) {}, 1));
+
+  CreateNotificationBridgeLinux(TestParams());
+  notification_bridge_linux_->Display(
+      NotificationHandler::Type::WEB_PERSISTENT, profile(),
+      NotificationBuilder("1")
+          .SetOriginUrl(GURL("https://google.com"))
+          .GetResult(),
+      nullptr);
+
+  InvokeAction(1, "settings");
+
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_EQ(NotificationCommon::OPERATION_SETTINGS, last_operation_);
+}
+
+TEST_F(NotificationPlatformBridgeLinuxTest, ActionButtonForwards) {
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("Notify"), _))
+      .WillOnce(OnNotify([](const NotificationRequest&) {}, 1));
+
+  CreateNotificationBridgeLinux(TestParams());
+  notification_bridge_linux_->Display(
+      NotificationHandler::Type::WEB_PERSISTENT, profile(),
+      NotificationBuilder("1")
+          .SetOriginUrl(GURL("https://google.com"))
+          .AddButton(ButtonInfo(base::ASCIIToUTF16("button0")))
+          .AddButton(ButtonInfo(base::ASCIIToUTF16("button1")))
+          .GetResult(),
+      nullptr);
+
+  InvokeAction(1, "1");
+
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_EQ(NotificationCommon::OPERATION_CLICK, last_operation_);
+  EXPECT_EQ(1, last_action_index_);
+}
+
+TEST_F(NotificationPlatformBridgeLinuxTest, CloseButtonForwards) {
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("Notify"), _))
+      .WillOnce(OnNotify([](const NotificationRequest&) {}, 1));
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("CloseNotification"), _))
+      .WillOnce(OnCloseNotification());
+
+  // custom close button is only added on cinnamon
+  CreateNotificationBridgeLinux(TestParams().SetServerName("cinnamon"));
+  notification_bridge_linux_->Display(
+      NotificationHandler::Type::WEB_PERSISTENT, profile(),
+      NotificationBuilder("1")
+          .SetOriginUrl(GURL("https://google.com"))
+          .GetResult(),
+      nullptr);
+
+  InvokeAction(1, "close");
+
+  content::RunAllTasksUntilIdle();
+
+  EXPECT_EQ(NotificationCommon::OPERATION_CLOSE, last_operation_);
+}