Skip to content

Commit

Permalink
[Eche] Introduce new control logic to EcheConnectionStatusHandler
Browse files Browse the repository at this point in the history
This change allows the EcheConnectionStatusHandler to update the
RecentAppsInteractionHandler to change the UI state under certain
conditions  as opposed to just notifying whenever the ConnectionStatus
changes.

Test: manually verified App Streaming still works when
kEcheNetworkConnectionState is disabled, added unit tests. When
kEcheNetworkConnectionState is enabled, it will stay on the loading
state (correct behavior until the rest of the feature lands).

Bug: None
Change-Id: I4f70df5d3bb0560d088d15e44798f74d2e1f4a8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4359150
Commit-Queue: Crisrael Lucero <crisrael@google.com>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Reviewed-by: Pu Shi <pushi@google.com>
Cr-Commit-Position: refs/heads/main@{#1120592}
  • Loading branch information
crisrael authored and Chromium LUCI CQ committed Mar 22, 2023
1 parent 48f68f0 commit c0f13dc
Show file tree
Hide file tree
Showing 7 changed files with 316 additions and 19 deletions.
12 changes: 10 additions & 2 deletions ash/webui/eche_app_ui/BUILD.gn
Expand Up @@ -22,13 +22,21 @@ static_library("eche_app_ui_pref") {
deps = [ "//base:base" ]
}

static_library("feature_status") {
sources = [
"feature_status.cc",
"feature_status.h",
]
}

static_library("eche_connection_status") {
sources = [
"eche_connection_status_handler.cc",
"eche_connection_status_handler.h",
]

deps = [
":feature_status",
"//ash/constants",
"//ash/webui/eche_app_ui/mojom",
"//base:base",
Expand Down Expand Up @@ -82,8 +90,6 @@ static_library("eche_app_ui") {
"eche_tray_stream_status_observer.h",
"eche_uid_provider.cc",
"eche_uid_provider.h",
"feature_status.cc",
"feature_status.h",
"feature_status_provider.cc",
"feature_status_provider.h",
"launch_app_helper.cc",
Expand All @@ -101,6 +107,7 @@ static_library("eche_app_ui") {
deps = [
":eche_app_ui_pref",
":eche_connection_status",
":feature_status",
"//ash",
"//ash/constants",
"//ash/public/cpp",
Expand Down Expand Up @@ -178,6 +185,7 @@ source_set("unit_tests") {
":eche_app_ui",
":eche_app_ui_pref",
":eche_connection_status",
":feature_status",
":test_support",
"//ash:test_support",
"//ash/public/cpp",
Expand Down
107 changes: 103 additions & 4 deletions ash/webui/eche_app_ui/eche_connection_status_handler.cc
Expand Up @@ -5,6 +5,7 @@
#include "ash/webui/eche_app_ui/eche_connection_status_handler.h"

#include "ash/constants/ash_features.h"
#include "base/timer/timer.h"
#include "chromeos/ash/components/multidevice/logging/logging.h"

namespace ash::eche_app {
Expand All @@ -13,12 +14,62 @@ EcheConnectionStatusHandler::EcheConnectionStatusHandler() = default;

EcheConnectionStatusHandler::~EcheConnectionStatusHandler() = default;

void EcheConnectionStatusHandler::Observer::OnConnectionStatusChanged(
mojom::ConnectionStatus connection_status) {}
void EcheConnectionStatusHandler::Observer::OnConnectionStatusForUiChanged(
mojom::ConnectionStatus connection_status) {}
void EcheConnectionStatusHandler::Observer::
OnRequestBackgroundConnectionAttempt() {}
void EcheConnectionStatusHandler::Observer::OnPhoneHubDisconnected() {}

void EcheConnectionStatusHandler::OnConnectionStatusChanged(
mojom::ConnectionStatus connection_status) {
if (features::IsEcheNetworkConnectionStateEnabled()) {
PA_LOG(INFO) << "echeapi EcheConnectionStatusHandler "
<< " OnConnectionStatusChanged " << connection_status;
NotifyConnectionStatusChanged(connection_status);
if (!features::IsEcheNetworkConnectionStateEnabled()) {
return;
}

PA_LOG(INFO) << "echeapi EcheConnectionStatusHandler "
<< " OnConnectionStatusChanged " << connection_status;
NotifyConnectionStatusChanged(connection_status);

// Anytime we have a successful connection to the phone (app stream or
// prewarm) we should make sure the UI is enabled.
if (connection_status ==
mojom::ConnectionStatus::kConnectionStatusConnected) {
SetConnectionStatusForUi(connection_status);
return;
}

// Only track failures triggered from background connection attempts (app
// stream failures can happen for other reasons).
if (connection_status == mojom::ConnectionStatus::kConnectionStatusFailed) {
SetConnectionStatusForUi(connection_status);
}
}

void EcheConnectionStatusHandler::OnFeatureStatusChanged(
FeatureStatus feature_status) {
feature_status_ = feature_status;
switch (feature_status) {
case FeatureStatus::kIneligible:
case FeatureStatus::kDisabled:
case FeatureStatus::kDisconnected:
case FeatureStatus::kDependentFeature:
case FeatureStatus::kDependentFeaturePending:
ResetConnectionStatus();
break;

case FeatureStatus::kConnecting:
break;

case FeatureStatus::kConnected:
status_check_delay_timer_ = std::make_unique<base::OneShotTimer>();
status_check_delay_timer_->Start(
FROM_HERE, base::Seconds(1),
base::BindOnce(
&EcheConnectionStatusHandler::CheckConnectionStatusForUi,
base::Unretained(this)));
break;
}
}

Expand All @@ -30,6 +81,41 @@ void EcheConnectionStatusHandler::RemoveObserver(Observer* observer) {
observer_list_.RemoveObserver(observer);
}

void EcheConnectionStatusHandler::SetConnectionStatusForUi(
mojom::ConnectionStatus connection_status) {
last_update_timestamp_ = base::Time::Now();
if (connection_status_for_ui_ == connection_status) {
return;
}
connection_status_for_ui_ = connection_status;
NotifyConnectionStatusForUiChanged(connection_status);
}

void EcheConnectionStatusHandler::ResetConnectionStatus() {
last_update_timestamp_ = base::Time();
connection_status_for_ui_ =
mojom::ConnectionStatus::kConnectionStatusConnecting;
}

void EcheConnectionStatusHandler::CheckConnectionStatusForUi() {
if (feature_status_ != FeatureStatus::kConnected) {
return;
}

// TODO(b/271478560): Make the time conditionals configurable through feature
// flags in order to tune them in the future.
base::TimeDelta time_since_last_check =
base::Time::Now() - last_update_timestamp_;
if (time_since_last_check > base::Seconds(10)) {
NotifyRequestBackgroundConnectionAttempt();
}
if (time_since_last_check > base::Minutes(10)) {
connection_status_for_ui_ =
mojom::ConnectionStatus::kConnectionStatusConnecting;
}
NotifyConnectionStatusForUiChanged(connection_status_for_ui_);
}

void EcheConnectionStatusHandler::Bind(
mojo::PendingReceiver<mojom::ConnectionStatusObserver> receiver) {
connection_status_receiver_.reset();
Expand All @@ -43,4 +129,17 @@ void EcheConnectionStatusHandler::NotifyConnectionStatusChanged(
}
}

void EcheConnectionStatusHandler::NotifyConnectionStatusForUiChanged(
mojom::ConnectionStatus connection_status) {
for (auto& observer : observer_list_) {
observer.OnConnectionStatusForUiChanged(connection_status);
}
}

void EcheConnectionStatusHandler::NotifyRequestBackgroundConnectionAttempt() {
for (auto& observer : observer_list_) {
observer.OnRequestBackgroundConnectionAttempt();
}
}

} // namespace ash::eche_app
46 changes: 43 additions & 3 deletions ash/webui/eche_app_ui/eche_connection_status_handler.h
Expand Up @@ -5,6 +5,7 @@
#ifndef ASH_WEBUI_ECHE_APP_UI_ECHE_CONNECTION_STATUS_HANDLER_H_
#define ASH_WEBUI_ECHE_APP_UI_ECHE_CONNECTION_STATUS_HANDLER_H_

#include "ash/webui/eche_app_ui/feature_status.h"
#include "ash/webui/eche_app_ui/mojom/eche_app.mojom.h"
#include "base/observer_list.h"
#include "base/observer_list_types.h"
Expand All @@ -21,8 +22,18 @@ class EcheConnectionStatusHandler : public mojom::ConnectionStatusObserver {
public:
~Observer() override = default;

// Includes connection status changes for all connections (app stream +
// pre-warm).
virtual void OnConnectionStatusChanged(
mojom::ConnectionStatus connection_status) = 0;
mojom::ConnectionStatus connection_status);

// For determining when app streaming is allowed in UI.
virtual void OnConnectionStatusForUiChanged(
mojom::ConnectionStatus connection_status);

virtual void OnRequestBackgroundConnectionAttempt();

virtual void OnPhoneHubDisconnected();
};

EcheConnectionStatusHandler();
Expand All @@ -39,12 +50,41 @@ class EcheConnectionStatusHandler : public mojom::ConnectionStatusObserver {
void AddObserver(Observer* observer);
void RemoveObserver(Observer* observer);

// Checks on the status of the connection to be used for deciding whether app
// streaming should be allowed on the current connection. Triggers
// OnConnectionStatusForUiChanged().
void CheckConnectionStatusForUi();
void SetConnectionStatusForUi(mojom::ConnectionStatus connection_status);

// TODO(b/274530047): Refactor to make this a real observer / actually
// override.
// EcheFeatureStatusProvider::Observer:
void OnFeatureStatusChanged(FeatureStatus feature_status);

void Bind(mojo::PendingReceiver<mojom::ConnectionStatusObserver> receiver);

protected:
private:
friend class EcheConnectionStatusHandlerTest;

void NotifyConnectionStatusChanged(mojom::ConnectionStatus connection_status);
void NotifyConnectionStatusForUiChanged(
mojom::ConnectionStatus connection_status);
void NotifyRequestBackgroundConnectionAttempt();

private:
void ResetConnectionStatus();

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;
base::Time last_update_timestamp_ = base::Time();
std::unique_ptr<base::OneShotTimer> status_check_delay_timer_{};
FeatureStatus feature_status_ = FeatureStatus::kDisconnected;
mojo::Receiver<mojom::ConnectionStatusObserver> connection_status_receiver_{
this};
base::ObserverList<Observer> observer_list_;
Expand Down

0 comments on commit c0f13dc

Please sign in to comment.