Use AshMessageCenterClient to get arc app id

Restores the arc app notification badging on shelf icons by re-wiring
ArcNotificationManager get app id code path through mojo.

- Change ArcNotificationManager to handle get app id asynchronously;
- Add GetArcAppIdByPackageName to AshMessageCenterClient mojo interface;
- ArcNotificationManager use the mojo call to get app id;

Bug: 768439
Change-Id: I199636b374d539677d93e35be1fd4f7b9b900bca
Reviewed-on: https://chromium-review.googlesource.com/1012123Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553603}

ash/message_center/message_center_controller.cc

@@ -9,6 +9,7 @@
 #include "ash/session/session_controller.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/unguessable_token.h"
 #include "components/signin/core/account_id/account_id.h"
@@ -160,6 +161,9 @@ void MessageCenterController::SetArcNotificationsInstance(
             ->GetPrimaryUserSession()
             ->user_info->account_id,
         message_center::MessageCenter::Get());
+    arc_notification_manager_->set_get_app_id_callback(
+        base::BindRepeating(&MessageCenterController::GetArcAppIdByPackageName,
+                            base::Unretained(this)));
   }
   arc_notification_manager_->SetInstance(std::move(arc_notification_instance));
 }
@@ -226,4 +230,11 @@ void MessageCenterController::OnGotNotifierList(
     notifier_id_->SetNotifierList(ui_data);
 }
 
+void MessageCenterController::GetArcAppIdByPackageName(
+    const std::string& package_name,
+    arc::ArcNotificationManager::GetAppIdResponseCallback callback) {
+  DCHECK(client_.is_bound());
+  client_->GetArcAppIdByPackageName(package_name, std::move(callback));
+}
+
 }  // namespace ash

ash/message_center/message_center_controller.h

@@ -78,6 +78,11 @@ class ASH_EXPORT MessageCenterController
   // Callback for GetNotifierList.
   void OnGotNotifierList(std::vector<mojom::NotifierUiDataPtr> ui_data);
 
+  // Handles get app id calls from ArcNotificationManager.
+  void GetArcAppIdByPackageName(
+      const std::string& package_name,
+      arc::ArcNotificationManager::GetAppIdResponseCallback callback);
+
   std::unique_ptr<FullscreenNotificationBlocker>
       fullscreen_notification_blocker_;
   std::unique_ptr<InactiveUserNotificationBlocker>

ash/message_center/notifier_settings_view_unittest.cc

@@ -67,6 +67,11 @@ class TestAshMessageCenterClient : public mojom::AshMessageCenterClient {
 
     std::move(callback).Run(std::move(ui_data));
   }
+  void GetArcAppIdByPackageName(
+      const std::string& package_name,
+      GetArcAppIdByPackageNameCallback callback) override {
+    std::move(callback).Run(std::string());
+  }
 
  private:
   bool no_notifiers_ = false;

ash/public/interfaces/ash_message_center_controller.mojom

@@ -89,4 +89,7 @@ interface AshMessageCenterClient {
   // Asks the client for a list of notifiers and associated UI information to be
   // displayed in a settings panel.
   GetNotifierList() => (array<NotifierUiData> notifiers);
+
+  // Gets ARC app id from a given package name.
+  GetArcAppIdByPackageName(string package_name) => (string app_id);
 };

chrome/browser/notifications/chrome_ash_message_center_client.cc

@@ -7,10 +7,12 @@
 #include "ash/public/interfaces/constants.mojom.h"
 #include "base/i18n/string_compare.h"
 #include "base/stl_util.h"
+#include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/notifications/arc_application_notifier_controller.h"
 #include "chrome/browser/notifications/extension_notifier_controller.h"
 #include "chrome/browser/notifications/web_page_notifier_controller.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/common/service_manager_connection.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -164,6 +166,14 @@ void ChromeAshMessageCenterClient::GetNotifierList(
   std::move(callback).Run(std::move(notifiers));
 }
 
