Add notification browsertests.

Modify AppNotifications to handle the multiple apps for one notification
case, because for one web app notification, there could be multiple web
apps affected.

BUG=1068884

Change-Id: I2257bee69b92411ee0b690bc2f8ea966ce488f8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2181246
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771759}

chrome/browser/apps/app_service/DEPS

@@ -9,6 +9,11 @@ specific_include_rules = {
     # expose it to any other files except through the public mojo interface.
     "+chrome/services/app_service/app_service_impl.h",
   ],
+  "notifications_browsertest\.cc": [
+    "+ash/system/message_center/arc/arc_notification_manager.h",
+    "+ash/system/message_center/arc/arc_notification_manager_delegate.h",
+    "+ui/message_center/message_center.h",
+  ],
   ".*unittest\.cc": [
     "+cc/test",
   ],

chrome/browser/apps/app_service/app_notifications.cc

@@ -14,32 +14,36 @@ AppNotifications::~AppNotifications() = default;
 
 void AppNotifications::AddNotification(const std::string& app_id,
                                        const std::string& notification_id) {
-  app_id_to_notification_id_[app_id].insert(notification_id);
-  notification_id_to_app_id_[notification_id] = app_id;
+  app_id_to_notification_ids_[app_id].insert(notification_id);
+  notification_id_to_app_ids_[notification_id].insert(app_id);
 }
 
 void AppNotifications::RemoveNotification(const std::string& notification_id) {
-  DCHECK(base::Contains(notification_id_to_app_id_, notification_id));
-
-  const std::string app_id = notification_id_to_app_id_[notification_id];
-  app_id_to_notification_id_[app_id].erase(notification_id);
-  if (app_id_to_notification_id_[app_id].empty()) {
-    app_id_to_notification_id_.erase(app_id);
+  auto it = notification_id_to_app_ids_.find(notification_id);
+  DCHECK(it != notification_id_to_app_ids_.end());
+
+  const auto& app_ids = it->second;
+  for (const auto& app_id : app_ids) {
+    auto app_id_it = app_id_to_notification_ids_.find(app_id);
+    app_id_it->second.erase(notification_id);
+    if (app_id_it->second.empty()) {
+      app_id_to_notification_ids_.erase(app_id_it);
+    }
   }
-
-  notification_id_to_app_id_.erase(notification_id);
+  notification_id_to_app_ids_.erase(it);
 }
 
 bool AppNotifications::HasNotification(const std::string& app_id) {
-  return base::Contains(app_id_to_notification_id_, app_id);
+  return base::Contains(app_id_to_notification_ids_, app_id);
 }
 
-const std::string AppNotifications::GetAppIdForNotification(
+std::set<std::string> AppNotifications::GetAppIdsForNotification(
     const std::string& notification_id) {
-  if (!base::Contains(notification_id_to_app_id_, notification_id)) {
-    return std::string();
+  auto it = notification_id_to_app_ids_.find(notification_id);
+  if (it == notification_id_to_app_ids_.end()) {
+    return {};
   }
-  return notification_id_to_app_id_[notification_id];
+  return it->second;
 }
 
 apps::mojom::AppPtr AppNotifications::GetAppWithHasBadgeStatus(

chrome/browser/apps/app_service/app_notifications.h

@@ -33,23 +33,25 @@ class AppNotifications {
   // Returns true, if the app has notifications. Otherwise, returns false.
   bool HasNotification(const std::string& app_id);
 
-  // Returns the app id for the given |notification_id|, if |notification_id|
-  // exists in |notification_id_to_app_id_|. Otherwise, return an empty string.
-  const std::string GetAppIdForNotification(const std::string& notification_id);
+  // Returns the set of app ids for the given |notification_id|, if
+  // |notification_id| exists in |notification_id_to_app_id_|. Otherwise, return
+  // an empty set.
+  std::set<std::string> GetAppIdsForNotification(
+      const std::string& notification_id);
 
   apps::mojom::AppPtr GetAppWithHasBadgeStatus(apps::mojom::AppType app_type,
                                                const std::string& app_id);
 
  private:
   // Maps one app id to a set of all matching notification ids.
-  std::map<std::string, std::set<std::string>> app_id_to_notification_id_;
+  std::map<std::string, std::set<std::string>> app_id_to_notification_ids_;
 
-  // Maps one notification id to one app id. When the notification has been
-  // delivered, the MessageCenter has already deleted the notification, so we
-  // can't fetch the corresponding app id when the notification is removed. So
-  // we need a record of this notification, and erase it from both
+  // Maps one notification id to a set of app ids. When the notification has
+  // been delivered, the MessageCenter has already deleted the notification, so
+  // we can't fetch the corresponding app id when the notification is removed.
+  // So we need a record of this notification, and erase it from both
   // |app_id_to_notification_id_| and |notification_id_to_app_id_|.
-  std::map<std::string, std::string> notification_id_to_app_id_;
+  std::map<std::string, std::set<std::string>> notification_id_to_app_ids_;
 };
 
 }  // namespace apps

chrome/browser/apps/app_service/arc_apps.cc

@@ -970,16 +970,19 @@ void ArcApps::OnNotificationUpdated(const std::string& notification_id,
 }
 
 void ArcApps::OnNotificationRemoved(const std::string& notification_id) {
-  const std::string& app_id =
-      app_notifications_.GetAppIdForNotification(notification_id);
-  if (app_id.empty()) {
+  const auto app_ids =
+      app_notifications_.GetAppIdsForNotification(notification_id);
+  if (app_ids.empty()) {
     return;
   }
 
   app_notifications_.RemoveNotification(notification_id);
-  Publish(app_notifications_.GetAppWithHasBadgeStatus(
-              apps::mojom::AppType::kArc, app_id),
-          subscribers_);
+
+  for (const auto& app_id : app_ids) {
+    Publish(app_notifications_.GetAppWithHasBadgeStatus(
+                apps::mojom::AppType::kArc, app_id),
+            subscribers_);
+  }
 }
 
 void ArcApps::OnArcNotificationManagerDestroyed(
@@ -1064,6 +1067,9 @@ apps::mojom::AppPtr ArcApps::Convert(ArcAppListPrefs* prefs,
   app->show_in_search = show;
   app->show_in_management = show;
 
+  app->has_badge = app_notifications_.HasNotification(app_id)
+                       ? apps::mojom::OptionalBool::kTrue
+                       : apps::mojom::OptionalBool::kFalse;
   app->paused = paused;
 
   std::unique_ptr<ArcAppListPrefs::PackageInfo> package =

chrome/browser/apps/app_service/extension_apps_chromeos.cc

@@ -514,14 +514,18 @@ void ExtensionAppsChromeOs::OnNotificationDisplayed(
 
 void ExtensionAppsChromeOs::OnNotificationClosed(
     const std::string& notification_id) {
-  const std::string& app_id =
-      app_notifications_.GetAppIdForNotification(notification_id);
-  if (app_id.empty() || MaybeGetExtension(app_id) == nullptr) {
+  const auto app_ids =
+      app_notifications_.GetAppIdsForNotification(notification_id);
+  if (app_ids.empty()) {
     return;
   }
+
   app_notifications_.RemoveNotification(notification_id);
-  Publish(app_notifications_.GetAppWithHasBadgeStatus(app_type(), app_id),
-          subscribers());
+
+  for (const auto& app_id : app_ids) {
+    Publish(app_notifications_.GetAppWithHasBadgeStatus(app_type(), app_id),
+            subscribers());
+  }
 }
 
 void ExtensionAppsChromeOs::OnNotificationDisplayServiceDestroyed(

chrome/browser/apps/app_service/web_apps_chromeos.cc

@@ -294,21 +294,18 @@ void WebAppsChromeOs::OnNotificationDisplayed(
 }
 
 void WebAppsChromeOs::OnNotificationClosed(const std::string& notification_id) {
-  const std::string& app_id =
-      app_notifications_.GetAppIdForNotification(notification_id);
-  if (app_id.empty()) {
-    return;
-  }
-
-  const web_app::WebApp* web_app = GetWebApp(app_id);
-  if (!web_app || !Accepts(app_id)) {
+  auto app_ids = app_notifications_.GetAppIdsForNotification(notification_id);
+  if (app_ids.empty()) {
     return;
   }
 
   app_notifications_.RemoveNotification(notification_id);
-  Publish(app_notifications_.GetAppWithHasBadgeStatus(
-              apps::mojom::AppType::kWeb, app_id),
-          subscribers());
+
+  for (const auto& app_id : app_ids) {
+    Publish(app_notifications_.GetAppWithHasBadgeStatus(
+                apps::mojom::AppType::kWeb, app_id),
+            subscribers());
+  }
 }
 
 void WebAppsChromeOs::OnNotificationDisplayServiceDestroyed(
@@ -373,6 +370,9 @@ apps::mojom::AppPtr WebAppsChromeOs::Convert(const web_app::WebApp* web_app,
   app->icon_key = icon_key_factory().MakeIconKey(
       GetIconEffects(web_app, paused, is_disabled));
 
+  app->has_badge = app_notifications_.HasNotification(web_app->app_id())
+                       ? apps::mojom::OptionalBool::kTrue
+                       : apps::mojom::OptionalBool::kFalse;
   app->paused = paused ? apps::mojom::OptionalBool::kTrue
                        : apps::mojom::OptionalBool::kFalse;
   return app;

chrome/test/BUILD.gn

@@ -1480,6 +1480,7 @@ if (!is_android) {
       ]
 
       sources += [
+        "../browser/apps/app_service/notifications_browsertest.cc",
         "../browser/policy/accessibility_policy_browsertest.cc",
         "../browser/policy/arc_policy_browsertest.cc",
         "../browser/policy/assistant_policy_browsertest.cc",