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}diff --git a/chrome/browser/usb/web_usb_detector.cc b/chrome/browser/usb/web_usb_detector.cc
index 4d92dfc..50d1ebf 100644
--- a/chrome/browser/usb/web_usb_detector.cc
+++ b/chrome/browser/usb/web_usb_detector.cc
@@ -96,9 +96,11 @@
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 @@
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 @@
} // namespace
-WebUsbDetector::WebUsbDetector() : client_binding_(this) {}
+WebUsbDetector::WebUsbDetector() : client_binding_(this), weak_factory_(this) {}
WebUsbDetector::~WebUsbDetector() {}
@@ -223,6 +228,9 @@
return;
}
+ if (IsDisplayingNotification(landing_page))
+ return;
+
std::string notification_id = device_info->guid;
message_center::RichNotificationData rich_notification_data;
@@ -240,10 +248,25 @@
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(
diff --git a/chrome/browser/usb/web_usb_detector.h b/chrome/browser/usb/web_usb_detector.h
index af6ea2a7..96ad098 100644
--- a/chrome/browser/usb/web_usb_detector.h
+++ b/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 @@
void SetDeviceManagerForTesting(
device::mojom::UsbDeviceManagerPtr fake_device_manager);
+ void RemoveNotification(const std::string& id);
private:
// device::mojom::UsbDeviceManagerClient implementation.
@@ -26,12 +29,17 @@
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);
};
diff --git a/chrome/browser/usb/web_usb_detector_unittest.cc b/chrome/browser/usb/web_usb_detector_unittest.cc
index c3d34aea..7c16af6 100644
--- a/chrome/browser/usb/web_usb_detector_unittest.cc
+++ b/chrome/browser/usb/web_usb_detector_unittest.cc
@@ -541,7 +541,6 @@
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 @@
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 @@
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
