Call ArcAppListPrefs::Observer::{OnPackage{Added,Removed}} from OnPackageListRefreshed.

ArcAppListPref's browser-side cache of ARC apps may become out of date
with the ARC container, as the async IPC communication pipe between
container and browser may be flaky, or be established after the
container has processed some package installations/removals. This means
observer classes which follow the OnPackageAdded or OnPackageRemoved
calls may miss installations/uninstallations.

This CL augments ArcAppListPrefs::OnPackageListRefreshed to additionally
call observers if it detects packages which have been installed or
uninstalled which it has not yet seen.

BUG=893927

Change-Id: Icd069fdc89e348918f1185102e566bfc772087e3
Reviewed-on: https://chromium-review.googlesource.com/c/1345705Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612055}

chrome/browser/extensions/chrome_app_icon_unittest.cc

@@ -377,13 +377,16 @@ TEST_F(ChromeAppIconTest, ChromeBadging) {
   arc_test.app_instance()->SendRefreshAppList(fake_apps);
   arc_test.app_instance()->SendRefreshPackageList(
       ArcAppTest::ClonePackages(arc_test.fake_packages()));
-  EXPECT_EQ(1U, reference_icon.icon_update_count());
+
+  // Expect the package list refresh to generate two icon updates - one called
+  // by ArcAppListPrefs, and one called by LaunchExtensionAppUpdate.
+  EXPECT_EQ(2U, reference_icon.icon_update_count());
   EXPECT_FALSE(AreEqual(reference_icon.image_skia(), image_before_badging));
 
   // Opts out the Play Store. Badge should be gone and icon image is the same
   // as it was before badging.
   arc::SetArcPlayStoreEnabledForProfile(profile(), false);
-  EXPECT_EQ(2U, reference_icon.icon_update_count());
+  EXPECT_EQ(3U, reference_icon.icon_update_count());
   EXPECT_TRUE(AreEqual(reference_icon.image_skia(), image_before_badging));
 }
 

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

@@ -9,9 +9,11 @@
 #include <string>
 #include <utility>
 
+#include "base/containers/flat_set.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task/post_task.h"
 #include "base/values.h"
@@ -1600,17 +1602,24 @@ void ArcAppListPrefs::OnPackageListRefreshed(
     std::vector<arc::mojom::ArcPackageInfoPtr> packages) {
   DCHECK(IsArcAndroidEnabledForProfile(profile_));
 
-  const std::vector<std::string> old_packages(GetPackagesFromPrefs());
-  std::unordered_set<std::string> current_packages;
+  const base::flat_set<std::string> old_packages(GetPackagesFromPrefs());
+  std::set<std::string> current_packages;
 
   for (const auto& package : packages) {
     AddOrUpdatePackagePrefs(*package);
-    current_packages.insert((*package).package_name);
+    if (!base::ContainsKey(old_packages, package->package_name)) {
+      for (auto& observer : observer_list_)
+        observer.OnPackageInstalled(*package);
+    }
+    current_packages.insert(package->package_name);
   }
 
   for (const auto& package_name : old_packages) {
-    if (!current_packages.count(package_name))
+    if (!base::ContainsKey(current_packages, package_name)) {
       RemovePackageFromPrefs(package_name);
+      for (auto& observer : observer_list_)
+        observer.OnPackageRemoved(package_name, false);
+    }
   }
 
   package_list_initial_refreshed_ = true;

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

@@ -1269,6 +1269,48 @@ TEST_P(ArcAppModelBuilderTest, AppLifeCycleEventsOnOptOut) {
   prefs->RemoveObserver(&observer);
 }
 
+TEST_P(ArcAppModelBuilderTest, AppLifeCycleEventsOnPackageListRefresh) {
+  ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
+  ASSERT_TRUE(prefs);
+
+  arc::MockArcAppListPrefsObserver observer;
+  prefs->AddObserver(&observer);
+
+  // Refreshing the package list with hithero unseen packages should call
+  // OnPackageInstalled.
+  EXPECT_CALL(observer, OnPackageInstalled(testing::Field(
+                            &arc::mojom::ArcPackageInfo::package_name,
+                            fake_packages()[0]->package_name)))
+      .Times(1);
+  EXPECT_CALL(observer, OnPackageInstalled(testing::Field(
+                            &arc::mojom::ArcPackageInfo::package_name,
+                            fake_packages()[1]->package_name)))
+      .Times(1);
+  EXPECT_CALL(observer, OnPackageInstalled(testing::Field(
+                            &arc::mojom::ArcPackageInfo::package_name,
+                            fake_packages()[2]->package_name)))
+      .Times(1);
+  app_instance()->SendRefreshPackageList(
+      ArcAppTest::ClonePackages(fake_packages()));
+
+  // Refreshing the package list and omitting previously seen packages should
+  // call OnPackageRemoved for the omitted packages only.
+  EXPECT_CALL(observer,
+              OnPackageRemoved(fake_packages()[0]->package_name, false))
+      .Times(0);
+  EXPECT_CALL(observer,
+              OnPackageRemoved(fake_packages()[1]->package_name, false))
+      .Times(1);
+  EXPECT_CALL(observer,
+              OnPackageRemoved(fake_packages()[2]->package_name, false))
+      .Times(1);
+
+  std::vector<arc::mojom::ArcPackageInfoPtr> packages;
+  packages.push_back(fake_packages()[0].Clone());
+  app_instance()->SendRefreshPackageList(std::move(packages));
+  prefs->RemoveObserver(&observer);
+}
+
 // Validate that arc model contains expected elements on restart.
 TEST_P(ArcAppModelBuilderRecreate, AppModelRestart) {
   // No apps on initial start.