Add installation packages event into app install logger.

This adds information about package installation started, finished and
failed into app install logger.

Bug: b/73277923
Test: unit_tests, manually
Change-Id: I0903e72eef8090328d9d9e3a14d12473d1610fab
Reviewed-on: https://chromium-review.googlesource.com/996302
Commit-Queue: Yury Khmel <khmel@google.com>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Bartosz Fabianowski <bartfab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550083}

chrome/browser/chromeos/policy/app_install_event_log_collector.cc

@@ -59,7 +59,10 @@ AppInstallEventLogCollector::AppInstallEventLogCollector(
     Delegate* delegate,
     Profile* profile,
     const std::set<std::string>& pending_packages)
-    : delegate_(delegate), profile_(profile), online_(GetOnlineState()) {
+    : delegate_(delegate),
+      profile_(profile),
+      online_(GetOnlineState()),
+      pending_packages_(pending_packages) {
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(
       this);
   net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
@@ -69,9 +72,17 @@ AppInstallEventLogCollector::AppInstallEventLogCollector(
   if (policy_bridge) {
     policy_bridge->AddObserver(this);
   }
+  ArcAppListPrefs* const app_prefs = ArcAppListPrefs::Get(profile_);
+  if (app_prefs) {
+    app_prefs->AddObserver(this);
+  }
 }
 
 AppInstallEventLogCollector::~AppInstallEventLogCollector() {
+  ArcAppListPrefs* const app_prefs = ArcAppListPrefs::Get(profile_);
+  if (app_prefs) {
+    app_prefs->RemoveObserver(this);
+  }
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(
       this);
   net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
@@ -83,8 +94,9 @@ AppInstallEventLogCollector::~AppInstallEventLogCollector() {
 }
 
 void AppInstallEventLogCollector::OnPendingPackagesChanged(
-    const std::set<std::string>& added,
-    const std::set<std::string>& removed) {}
+    const std::set<std::string>& pending_packages) {
+  pending_packages_ = pending_packages;
+}
 
 void AppInstallEventLogCollector::AddLoginEvent() {
   // Don't log in case session is restared or recovered from crash.
@@ -173,4 +185,31 @@ void AppInstallEventLogCollector::OnCloudDpsFailed(
                  std::move(event));
 }
 
+void AppInstallEventLogCollector::OnInstallationStarted(
+    const std::string& package_name) {
+  if (!pending_packages_.count(package_name)) {
+    return;
+  }
+
+  auto event = std::make_unique<em::AppInstallReportLogEvent>();
+  event->set_event_type(em::AppInstallReportLogEvent::INSTALLATION_STARTED);
+  delegate_->Add(package_name, true /* gather_disk_space_info */,
+                 std::move(event));
+}
+
+void AppInstallEventLogCollector::OnInstallationFinished(
+    const std::string& package_name,
+    bool success) {
+  if (!pending_packages_.count(package_name)) {
+    return;
+  }
+
+  auto event = std::make_unique<em::AppInstallReportLogEvent>();
+  event->set_event_type(
+      success ? em::AppInstallReportLogEvent::INSTALLATION_FINISHED
+              : em::AppInstallReportLogEvent::INSTALLATION_FAILED);
+  delegate_->Add(package_name, true /* gather_disk_space_info */,
+                 std::move(event));
+}
+
 }  // namespace policy

chrome/browser/chromeos/policy/app_install_event_log_collector.h

@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_bridge.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chromeos/dbus/power_manager_client.h"
 #include "components/arc/common/policy.mojom.h"
 #include "net/base/network_change_notifier.h"
@@ -30,7 +31,8 @@ namespace policy {
 class AppInstallEventLogCollector
     : public chromeos::PowerManagerClient::Observer,
       public arc::ArcPolicyBridge::Observer,
-      public net::NetworkChangeNotifier::NetworkChangeObserver {
+      public net::NetworkChangeNotifier::NetworkChangeObserver,
+      public ArcAppListPrefs::Observer {
  public:
   // The delegate that events are forwarded to for inclusion in the log.
   class Delegate {
@@ -62,8 +64,7 @@ class AppInstallEventLogCollector
   ~AppInstallEventLogCollector() override;
 
   // Called whenever the list of pending app-install requests changes.
-  void OnPendingPackagesChanged(const std::set<std::string>& added,
-                                const std::set<std::string>& removed);
+  void OnPendingPackagesChanged(const std::set<std::string>& pending_packages);
 
   // Called in case of login and pending apps.
   void AddLoginEvent();
@@ -88,6 +89,11 @@ class AppInstallEventLogCollector
                         const std::string& package_name,
                         arc::mojom::InstallErrorReason reason) override;
 
+  // ArcAppListPrefs::Observer:
+  void OnInstallationStarted(const std::string& package_name) override;
+  void OnInstallationFinished(const std::string& package_name,
+                              bool success) override;
+
  private:
   Delegate* const delegate_;
   Profile* const profile_;
@@ -95,6 +101,9 @@ class AppInstallEventLogCollector
   // Whether the device is currently online.
   bool online_ = false;
 
+  // Set of apps whose push-install is currently pending.
+  std::set<std::string> pending_packages_;
+
   DISALLOW_COPY_AND_ASSIGN(AppInstallEventLogCollector);
 };
 

chrome/browser/chromeos/policy/app_install_event_log_collector_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_test.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -21,6 +22,7 @@
 #include "chromeos/dbus/fake_power_manager_client.h"
 #include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/network/network_handler.h"
+#include "components/arc/common/app.mojom.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/testing_pref_service.h"
@@ -131,9 +133,13 @@ class AppInstallEventLogCollectorTest : public testing::Test {
                               shill::kTypeEthernet, shill::kStateOffline,
                               true /* visible */);
     base::RunLoop().RunUntilIdle();
+
+    arc_app_test_.SetUp(profile_.get());
   }
 
   void TearDown() override {
+    arc_app_test_.TearDown();
+
     profile_.reset();
     chromeos::NetworkHandler::Shutdown();
     chromeos::DBusThreadManager::Shutdown();
@@ -169,6 +175,7 @@ class AppInstallEventLogCollectorTest : public testing::Test {
   chromeos::FakePowerManagerClient* power_manager_client() {
     return power_manager_client_;
   }
+  ArcAppListPrefs* app_prefs() { return arc_app_test_.arc_app_list_prefs(); }
 
   const std::set<std::string> packages_ = {kPackageName};
 
@@ -181,6 +188,7 @@ class AppInstallEventLogCollectorTest : public testing::Test {
   FakeAppInstallEventLogCollectorDelegate delegate_;
   TestingPrefServiceSimple pref_service_;
   chromeos::FakePowerManagerClient* power_manager_client_ = nullptr;
+  ArcAppTest arc_app_test_;
 
   DISALLOW_COPY_AND_ASSIGN(AppInstallEventLogCollectorTest);
 };
@@ -385,4 +393,56 @@ TEST_F(AppInstallEventLogCollectorTest, CloudDPSEvent) {
             delegate()->last_request().event.clouddps_response());
 }
 
+TEST_F(AppInstallEventLogCollectorTest, InstallPackages) {
+  arc::mojom::AppHost* const app_host = app_prefs();
+
+  std::unique_ptr<AppInstallEventLogCollector> collector =
+      std::make_unique<AppInstallEventLogCollector>(delegate(), profile(),
+                                                    packages_);
+
+  app_host->OnInstallationStarted(kPackageName);
+  ASSERT_EQ(1, delegate()->add_count());
+  EXPECT_EQ(em::AppInstallReportLogEvent::INSTALLATION_STARTED,
+            delegate()->last_event().event_type());
+  EXPECT_EQ(kPackageName, delegate()->last_request().package_name);
+  EXPECT_TRUE(delegate()->last_request().add_disk_space_info);
+
+  // kPackageName2 is not in the pending set.
+  app_host->OnInstallationStarted(kPackageName2);
+  EXPECT_EQ(1, delegate()->add_count());
+
+  arc::mojom::InstallationResult result;
+  result.package_name = kPackageName;
+  result.success = true;
+  app_host->OnInstallationFinished(
+      arc::mojom::InstallationResultPtr(result.Clone()));
+  EXPECT_EQ(2, delegate()->add_count());
+  EXPECT_EQ(em::AppInstallReportLogEvent::INSTALLATION_FINISHED,
+            delegate()->last_event().event_type());
+  EXPECT_EQ(kPackageName, delegate()->last_request().package_name);
+  EXPECT_TRUE(delegate()->last_request().add_disk_space_info);
+
+  collector->OnPendingPackagesChanged({kPackageName, kPackageName2});
+
+  // Now kPackageName2 is in the pending set.
+  app_host->OnInstallationStarted(kPackageName2);
+  EXPECT_EQ(3, delegate()->add_count());
+  EXPECT_EQ(em::AppInstallReportLogEvent::INSTALLATION_STARTED,
+            delegate()->last_event().event_type());
+  EXPECT_EQ(kPackageName2, delegate()->last_request().package_name);
+  EXPECT_TRUE(delegate()->last_request().add_disk_space_info);
+
+  result.package_name = kPackageName2;
+  result.success = false;
+  app_host->OnInstallationFinished(
+      arc::mojom::InstallationResultPtr(result.Clone()));
+  EXPECT_EQ(4, delegate()->add_count());
+  EXPECT_EQ(em::AppInstallReportLogEvent::INSTALLATION_FAILED,
+            delegate()->last_event().event_type());
+  EXPECT_EQ(kPackageName2, delegate()->last_request().package_name);
+  EXPECT_TRUE(delegate()->last_request().add_disk_space_info);
+
+  EXPECT_EQ(0, delegate()->add_for_all_count());
+}
+
 }  // namespace policy

chrome/browser/chromeos/policy/app_install_event_logger.cc

@@ -260,7 +260,7 @@ void AppInstallEventLogger::OnComplianceReportReceived(
   SetPref(arc::prefs::kArcPushInstallAppsPending, current_pending);
 
   if (!current_pending.empty()) {
-    UpdateCollector({} /* added */, removed);
+    UpdateCollector(current_pending);
   } else {
     StopCollector();
   }
@@ -290,13 +290,12 @@ void AppInstallEventLogger::SetPref(const std::string& pref_name,
 }
 
 void AppInstallEventLogger::UpdateCollector(
-    const std::set<std::string>& added,
-    const std::set<std::string>& removed) {
+    const std::set<std::string>& pending) {
   if (!log_collector_) {
     log_collector_ =
-        std::make_unique<AppInstallEventLogCollector>(this, profile_, added);
+        std::make_unique<AppInstallEventLogCollector>(this, profile_, pending);
   } else {
-    log_collector_->OnPendingPackagesChanged(added, removed);
+    log_collector_->OnPendingPackagesChanged(pending);
   }
 }
 
@@ -329,11 +328,9 @@ void AppInstallEventLogger::EvaluatePolicy(const policy::PolicyMap& policy,
   SetPref(arc::prefs::kArcPushInstallAppsPending, current_pending);
 
   if (!current_pending.empty()) {
+    UpdateCollector(current_pending);
     if (initial) {
-      UpdateCollector(current_pending /* added */, {} /* removed */);
       log_collector_->AddLoginEvent();
-    } else {
-      UpdateCollector(added, removed);
     }
   } else {
     StopCollector();

chrome/browser/chromeos/policy/app_install_event_logger.h

@@ -104,8 +104,7 @@ class AppInstallEventLogger : public AppInstallEventLogCollector::Delegate,
   // Informs the existing |log_collector_| that the list of pending app
   // push-install requests has changed or instantiates a new |log_collector_| if
   // none exists yet.
-  void UpdateCollector(const std::set<std::string>& added,
-                       const std::set<std::string>& removed);
+  void UpdateCollector(const std::set<std::string>& pending);
 
   // Destroys the |log_collector_|, if it exists.
   void StopCollector();

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

@@ -1565,8 +1565,14 @@ void ArcAppListPrefs::OnInstallationStarted(
     const base::Optional<std::string>& package_name) {
   ++installing_packages_count_;
 
-  if (package_name.has_value() && default_apps_.HasPackage(*package_name))
+  if (!package_name.has_value())
+    return;
+
+  if (default_apps_.HasPackage(*package_name))
     default_apps_installations_.insert(*package_name);
+
+  for (auto& observer : observer_list_)
+    observer.OnInstallationStarted(*package_name);
 }
 
 void ArcAppListPrefs::OnInstallationFinished(
@@ -1578,6 +1584,11 @@ void ArcAppListPrefs::OnInstallationFinished(
       HandlePackageRemoved(result->package_name);
   }
 
+  if (result) {
+    for (auto& observer : observer_list_)
+      observer.OnInstallationFinished(result->package_name, result->success);
+  }
+
   if (!installing_packages_count_) {
     VLOG(2) << "Received unexpected installation finished event";
     return;

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

@@ -160,6 +160,14 @@ class ArcAppListPrefs : public KeyedService,
     // Notifies sync date type controller the model is ready to start.
     virtual void OnPackageListInitialRefreshed() {}
 
+    // Notifies that installation of package started.
+    virtual void OnInstallationStarted(const std::string& package_name) {}
+
+    // Notifies that installation of package finished. |succeed| is set to true
+    // in case of success.
+    virtual void OnInstallationFinished(const std::string& package_name,
+                                        bool success) {}
+
    protected:
     virtual ~Observer() {}
   };

components/policy/proto/device_management_backend.proto

@@ -1491,6 +1491,12 @@ message AppInstallReportLogEvent {
     CONNECTIVITY_CHANGE = 8;
     // Session state changed
     SESSION_STATE_CHANGE = 9;
+    // Package installation started
+    INSTALLATION_STARTED = 10;
+    // Package installation finished
+    INSTALLATION_FINISHED = 11;
+    // Package installation failed
+    INSTALLATION_FAILED = 12;
   }
 
   // Enumerates the possible changes in session state.
@@ -1515,7 +1521,8 @@ message AppInstallReportLogEvent {
   optional EventType event_type = 2;
 
   // Total and available space on the stateful partition, in bytes. Set for
-  // event types SERVER_REQUEST, CLOUDDPS_RESPONSE and SUCCESS.
+  // event types SERVER_REQUEST, CLOUDDPS_RESPONSE, INSTALLATION_STARTED,
+  // INSTALLATION_FINISHED, INSTALLATION_FAILED and SUCCESS.
   optional int64 stateful_total = 3;
   optional int64 stateful_free = 4;