Stop duplicates WebUSB notification

This change adds a list of notifications that are currently displayed and checks
that list for duplicates before displaying a new notification.

Bug: 789362
Change-Id: I1f8025dbb237da35b088de5bd6f21e80a1bbec52
Reviewed-on: https://chromium-review.googlesource.com/c/1373436
Commit-Queue: James Hollyer <jameshollyer@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619541}

chrome/browser/usb/web_usb_detector.cc

@@ -96,9 +96,11 @@ void OpenURL(const GURL& url) {
 class WebUsbNotificationDelegate : public TabStripModelObserver,
                                    public message_center::NotificationDelegate {
  public:
-  WebUsbNotificationDelegate(const GURL& landing_page,
+  WebUsbNotificationDelegate(base::WeakPtr<WebUsbDetector> detector,
+                             const GURL& landing_page,
                              const std::string& notification_id)
-      : landing_page_(landing_page),
+      : detector_(std::move(detector)),
+        landing_page_(landing_page),
         notification_id_(notification_id),
         disposition_(WEBUSB_NOTIFICATION_CLOSED),
         browser_tab_strip_tracker_(this, nullptr, nullptr) {
@@ -157,11 +159,14 @@ class WebUsbNotificationDelegate : public TabStripModelObserver,
     RecordNotificationClosure(disposition_);
 
     browser_tab_strip_tracker_.StopObservingAndSendOnBrowserRemoved();
+    if (detector_)
+      detector_->RemoveNotification(notification_id_);
   }
 
  private:
   ~WebUsbNotificationDelegate() override = default;
 
+  base::WeakPtr<WebUsbDetector> detector_;
   GURL landing_page_;
   std::string notification_id_;
   WebUsbNotificationClosed disposition_;
@@ -172,7 +177,7 @@ class WebUsbNotificationDelegate : public TabStripModelObserver,
 
 }  // namespace
 
-WebUsbDetector::WebUsbDetector() : client_binding_(this) {}
+WebUsbDetector::WebUsbDetector() : client_binding_(this), weak_factory_(this) {}
 
 WebUsbDetector::~WebUsbDetector() {}
 
@@ -223,6 +228,9 @@ void WebUsbDetector::OnDeviceAdded(
     return;
   }
 
+  if (IsDisplayingNotification(landing_page))
+    return;
+
   std::string notification_id = device_info->guid;
 
   message_center::RichNotificationData rich_notification_data;
@@ -240,10 +248,25 @@ void WebUsbDetector::OnDeviceAdded(
       message_center::NotifierId(message_center::NotifierType::SYSTEM_COMPONENT,
                                  kNotifierWebUsb),
       rich_notification_data,
-      base::MakeRefCounted<WebUsbNotificationDelegate>(landing_page,
-                                                       notification_id));
+      base::MakeRefCounted<WebUsbNotificationDelegate>(
+          weak_factory_.GetWeakPtr(), landing_page, notification_id));
   notification.SetSystemPriority();
   SystemNotificationHelper::GetInstance()->Display(notification);
+  open_notifications_by_id_[notification_id] = landing_page;
+}
+
+bool WebUsbDetector::IsDisplayingNotification(const GURL& url) {
+  for (const auto& map_entry : open_notifications_by_id_) {
+    const GURL& entry_url = map_entry.second;
+    if (url == entry_url)
+      return true;
+  }
+
+  return false;
+}
+
+void WebUsbDetector::RemoveNotification(const std::string& id) {
+  open_notifications_by_id_.erase(id);
 }
 
 void WebUsbDetector::OnDeviceRemoved(

chrome/browser/usb/web_usb_detector.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_USB_WEB_USB_DETECTOR_H_
 #define CHROME_BROWSER_USB_WEB_USB_DETECTOR_H_
 
+#include <map>
+
 #include "base/macros.h"
 #include "device/usb/public/mojom/device_manager.mojom.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
@@ -19,6 +21,7 @@ class WebUsbDetector : public device::mojom::UsbDeviceManagerClient {
 
   void SetDeviceManagerForTesting(
       device::mojom::UsbDeviceManagerPtr fake_device_manager);
+  void RemoveNotification(const std::string& id);
 
  private:
   // device::mojom::UsbDeviceManagerClient implementation.
@@ -26,12 +29,17 @@ class WebUsbDetector : public device::mojom::UsbDeviceManagerClient {
   void OnDeviceRemoved(device::mojom::UsbDeviceInfoPtr device_info) override;
 
   void OnDeviceManagerConnectionError();
+  bool IsDisplayingNotification(const GURL& url);
+
+  std::map<std::string, GURL> open_notifications_by_id_;
 
   // Connection to |device_manager_instance_|.
   device::mojom::UsbDeviceManagerPtr device_manager_;
   mojo::AssociatedBinding<device::mojom::UsbDeviceManagerClient>
       client_binding_;
 
+  base::WeakPtrFactory<WebUsbDetector> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(WebUsbDetector);
 };
 

chrome/browser/usb/web_usb_detector_unittest.cc

@@ -541,7 +541,6 @@ TEST_F(WebUsbDetectorTest,
 
 TEST_F(WebUsbDetectorTest, NotificationClickedWhileNoTabUrlIsLandingPage) {
   GURL landing_page_1(kLandingPage_1);
-  GURL landing_page_2(kLandingPage_2);
   auto device_1 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
       0, 1, "Google", kProductName_1, "002", landing_page_1);
   std::string guid_1 = device_1->guid();
@@ -604,6 +603,103 @@ TEST_F(WebUsbDetectorTest, UsbDeviceAddedWhileActiveTabFuzzyUrlIsLandingPage) {
   EXPECT_FALSE(display_service_->GetNotification(guid_1));
 }
 
+TEST_F(WebUsbDetectorTest, TwoDevicesSameLandingPageAddedRemovedAndAddedAgain) {
+  GURL landing_page_1(kLandingPage_1);
+  auto device_1 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      0, 1, "Google", kProductName_1, "002", landing_page_1);
+  std::string guid_1 = device_1->guid();
+
+  auto device_2 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      3, 4, "Google", kProductName_2, "005", landing_page_1);
+  std::string guid_2 = device_2->guid();
+
+  Initialize();
+  base::RunLoop().RunUntilIdle();
+
+  device_manager_.AddDevice(device_1);
+  base::RunLoop().RunUntilIdle();
+  base::Optional<message_center::Notification> notification_1 =
+      display_service_->GetNotification(guid_1);
+  ASSERT_TRUE(notification_1);
+  base::string16 expected_title_1 =
+      base::ASCIIToUTF16("Google Product A detected");
+  EXPECT_EQ(expected_title_1, notification_1->title());
+  base::string16 expected_message_1 =
+      base::ASCIIToUTF16("Go to www.google.com to connect.");
+  EXPECT_EQ(expected_message_1, notification_1->message());
+  EXPECT_TRUE(notification_1->delegate() != nullptr);
+
+  device_manager_.AddDevice(device_2);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(display_service_->GetNotification(guid_2));
+
+  device_manager_.RemoveDevice(device_2);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(display_service_->GetNotification(guid_1));
+
+  device_manager_.RemoveDevice(device_1);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(display_service_->GetNotification(guid_1));
+
+  device_manager_.AddDevice(device_2);
+  base::RunLoop().RunUntilIdle();
+  base::Optional<message_center::Notification> notification_2 =
+      display_service_->GetNotification(guid_2);
+  ASSERT_TRUE(notification_2);
+  base::string16 expected_title_2 =
+      base::ASCIIToUTF16("Google Product B detected");
+  EXPECT_EQ(expected_title_2, notification_2->title());
+  base::string16 expected_message_2 =
+      base::ASCIIToUTF16("Go to www.google.com to connect.");
+  EXPECT_EQ(expected_message_2, notification_2->message());
+  EXPECT_TRUE(notification_2->delegate() != nullptr);
+
+  device_manager_.AddDevice(device_1);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(display_service_->GetNotification(guid_1));
+}
+
+TEST_F(
+    WebUsbDetectorTest,
+    DeviceWithSameLandingPageAddedAfterNotificationClickedAndThenNewTabActive) {
+  GURL landing_page_1(kLandingPage_1);
+  GURL landing_page_2(kLandingPage_2);
+  auto device_1 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      0, 1, "Google", kProductName_1, "002", landing_page_1);
+  std::string guid_1 = device_1->guid();
+
+  auto device_2 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      0, 1, "Google", kProductName_2, "002", landing_page_1);
+  std::string guid_2 = device_2->guid();
+
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+
+  base::HistogramTester histogram_tester;
+  Initialize();
+  base::RunLoop().RunUntilIdle();
+
+  device_manager_.AddDevice(device_1);
+  base::RunLoop().RunUntilIdle();
+  base::Optional<message_center::Notification> notification_1 =
+      display_service_->GetNotification(guid_1);
+  ASSERT_TRUE(notification_1);
+  EXPECT_EQ(0, tab_strip_model->count());
+
+  notification_1->delegate()->Click(base::nullopt, base::nullopt);
+  EXPECT_EQ(1, tab_strip_model->count());
+  content::WebContents* web_contents =
+      tab_strip_model->GetWebContentsAt(tab_strip_model->active_index());
+  EXPECT_EQ(landing_page_1, web_contents->GetURL());
+  EXPECT_FALSE(display_service_->GetNotification(guid_1));
+  histogram_tester.ExpectUniqueSample("WebUsb.NotificationClosed", 2, 1);
+
+  AddTab(browser(), landing_page_2);
+
+  device_manager_.AddDevice(device_2);
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(display_service_->GetNotification(guid_2));
+}
+
 TEST_F(WebUsbDetectorTest,
        NotificationClickedWhileInactiveTabFuzzyUrlIsLandingPage) {
   GURL landing_page_1(kLandingPage_1);
@@ -637,4 +733,34 @@ TEST_F(WebUsbDetectorTest,
   histogram_tester.ExpectUniqueSample("WebUsb.NotificationClosed", 2, 1);
 }
 
+TEST_F(WebUsbDetectorTest,
+       DeviceWithSameLandingPageAddedAfterPageVisitedAndNewTabActive) {
+  GURL landing_page_1(kLandingPage_1);
+  GURL landing_page_2(kLandingPage_2);
+  auto device_1 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      0, 1, "Google", kProductName_1, "002", landing_page_1);
+  std::string guid_1 = device_1->guid();
+
+  auto device_2 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      0, 1, "Google", kProductName_1, "002", landing_page_1);
+  std::string guid_2 = device_2->guid();
+
+  base::HistogramTester histogram_tester;
+  Initialize();
+  base::RunLoop().RunUntilIdle();
+
+  device_manager_.AddDevice(device_1);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(display_service_->GetNotification(guid_1));
+
+  AddTab(browser(), landing_page_1);
+  EXPECT_FALSE(display_service_->GetNotification(guid_1));
+  histogram_tester.ExpectUniqueSample("WebUsb.NotificationClosed", 3, 1);
+
+  AddTab(browser(), landing_page_2);
+  device_manager_.AddDevice(device_2);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(display_service_->GetNotification(guid_2));
+}
+
 #endif  // !OS_WIN