[Eche] Fix the issue that streaming success is recorded to wrong bucket

This change records connection status in AppsLaunchInfoProvider only when starting the app streaming and prevent the status being updated during an app streaming session. After the change, when app streaming is initialized from notification while connection status is failed and successfully started, the event will not be incorrectly logged to Eche.StreamEvent bucket. and click notification on chromebook to start app stream, meanwhile set both devices on same network. Verified that both initialization and start events are logged to Eche.StreamEvent.FromNotification.PreviousNetworkCheckFailed.Result Tested: Set both devices on different network, send message to phone Change-Id: I63a2fb4ace0c1ab4a976640313e3fdc7def3c682 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4548334 Commit-Queue: Pu Shi <pushi@google.com> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Reviewed-by: Jon Mann <jonmann@chromium.org> Cr-Commit-Position: refs/heads/main@{#1146774}
chromium · May 19, 2023 · 474902c · 474902c
1 parent 7b1759b
commit 474902c
Show file tree

Hide file tree

Showing 11 changed files with 56 additions and 77 deletions.
diff --git a/ash/system/eche/eche_tray.cc b/ash/system/eche/eche_tray.cc
@@ -611,8 +611,8 @@ void EcheTray::InitBubble(
     eche_app::mojom::ConnectionStatus last_connection_status,
     eche_app::mojom::AppStreamLaunchEntryPoint entry_point) {
   if (features::IsEcheNetworkConnectionStateEnabled() &&
-      last_connection_status ==
-          eche_app::mojom::ConnectionStatus::kConnectionStatusFailed &&
+      last_connection_status !=
+          eche_app::mojom::ConnectionStatus::kConnectionStatusConnected &&
       entry_point == eche_app::mojom::AppStreamLaunchEntryPoint::NOTIFICATION) {
     base::UmaHistogramEnumeration(
         "Eche.StreamEvent.FromNotification.PreviousNetworkCheckFailed.Result",

diff --git a/ash/webui/eche_app_ui/apps_launch_info_provider.cc b/ash/webui/eche_app_ui/apps_launch_info_provider.cc
@@ -6,28 +6,17 @@
 
 #include "ash/webui/eche_app_ui/mojom/eche_app.mojom-shared.h"
 
-namespace ash {
-namespace eche_app {
+namespace ash::eche_app {
 
 AppsLaunchInfoProvider::AppsLaunchInfoProvider(
     EcheConnectionStatusHandler* connection_handler)
-    : eche_connection_status_handler_(connection_handler) {
-  eche_connection_status_handler_->AddObserver(this);
-}
-
-AppsLaunchInfoProvider::~AppsLaunchInfoProvider() {
-  eche_connection_status_handler_->RemoveObserver(this);
-}
-
-void AppsLaunchInfoProvider::OnConnectionStatusForUiChanged(
-    mojom::ConnectionStatus connection_status) {
-  last_connection_ = connection_status;
-}
+    : eche_connection_status_handler_(connection_handler) {}
 
-void AppsLaunchInfoProvider::SetEntryPoint(
+void AppsLaunchInfoProvider::SetAppLaunchInfo(
     mojom::AppStreamLaunchEntryPoint entry_point) {
   entry_point_ = entry_point;
+  last_connection_ =
+      eche_connection_status_handler_->connection_status_for_ui();
 }
 
-}  // namespace eche_app
-}  // namespace ash
+}  // namespace ash::eche_app
diff --git a/ash/webui/eche_app_ui/apps_launch_info_provider.h b/ash/webui/eche_app_ui/apps_launch_info_provider.h
@@ -15,21 +15,17 @@ namespace ash {
 namespace eche_app {
 
 // A class to store app stream entry point and last connection status.
-class AppsLaunchInfoProvider : public EcheConnectionStatusHandler::Observer {
+class AppsLaunchInfoProvider {
  public:
   explicit AppsLaunchInfoProvider(EcheConnectionStatusHandler*);
-  ~AppsLaunchInfoProvider() override;
+  ~AppsLaunchInfoProvider() = default;
 
   AppsLaunchInfoProvider(const AppsLaunchInfoProvider&) = delete;
   AppsLaunchInfoProvider& operator=(const AppsLaunchInfoProvider&) = delete;
 
-  // EcheConnectionStatusHandler::Observer:
-  void OnConnectionStatusForUiChanged(
-      mojom::ConnectionStatus connection_status) override;
+  void SetAppLaunchInfo(mojom::AppStreamLaunchEntryPoint entry_point);
 
-  void SetEntryPoint(mojom::AppStreamLaunchEntryPoint entry_point);
-
-  mojom::ConnectionStatus GetConnectionStatusForUi() {
+  mojom::ConnectionStatus GetConnectionStatusFromLastAttempt() {
     return last_connection_;
   }
 

diff --git a/ash/webui/eche_app_ui/apps_launch_info_provider_unittest.cc b/ash/webui/eche_app_ui/apps_launch_info_provider_unittest.cc
@@ -37,16 +37,14 @@ class AppsLaunchInfoProviderTest : public testing::Test {
     handler_.reset();
   }
 
-  void NotifyConnectionStatusForUiChanged(mojom::ConnectionStatus status) {
-    handler_->SetConnectionStatusForUi(status);
-  }
-
-  mojom::ConnectionStatus GetLastConnectionStatus() {
-    return provider_->GetConnectionStatusForUi();
+  mojom::ConnectionStatus GetConnectionStatusFromLastAttempt() {
+    return provider_->GetConnectionStatusFromLastAttempt();
   }
 
-  void SetEntryPoint(mojom::AppStreamLaunchEntryPoint entry_point) {
-    provider_->SetEntryPoint(entry_point);
+  void SetAppLaunchInfo(mojom::AppStreamLaunchEntryPoint entry_point,
+                        mojom::ConnectionStatus status) {
+    handler_->SetConnectionStatusForUi(status);
+    provider_->SetAppLaunchInfo(entry_point);
   }
 
   mojom::AppStreamLaunchEntryPoint GetEntryPoint() {
@@ -60,42 +58,28 @@ class AppsLaunchInfoProviderTest : public testing::Test {
   std::unique_ptr<AppsLaunchInfoProvider> provider_;
 };
 
-TEST_F(AppsLaunchInfoProviderTest, OnConnectionStatusForUiChanged) {
-  EXPECT_EQ(GetLastConnectionStatus(),
-            mojom::ConnectionStatus::kConnectionStatusDisconnected);
-
-  NotifyConnectionStatusForUiChanged(
-      mojom::ConnectionStatus::kConnectionStatusConnecting);
-  EXPECT_EQ(GetLastConnectionStatus(),
-            mojom::ConnectionStatus::kConnectionStatusConnecting);
-
-  NotifyConnectionStatusForUiChanged(
-      mojom::ConnectionStatus::kConnectionStatusConnected);
-  EXPECT_EQ(GetLastConnectionStatus(),
-            mojom::ConnectionStatus::kConnectionStatusConnected);
-
-  NotifyConnectionStatusForUiChanged(
-      mojom::ConnectionStatus::kConnectionStatusFailed);
-  EXPECT_EQ(GetLastConnectionStatus(),
-            mojom::ConnectionStatus::kConnectionStatusFailed);
-
-  NotifyConnectionStatusForUiChanged(
-      mojom::ConnectionStatus::kConnectionStatusDisconnected);
-  EXPECT_EQ(GetLastConnectionStatus(),
-            mojom::ConnectionStatus::kConnectionStatusDisconnected);
-}
-
 TEST_F(AppsLaunchInfoProviderTest, SetEntryPoint) {
   EXPECT_EQ(GetEntryPoint(), mojom::AppStreamLaunchEntryPoint::UNKNOWN);
+  EXPECT_EQ(GetConnectionStatusFromLastAttempt(),
+            mojom::ConnectionStatus::kConnectionStatusDisconnected);
 
-  SetEntryPoint(mojom::AppStreamLaunchEntryPoint::NOTIFICATION);
+  SetAppLaunchInfo(mojom::AppStreamLaunchEntryPoint::NOTIFICATION,
+                   mojom::ConnectionStatus::kConnectionStatusConnecting);
   EXPECT_EQ(GetEntryPoint(), mojom::AppStreamLaunchEntryPoint::NOTIFICATION);
+  EXPECT_EQ(GetConnectionStatusFromLastAttempt(),
+            mojom::ConnectionStatus::kConnectionStatusConnecting);
 
-  SetEntryPoint(mojom::AppStreamLaunchEntryPoint::APPS_LIST);
+  SetAppLaunchInfo(mojom::AppStreamLaunchEntryPoint::APPS_LIST,
+                   mojom::ConnectionStatus::kConnectionStatusConnected);
   EXPECT_EQ(GetEntryPoint(), mojom::AppStreamLaunchEntryPoint::APPS_LIST);
+  EXPECT_EQ(GetConnectionStatusFromLastAttempt(),
+            mojom::ConnectionStatus::kConnectionStatusConnected);
 
-  SetEntryPoint(mojom::AppStreamLaunchEntryPoint::RECENT_APPS);
+  SetAppLaunchInfo(mojom::AppStreamLaunchEntryPoint::RECENT_APPS,
+                   mojom::ConnectionStatus::kConnectionStatusFailed);
   EXPECT_EQ(GetEntryPoint(), mojom::AppStreamLaunchEntryPoint::RECENT_APPS);
+  EXPECT_EQ(GetConnectionStatusFromLastAttempt(),
+            mojom::ConnectionStatus::kConnectionStatusFailed);
 }
 
 }  // namespace ash::eche_app
diff --git a/ash/webui/eche_app_ui/eche_connection_status_handler.h b/ash/webui/eche_app_ui/eche_connection_status_handler.h
@@ -66,6 +66,10 @@ class EcheConnectionStatusHandler : public mojom::ConnectionStatusObserver {
 
   void Bind(mojo::PendingReceiver<mojom::ConnectionStatusObserver> receiver);
 
+  mojom::ConnectionStatus connection_status_for_ui() const {
+    return connection_status_for_ui_;
+  }
+
  private:
   friend class EcheConnectionStatusHandlerTest;
 
@@ -79,9 +83,6 @@ class EcheConnectionStatusHandler : public mojom::ConnectionStatusObserver {
   void set_feature_status_for_test(FeatureStatus feature_status) {
     feature_status_ = feature_status;
   }
-  mojom::ConnectionStatus get_connection_status_for_ui_for_test() const {
-    return connection_status_for_ui_;
-  }
 
   mojom::ConnectionStatus connection_status_for_ui_ =
       mojom::ConnectionStatus::kConnectionStatusDisconnected;

diff --git a/ash/webui/eche_app_ui/eche_connection_status_handler_unittest.cc b/ash/webui/eche_app_ui/eche_connection_status_handler_unittest.cc
@@ -137,7 +137,7 @@ class EcheConnectionStatusHandlerTest : public testing::Test {
   }
 
   mojom::ConnectionStatus GetConnectionStatusForUi() const {
-    return handler_->get_connection_status_for_ui_for_test();
+    return handler_->connection_status_for_ui();
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;

diff --git a/ash/webui/eche_app_ui/eche_notification_click_handler.cc b/ash/webui/eche_app_ui/eche_notification_click_handler.cc
@@ -52,7 +52,7 @@ void EcheNotificationClickHandler::HandleNotificationClick(
       base::UmaHistogramEnumeration(
           "Eche.AppStream.LaunchAttempt",
           mojom::AppStreamLaunchEntryPoint::NOTIFICATION);
-      apps_launch_info_provider_->SetEntryPoint(
+      apps_launch_info_provider_->SetAppLaunchInfo(
           mojom::AppStreamLaunchEntryPoint::NOTIFICATION);
       launch_app_helper_->LaunchEcheApp(
           notification_id, app_metadata.package_name,

diff --git a/ash/webui/eche_app_ui/eche_recent_app_click_handler.cc b/ash/webui/eche_app_ui/eche_recent_app_click_handler.cc
@@ -87,7 +87,7 @@ void EcheRecentAppClickHandler::OnRecentAppClicked(
       base::UmaHistogramEnumeration("Eche.AppStream.LaunchAttempt", entrypoint);
 
       to_stream_apps_.emplace_back(app_metadata);
-      apps_launch_info_provider_->SetEntryPoint(entrypoint);
+      apps_launch_info_provider_->SetAppLaunchInfo(entrypoint);
       launch_app_helper_->LaunchEcheApp(
           /*notification_id=*/absl::nullopt, app_metadata.package_name,
           app_metadata.visible_app_name, app_metadata.user_id,

diff --git a/ash/webui/eche_app_ui/eche_signaler.cc b/ash/webui/eche_app_ui/eche_signaler.cc
@@ -198,7 +198,7 @@ void EcheSignaler::RecordSignalingTimeout() {
   if (eche_tray && eche_tray->IsBackgroundConnectionAttemptInProgress()) {
     base::UmaHistogramEnumeration("Eche.NetworkCheck.FailureReason",
                                   probably_connection_failed_reason_);
-  } else if (apps_launch_info_provider_->GetConnectionStatusForUi() ==
+  } else if (apps_launch_info_provider_->GetConnectionStatusFromLastAttempt() ==
                  mojom::ConnectionStatus::kConnectionStatusFailed &&
              apps_launch_info_provider_->entry_point() ==
                  mojom::AppStreamLaunchEntryPoint::NOTIFICATION) {

diff --git a/ash/webui/eche_app_ui/eche_stream_status_change_handler.cc b/ash/webui/eche_app_ui/eche_stream_status_change_handler.cc
@@ -37,10 +37,18 @@ void EcheStreamStatusChangeHandler::OnStreamStatusChanged(
                << status;
   NotifyStreamStatusChanged(status);
 
+  // Note that the stream status and connection status from
+  // |apps_launch_info_provider_| can be out of sync. This is because the
+  // pre-warm connection may not havbe been established (e.g. launched from a
+  // notification when Phone Hub just get connected) and the value is stored in
+  // |apps_launch_info_provider_| when app streaming gets initialized. This is
+  // very likely to fail to start actual streaming and we are recording these
+  // events to separate bucket to prevent it from polluting our real success
+  // metrics.
   if (status == mojom::StreamStatus::kStreamStatusStarted) {
     if (features::IsEcheNetworkConnectionStateEnabled() &&
-        apps_launch_info_provider_->GetConnectionStatusForUi() ==
-            mojom::ConnectionStatus::kConnectionStatusFailed &&
+        apps_launch_info_provider_->GetConnectionStatusFromLastAttempt() !=
+            mojom::ConnectionStatus::kConnectionStatusConnected &&
         apps_launch_info_provider_->entry_point() ==
             mojom::AppStreamLaunchEntryPoint::NOTIFICATION) {
       base::UmaHistogramEnumeration(

diff --git a/chrome/browser/ash/eche_app/eche_app_manager_factory.cc b/chrome/browser/ash/eche_app/eche_app_manager_factory.cc
@@ -109,11 +109,12 @@ void LaunchWebApp(const std::string& package_name,
   }
   const auto gurl = GURL(url);
 
-  return LaunchBubble(gurl, icon, visible_name, phone_name,
-                      apps_launch_info_provider->GetConnectionStatusForUi(),
-                      apps_launch_info_provider->entry_point(),
-                      base::BindOnce(&EnsureStreamClose, profile),
-                      base::BindRepeating(&StreamGoBack, profile));
+  return LaunchBubble(
+      gurl, icon, visible_name, phone_name,
+      apps_launch_info_provider->GetConnectionStatusFromLastAttempt(),
+      apps_launch_info_provider->entry_point(),
+      base::BindOnce(&EnsureStreamClose, profile),
+      base::BindRepeating(&StreamGoBack, profile));
 }
 
 void RelaunchLast(Profile* profile) {