Network state handling in update required screen

This CL adds the following network handling for the update required
screen :-
1) Metered(cellular) network at the start - Show dialog to connect to a
non metered network or accept update over metered. If good network is
connected, update starts. If accepted to update, update starts.
2) No network at the start - User is prompted to connect to a network.
When connected to a good network, update starts.
3) Good connection is lost and metered connected while updating -
User prompted to accept update over metered. If accepted, update
starts.

We register for the network updates only up to starting the update
process. Post that, the network changes are reflected in the update
engine result and thus, handled for there.

Bug: 1048607
Change-Id: I50d939c0152f76f4456830cdd5e07136819b91a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091534
Commit-Queue: Saurabh Nijhara <snijhara@google.com>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751463}

chrome/browser/chromeos/login/screens/update_required_screen.cc

@@ -47,7 +47,6 @@ UpdateRequiredScreen::UpdateRequiredScreen(UpdateRequiredView* view,
       histogram_helper_(
           std::make_unique<ErrorScreensHistogramHelper>("UpdateRequired")),
       version_updater_(std::make_unique<VersionUpdater>(this)),
-      network_state_helper_(std::make_unique<login::NetworkStateHelper>()),
       clock_(base::DefaultClock::GetInstance()) {
   error_message_delay_ = kDelayErrorMessage;
   if (view_)
@@ -55,7 +54,7 @@ UpdateRequiredScreen::UpdateRequiredScreen(UpdateRequiredView* view,
 }
 
 UpdateRequiredScreen::~UpdateRequiredScreen() {
-  UnsubscribeNetworkNotification();
+  StopObservingNetworkState();
   if (view_)
     view_->Unbind();
 }
@@ -67,9 +66,6 @@ void UpdateRequiredScreen::OnViewDestroyed(UpdateRequiredView* view) {
 
 void UpdateRequiredScreen::ShowImpl() {
   ash::LoginScreen::Get()->SetAllowLoginAsGuest(false);
-  RefreshNetworkState();
-  SubscribeNetworkNotification();
-
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
   view_->SetEnterpriseAndDeviceName(connector->GetEnterpriseDisplayDomain(),
@@ -84,6 +80,8 @@ void UpdateRequiredScreen::ShowImpl() {
       view_->Show();
     }
   }
+  // Check network state to set initial screen UI.
+  RefreshNetworkState();
   version_updater_->GetEolInfo(base::BindOnce(
       &UpdateRequiredScreen::OnGetEolInfo, weak_factory_.GetWeakPtr()));
 }
@@ -96,6 +94,11 @@ void UpdateRequiredScreen::OnGetEolInfo(
     EnsureScreenIsShown();
     if (view_)
       view_->SetUIState(UpdateRequiredView::EOL_REACHED);
+  } else {
+    // UI state does not change for EOL devices.
+    // Subscribe to network state change notifications to adapt the UI as
+    // network changes till update is started.
+    ObserveNetworkState();
   }
 }
 
@@ -103,7 +106,7 @@ void UpdateRequiredScreen::HideImpl() {
   if (view_)
     view_->Hide();
   is_shown_ = false;
-  UnsubscribeNetworkNotification();
+  StopObservingNetworkState();
 }
 
 void UpdateRequiredScreen::OnUserAction(const std::string& action_id) {
@@ -112,7 +115,16 @@ void UpdateRequiredScreen::OnUserAction(const std::string& action_id) {
   } else if (action_id == kUserActionUpdateButtonClicked) {
     OnUpdateButtonClicked();
   } else if (action_id == kUserActionAcceptUpdateOverCellular) {
-    version_updater_->SetUpdateOverCellularOneTimePermission();
+    if (version_updater_->update_info().status.current_operation() ==
+        update_engine::Operation::NEED_PERMISSION_TO_UPDATE) {
+      version_updater_->SetUpdateOverCellularOneTimePermission();
+    } else {
+      // This is to handle the case when metered network screen is shown at the
+      // start and user accepts update over it.
+      metered_network_update_permission = true;
+      StopObservingNetworkState();
+      version_updater_->StartNetworkCheck();
+    }
   } else if (action_id == kUserActionRejectUpdateOverCellular) {
     version_updater_->RejectUpdateOverCellular();
     version_updater_->StartExitUpdate(VersionUpdater::Result::UPDATE_ERROR);
@@ -121,12 +133,8 @@ void UpdateRequiredScreen::OnUserAction(const std::string& action_id) {
   }
 }
 
-void UpdateRequiredScreen::NetworkConnectionStateChanged(
-    const NetworkState* network) {
-  RefreshNetworkState();
-}
-
 void UpdateRequiredScreen::DefaultNetworkChanged(const NetworkState* network) {
+  // Refresh the screen UI to reflect network state.
   RefreshNetworkState();
 }
 
@@ -134,7 +142,29 @@ void UpdateRequiredScreen::RefreshNetworkState() {
   if (!view_)
     return;
 
-  view_->SetIsConnected(network_state_helper_->IsConnected());
+  const NetworkState* network =
+      NetworkHandler::Get()->network_state_handler()->DefaultNetwork();
+  // Set the UI state as per the current network state.
+  // If device was connected to a good network at the start, we are already
+  // showing (by default) the update required screen. If network switches from
+  // one good network to another, it has no change in the UI state. This is only
+  // till the update process is triggered. Post that, update engine status
+  // drives the UI state.
+  if (!network || !network->IsConnectedState()) {
+    view_->SetUIState(UpdateRequiredView::UPDATE_NO_NETWORK);
+    waiting_for_connection_ = true;
+  } else if (network->IsUsingMobileData()) {
+    view_->SetUIState(UpdateRequiredView::UPDATE_NEED_PERMISSION);
+    waiting_for_connection_ = true;
+  } else if (waiting_for_connection_) {
+    // Network is good for the update process to start. Start the update process
+    // and unsubscribe from the network change notifications as any change in
+    // network state is reflected in the update engine result.
+    waiting_for_connection_ = false;
+    view_->SetUIState(UpdateRequiredView::UPDATE_PROCESS);
+    StopObservingNetworkState();
+    version_updater_->StartNetworkCheck();
+  }
 }
 
 void UpdateRequiredScreen::RefreshView(
@@ -144,11 +174,11 @@ void UpdateRequiredScreen::RefreshView(
 
   if (update_info.requires_permission_for_cellular) {
     view_->SetUIState(UpdateRequiredView::UPDATE_NEED_PERMISSION);
-    waiting_for_permission_ = true;
-  } else if (waiting_for_permission_) {
+    waiting_for_connection_ = true;
+  } else if (waiting_for_connection_) {
     // Return UI state after getting permission.
     view_->SetUIState(UpdateRequiredView::UPDATE_PROCESS);
-    waiting_for_permission_ = false;
+    waiting_for_connection_ = false;
   }
 
   view_->SetUpdateProgressUnavailable(update_info.progress_unavailable);
@@ -158,7 +188,7 @@ void UpdateRequiredScreen::RefreshView(
   view_->SetEstimatedTimeLeft(update_info.estimated_time_left_in_secs);
 }
 
-void UpdateRequiredScreen::SubscribeNetworkNotification() {
+void UpdateRequiredScreen::ObserveNetworkState() {
   if (!is_network_subscribed_) {
     is_network_subscribed_ = true;
     NetworkHandler::Get()->network_state_handler()->AddObserver(this,
@@ -166,7 +196,7 @@ void UpdateRequiredScreen::SubscribeNetworkNotification() {
   }
 }
 
-void UpdateRequiredScreen::UnsubscribeNetworkNotification() {
+void UpdateRequiredScreen::StopObservingNetworkState() {
   if (is_network_subscribed_) {
     is_network_subscribed_ = false;
     NetworkHandler::Get()->network_state_handler()->RemoveObserver(this,
@@ -186,6 +216,9 @@ void UpdateRequiredScreen::OnUpdateButtonClicked() {
   if (view_)
     view_->SetUIState(UpdateRequiredView::UPDATE_PROCESS);
 
+  // Do not need network notification now as UI state depends on the result
+  // received from the update engine.
+  StopObservingNetworkState();
   version_updater_->StartNetworkCheck();
 }
 
@@ -258,8 +291,14 @@ void UpdateRequiredScreen::UpdateInfoChanged(
     case update_engine::Operation::DOWNLOADING:
     case update_engine::Operation::VERIFYING:
     case update_engine::Operation::FINALIZING:
+      EnsureScreenIsShown();
+      break;
     case update_engine::Operation::NEED_PERMISSION_TO_UPDATE:
       EnsureScreenIsShown();
+      if (metered_network_update_permission) {
+        version_updater_->SetUpdateOverCellularOneTimePermission();
+        return;
+      }
       break;
     case update_engine::Operation::UPDATED_NEED_REBOOT:
       EnsureScreenIsShown();

chrome/browser/chromeos/login/screens/update_required_screen.h

@@ -26,10 +26,6 @@ namespace chromeos {
 class ErrorScreensHistogramHelper;
 class UpdateRequiredView;
 
-namespace login {
-class NetworkStateHelper;
-}  // namespace login
-
 // Controller for the update required screen.
 class UpdateRequiredScreen : public BaseScreen,
                              public VersionUpdater::Delegate,
@@ -75,17 +71,16 @@ class UpdateRequiredScreen : public BaseScreen,
   void OnUpdateButtonClicked();
 
   // NetworkStateHandlerObserver:
-  void NetworkConnectionStateChanged(const NetworkState* network) override;
   void DefaultNetworkChanged(const NetworkState* network) override;
 
   void RefreshNetworkState();
   void RefreshView(const VersionUpdater::UpdateInfo& update_info);
 
   // Subscribes to network change notifications.
-  void SubscribeNetworkNotification();
+  void ObserveNetworkState();
 
   // Unsubscribes from network change notifications.
-  void UnsubscribeNetworkNotification();
+  void StopObservingNetworkState();
 
   void HideErrorMessage();
 
@@ -114,10 +109,10 @@ class UpdateRequiredScreen : public BaseScreen,
   bool first_time_shown_ = true;
   bool is_updating_now_ = false;
   bool waiting_for_reboot_ = false;
-  bool waiting_for_permission_ = false;
+  bool waiting_for_connection_ = false;
+  bool metered_network_update_permission = false;
 
   std::unique_ptr<VersionUpdater> version_updater_;
-  std::unique_ptr<login::NetworkStateHelper> network_state_helper_;
 
   // Timer for the captive portal detector to show portal login page.
   // If redirect did not happen during this delay, error message is shown

chrome/browser/chromeos/login/screens/update_required_screen_unittest.cc

@@ -20,6 +20,7 @@
 #include "chromeos/dbus/fake_update_engine_client.h"
 #include "chromeos/dbus/update_engine_client.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state_test_helper.h"
 #include "chromeos/network/portal_detector/mock_network_portal_detector.h"
 #include "chromeos/network/portal_detector/network_portal_detector.h"
 #include "chromeos/tpm/stub_install_attributes.h"
@@ -54,6 +55,7 @@ class UpdateRequiredScreenUnitTest : public testing::Test {
     fake_update_engine_client_ = new FakeUpdateEngineClient();
     DBusThreadManager::GetSetterForTesting()->SetUpdateEngineClient(
         std::unique_ptr<UpdateEngineClient>(fake_update_engine_client_));
+
     NetworkHandler::Initialize();
     mock_network_portal_detector_ = new MockNetworkPortalDetector();
     network_portal_detector::SetNetworkPortalDetector(
@@ -70,6 +72,10 @@ class UpdateRequiredScreenUnitTest : public testing::Test {
 
     update_required_screen_->GetVersionUpdaterForTesting()
         ->set_wait_for_reboot_time_for_testing(base::TimeDelta::FromSeconds(0));
+
+    network_state_test_helper_ =
+        std::make_unique<chromeos::NetworkStateTestHelper>(
+            true /*use_default_devices_and_services*/);
   }
 
   void TearDown() override {
@@ -78,6 +84,7 @@ class UpdateRequiredScreenUnitTest : public testing::Test {
     update_required_screen_.reset();
     mock_error_view_.reset();
     mock_error_screen_.reset();
+    network_state_test_helper_.reset();
 
     network_portal_detector::Shutdown();
     NetworkHandler::Shutdown();
@@ -97,6 +104,8 @@ class UpdateRequiredScreenUnitTest : public testing::Test {
   MockNetworkPortalDetector* mock_network_portal_detector_;
   // Will be deleted in |DBusThreadManager::Shutdown()|.
   FakeUpdateEngineClient* fake_update_engine_client_;
+  // Initializes NetworkStateHandler
+  std::unique_ptr<chromeos::NetworkStateTestHelper> network_state_test_helper_;
 
  private:
   // Test versions of core browser infrastructure.

chrome/browser/resources/chromeos/login/update_required_card.html

@@ -41,8 +41,7 @@
         subtitle-key="errorMessage">
       <div slot="bottom-buttons" class="layout horizontal end-justified">
         <oobe-text-button inverse on-tap="onSelectNetworkClicked_"
-            class="focus-on-show" disabled="[[selectNetworkDisabled]]"
-            text-key="selectNetworkButtonCaption">
+            class="focus-on-show" text-key="selectNetworkButtonCaption">
         </oobe-text-button>
         <oobe-text-button inverse on-tap="onUpdateClicked_"
             id="update-button" text-key="updateButtonCaption">
@@ -78,13 +77,15 @@
       </div>
       <div slot="bottom-buttons" class="layout horizontal end-justified">
         <oobe-text-button inverse on-tap="onCellularPermissionAccepted_"
-            class="focus-on-show" text-key="AcceptUpdateOverCellularButton">
+            class="focus-on-show" text-key="AcceptUpdateOverCellularButton"
+            id="cellular-permission-accept-button">
         </oobe-text-button>
       </div>
     </oobe-dialog>
 
     <oobe-dialog hidden="[[showOn_(ui_state, 'update-no-network')]]"
-        has-buttons title-key="updateRequiredTitle">
+        has-buttons title-key="updateRequiredTitle"
+        id="update-required-no-network-dialog">
       <div slot="subtitle" class="update-subtitle"
           id="no-network-message">
         [[i18nDynamic(locale,'noNetworkMessage', enterpriseDomain)]]