Don't duplicate close button on newer Cinnamon notifications

BUG=804637
R=thestig

Change-Id: Ia478f17bdc0ab07243a94fca6092b63d3cbaabfd
Reviewed-on: https://chromium-review.googlesource.com/c/1437479
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626178}

chrome/browser/notifications/notification_platform_bridge_linux.cc

@@ -26,6 +26,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
+#include "base/version.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/dbus/dbus_thread_linux.h"
@@ -171,13 +172,17 @@ gfx::Image ResizeImageToFdoMaxSize(const gfx::Image& image) {
           height)));
 }
 
-bool ShouldAddCloseButton(const std::string& server_name) {
+bool ShouldAddCloseButton(const std::string& server_name,
+                          const base::Version& server_version) {
   // Cinnamon doesn't add a close button on notifications.  With eg. calendar
   // notifications, which are stay-on-screen, this can lead to a situation where
   // the only way to dismiss a notification is to click on it, which would
   // create an unwanted web navigation.  For this reason, manually add a close
-  // button. (https://crbug.com/804637)
-  return server_name == "cinnamon";
+  // button (https://crbug.com/804637).  Cinnamon 3.8.0 adds a close button
+  // (https://github.com/linuxmint/Cinnamon/blob/8717fa/debian/changelog#L1075),
+  // so exclude versions that provide one already.
+  return server_name == "cinnamon" && server_version.IsValid() &&
+         server_version.CompareToWildcardString("3.8.0") < 0;
 }
 
 void ForwardNotificationOperationOnUiThread(
@@ -460,6 +465,10 @@ class NotificationPlatformBridgeLinuxImpl
     if (server_information_response) {
       dbus::MessageReader reader(server_information_response.get());
       reader.PopString(&server_name_);
+      std::string server_version;
+      reader.PopString(&server_version);  // Vendor
+      reader.PopString(&server_version);  // Server version
+      server_version_ = base::Version(server_version);
     }
 
     connected_signals_barrier_ = base::BarrierClosure(
@@ -627,7 +636,7 @@ class NotificationPlatformBridgeLinuxImpl
         actions.push_back(
             l10n_util::GetStringUTF8(IDS_NOTIFICATION_BUTTON_SETTINGS));
       }
-      if (ShouldAddCloseButton(server_name_)) {
+      if (ShouldAddCloseButton(server_name_, server_version_)) {
         actions.push_back(kCloseButtonId);
         actions.push_back(
             l10n_util::GetStringUTF8(IDS_NOTIFICATION_BUTTON_CLOSE));
@@ -969,6 +978,7 @@ class NotificationPlatformBridgeLinuxImpl
   std::unordered_set<std::string> capabilities_;
 
   std::string server_name_;
+  base::Version server_version_;
 
   base::Closure connected_signals_barrier_;
 

chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc

@@ -131,6 +131,7 @@ struct TestParams {
       : capabilities{"actions", "body", "body-hyperlinks", "body-images",
                      "body-markup"},
         server_name("NPBL_unittest"),
+        server_version("1.0"),
         expect_init_success(true),
         expect_shutdown(true),
         connect_signals(true) {}
@@ -145,6 +146,11 @@ struct TestParams {
     return *this;
   }
 
+  TestParams& SetServerVersion(const std::string& server_version) {
+    this->server_version = server_version;
+    return *this;
+  }
+
   TestParams& SetExpectInitSuccess(bool expect_init_success) {
     this->expect_init_success = expect_init_success;
     return *this;
@@ -162,6 +168,7 @@ struct TestParams {
 
   std::vector<std::string> capabilities;
   std::string server_name;
+  std::string server_version;
   bool expect_init_success;
   bool expect_shutdown;
   bool connect_signals;
@@ -237,13 +244,13 @@ std::unique_ptr<dbus::Response> GetIdResponse(uint32_t id) {
   return response;
 }
 
-ACTION_P(OnGetServerInformation, server_name) {
+ACTION_P2(OnGetServerInformation, server_name, server_version) {
   std::unique_ptr<dbus::Response> response = dbus::Response::CreateEmpty();
   dbus::MessageWriter writer(response.get());
-  writer.AppendString(server_name);  // name
-  writer.AppendString("chromium");   // vendor
-  writer.AppendString("1.0");        // version
-  writer.AppendString("1.2");        // spec_version
+  writer.AppendString(server_name);     // name
+  writer.AppendString("chromium");      // vendor
+  writer.AppendString(server_version);  // version
+  writer.AppendString("1.2");           // spec_version
   return response;
 }
 
@@ -338,7 +345,8 @@ class NotificationPlatformBridgeLinuxTest : public BrowserWithTestWindowTest {
     if (test_params.expect_init_success) {
       EXPECT_CALL(*mock_notification_proxy_.get(),
                   CallMethodAndBlock(Calls("GetServerInformation"), _))
-          .WillOnce(OnGetServerInformation(test_params.server_name));
+          .WillOnce(OnGetServerInformation(test_params.server_name,
+                                           test_params.server_version));
     }
 
     if (test_params.connect_signals) {
@@ -694,12 +702,12 @@ TEST_F(NotificationPlatformBridgeLinuxTest, OriginUrlFormat) {
 }
 
 TEST_F(NotificationPlatformBridgeLinuxTest,
-       CinnamonNotificationsHaveClosebutton) {
+       OldCinnamonNotificationsHaveClosebutton) {
   EXPECT_CALL(*mock_notification_proxy_.get(),
               CallMethodAndBlock(Calls("Notify"), _))
       .WillOnce(OnNotify(
           [](const NotificationRequest& request) {
-            EXPECT_EQ(3UL, request.actions.size());
+            ASSERT_EQ(3UL, request.actions.size());
             EXPECT_EQ("default", request.actions[0].id);
             EXPECT_EQ("Activate", request.actions[0].label);
             EXPECT_EQ("settings", request.actions[1].id);
@@ -709,7 +717,29 @@ TEST_F(NotificationPlatformBridgeLinuxTest,
           },
           1));
 
-  CreateNotificationBridgeLinux(TestParams().SetServerName("cinnamon"));
+  CreateNotificationBridgeLinux(
+      TestParams().SetServerName("cinnamon").SetServerVersion("3.6.7"));
+  notification_bridge_linux_->Display(
+      NotificationHandler::Type::WEB_PERSISTENT, profile(),
+      NotificationBuilder("").GetResult(), nullptr);
+}
+
+TEST_F(NotificationPlatformBridgeLinuxTest,
+       NewCinnamonNotificationsDontHaveClosebutton) {
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("Notify"), _))
+      .WillOnce(OnNotify(
+          [](const NotificationRequest& request) {
+            ASSERT_EQ(2UL, request.actions.size());
+            EXPECT_EQ("default", request.actions[0].id);
+            EXPECT_EQ("Activate", request.actions[0].label);
+            EXPECT_EQ("settings", request.actions[1].id);
+            EXPECT_EQ("Settings", request.actions[1].label);
+          },
+          1));
+
+  CreateNotificationBridgeLinux(
+      TestParams().SetServerName("cinnamon").SetServerVersion("3.8.0"));
   notification_bridge_linux_->Display(
       NotificationHandler::Type::WEB_PERSISTENT, profile(),
       NotificationBuilder("").GetResult(), nullptr);