+void ChromeAshMessageCenterClient::GetArcAppIdByPackageName(
+    const std::string& package_name,
+    GetArcAppIdByPackageNameCallback callback) {
+  std::move(callback).Run(
+      ArcAppListPrefs::Get(arc::ArcSessionManager::Get()->profile())
+          ->GetAppIdByPackageName(package_name));
+}
+
 void ChromeAshMessageCenterClient::OnIconImageUpdated(
     const NotifierId& notifier_id,
     const gfx::ImageSkia& image) {

chrome/browser/notifications/chrome_ash_message_center_client.h

@@ -42,6 +42,9 @@ class ChromeAshMessageCenterClient : public ash::mojom::AshMessageCenterClient,
   void SetNotifierEnabled(const message_center::NotifierId& notifier_id,
                           bool enabled) override;
   void GetNotifierList(GetNotifierListCallback callback) override;
+  void GetArcAppIdByPackageName(
+      const std::string& package_name,
+      GetArcAppIdByPackageNameCallback callback) override;
 
   // NotifierController::Observer:
   void OnIconImageUpdated(const message_center::NotifierId& notifier_id,

ui/arc/notification/arc_notification_manager.cc

@@ -100,6 +100,9 @@ ArcNotificationManager::ArcNotificationManager(
 ArcNotificationManager::~ArcNotificationManager() {
   instance_owner_->holder()->RemoveObserver(this);
   instance_owner_->holder()->SetHost(nullptr);
+
+  // Ensures that any callback tied to |instance_owner_| is not invoked.
+  instance_owner_.reset();
 }
 
 void ArcNotificationManager::SetInstance(
@@ -149,9 +152,10 @@ void ArcNotificationManager::OnNotificationPosted(
     DCHECK(result.second);
     it = result.first;
   }
-  const std::string app_id =
-      data->package_name ? GetAppId(data->package_name.value()) : std::string();
-  it->second->OnUpdatedFromAndroid(std::move(data), app_id);
+
+  GetAppId(data->package_name.value_or(std::string()),
+           base::BindOnce(&ArcNotificationManager::OnGotAppId,
+                          weak_ptr_factory_.GetWeakPtr(), std::move(data)));
 }
 
 void ArcNotificationManager::OnNotificationUpdated(
@@ -166,8 +170,9 @@ void ArcNotificationManager::OnNotificationUpdated(
   if (it == items_.end())
     return;
 
-  const std::string app_id = GetAppId(data->package_name.value());
-  it->second->OnUpdatedFromAndroid(std::move(data), app_id);
+  GetAppId(data->package_name.value_or(std::string()),
+           base::BindOnce(&ArcNotificationManager::OnGotAppId,
+                          weak_ptr_factory_.GetWeakPtr(), std::move(data)));
 }
 
 void ArcNotificationManager::OnNotificationRemoved(const std::string& key) {
@@ -369,12 +374,24 @@ bool ArcNotificationManager::ShouldIgnoreNotification(
   return false;
 }
 
-std::string ArcNotificationManager::GetAppId(
-    const std::string& package_name) const {
-  if (get_app_id_callback_.is_null())
-    return std::string();
+void ArcNotificationManager::GetAppId(const std::string& package_name,
+                                      GetAppIdResponseCallback callback) const {
+  if (get_app_id_callback_.is_null() || package_name.empty()) {
+    std::move(callback).Run(std::string());
+    return;
+  }
+
+  get_app_id_callback_.Run(package_name, std::move(callback));
+}
+
+void ArcNotificationManager::OnGotAppId(mojom::ArcNotificationDataPtr data,
+                                        const std::string& app_id) {
+  const std::string& key = data->key;
+  auto it = items_.find(key);
+  if (it == items_.end())
+    return;
 
-  return get_app_id_callback_.Run(package_name);
+  it->second->OnUpdatedFromAndroid(std::move(data), app_id);
 }
 
 }  // namespace arc

ui/arc/notification/arc_notification_manager.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <unordered_map>
 
+#include "base/memory/weak_ptr.h"
 #include "components/arc/common/notifications.mojom.h"
 #include "components/arc/connection_holder.h"
 #include "components/arc/connection_observer.h"
@@ -37,10 +38,13 @@ class ArcNotificationManager
   ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>*
   GetConnectionHolderForTest();
 
-  void set_get_app_id_callback(
-      base::RepeatingCallback<std::string(const std::string&)>
-          get_app_id_callback) {
-    get_app_id_callback_ = std::move(get_app_id_callback);
+  using GetAppIdResponseCallback =
+      base::OnceCallback<void(const std::string& app_id)>;
+  using GetAppIdCallback =
+      base::RepeatingCallback<void(const std::string& package_name,
+                                   GetAppIdResponseCallback callback)>;
+  void set_get_app_id_callback(const GetAppIdCallback& get_app_id_callback) {
+    get_app_id_callback_ = get_app_id_callback;
   }
 
   // ConnectionObserver<mojom::NotificationsInstance> implementation:
@@ -69,8 +73,14 @@ class ArcNotificationManager
 
   bool ShouldIgnoreNotification(mojom::ArcNotificationData* data);
 
-  // Calls |get_app_id_callback_| to retrieve the app id from ArcAppListPrefs.
-  std::string GetAppId(const std::string& package_name) const;
+  // Calls |get_app_id_callback_| to retrieve the app id. |callback| will be
+  // invoked with the app id or an empty string.
+  void GetAppId(const std::string& package_name,
+                GetAppIdResponseCallback callback) const;
+
+  // Invoked when |get_app_id_callback_| gets back the app id.
+  void OnGotAppId(mojom::ArcNotificationDataPtr data,
+                  const std::string& app_id);
 
   const AccountId main_profile_id_;
   message_center::MessageCenter* const message_center_;
@@ -79,14 +89,14 @@ class ArcNotificationManager
       std::unordered_map<std::string, std::unique_ptr<ArcNotificationItem>>;
   ItemMap items_;
 
-  base::RepeatingCallback<std::string(const std::string&)> get_app_id_callback_;
+  GetAppIdCallback get_app_id_callback_;
 
   bool ready_ = false;
 
-  // Put as a last member to ensure that any callback tied to the elements
-  // is not invoked.
   std::unique_ptr<InstanceOwner> instance_owner_;
 
+  base::WeakPtrFactory<ArcNotificationManager> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ArcNotificationManager);
 };