Simplify UpdateScreen exit codes and move them to UpdateScreen

Removes update screen related exit codes from ScreenExitCode enum, and
adds a separate enum to UpdateScreen class.

Error exit codes are condensed into a single UPDATE_ERROR code (as the
action taken by wizard controller is pretty much the same for all), and
the decidion whether non-critical updates should be ignored is moved
from WizardController to UpdateScreen - i.e. for non critical updates,
UpdateScreen will return UPDATE_NOT_REQUIRED result instead of an error.

Changes how UpdateScreen reports exit - instead of calling the
BaseScreenDelegate::OnExit, it runs a callback that is provided in its
constructor.

BUG=930267

Change-Id: I74b9c4c87f213eafbe6adb98f8942d4d8450dc09
Reviewed-on: https://chromium-review.googlesource.com/c/1481852Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635250}

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

@@ -9,12 +9,18 @@ using ::testing::_;
 
 namespace chromeos {
 
-MockUpdateScreen::MockUpdateScreen(BaseScreenDelegate* base_screen_delegate,
-                                   UpdateView* view)
-    : UpdateScreen(base_screen_delegate, view) {}
+MockUpdateScreen::MockUpdateScreen(
+    BaseScreenDelegate* base_screen_delegate,
+    UpdateView* view,
+    const UpdateScreen::ScreenExitCallback& exit_callback)
+    : UpdateScreen(base_screen_delegate, view, exit_callback) {}
 
 MockUpdateScreen::~MockUpdateScreen() {}
 
+void MockUpdateScreen::RunExit(UpdateScreen::Result result) {
+  ExitUpdate(result);
+}
+
 MockUpdateView::MockUpdateView() {
   EXPECT_CALL(*this, MockBind(_)).Times(AtLeast(1));
 }

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

@@ -14,12 +14,16 @@ namespace chromeos {
 
 class MockUpdateScreen : public UpdateScreen {
  public:
-  MockUpdateScreen(BaseScreenDelegate* base_screen_delegate, UpdateView* view);
+  MockUpdateScreen(BaseScreenDelegate* base_screen_delegate,
+                   UpdateView* view,
+                   const ScreenExitCallback& exit_callback);
   virtual ~MockUpdateScreen();
 
   MOCK_METHOD0(Show, void());
   MOCK_METHOD0(Hide, void());
   MOCK_METHOD0(StartNetworkCheck, void());
+
+  void RunExit(UpdateScreen::Result result);
 };
 
 class MockUpdateView : public UpdateView {

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

@@ -16,13 +16,13 @@ std::string ExitCodeToString(ScreenExitCode code) {
       return "HID_DETECTION_COMPLETED";
     case ScreenExitCode::CONNECTION_FAILED:
       return "CONNECTION_FAILED";
-    case ScreenExitCode::UPDATE_INSTALLED:
+    case ScreenExitCode::DEPRECATED_UPDATE_INSTALLED:
       return "UPDATE_INSTALLED";
-    case ScreenExitCode::UPDATE_NOUPDATE:
+    case ScreenExitCode::DEPRECATED_UPDATE_NOUPDATE:
       return "UPDATE_NOUPDATE";
-    case ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE:
+    case ScreenExitCode::DEPRECATED_UPDATE_ERROR_CHECKING_FOR_UPDATE:
       return "UPDATE_ERROR_CHECKING_FOR_UPDATE";
-    case ScreenExitCode::UPDATE_ERROR_UPDATING:
+    case ScreenExitCode::DEPRECATED_UPDATE_ERROR_UPDATING:
       return "UPDATE_ERROR_UPDATING";
     case ScreenExitCode::USER_IMAGE_SELECTED:
       return "USER_IMAGE_SELECTED";
@@ -58,7 +58,7 @@ std::string ExitCodeToString(ScreenExitCode code) {
       return "ARC_TERMS_OF_SERVICE_SKIPPED";
     case ScreenExitCode::ARC_TERMS_OF_SERVICE_ACCEPTED:
       return "ARC_TERMS_OF_SERVICE_ACCEPTED";
-    case ScreenExitCode::UPDATE_ERROR_UPDATING_CRITICAL_UPDATE:
+    case ScreenExitCode::DEPRECATED_UPDATE_ERROR_UPDATING_CRITICAL_UPDATE:
       return "UPDATE_ERROR_UPDATING_CRITICAL_UPDATE";
     case ScreenExitCode::SYNC_CONSENT_FINISHED:
       return "SYNC_CONSENT_FINISHED";

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

@@ -24,13 +24,13 @@ enum class ScreenExitCode {
   HID_DETECTION_COMPLETED = 1,
   // Connection failed while trying to load a WebPageScreen.
   CONNECTION_FAILED = 2,
-  UPDATE_INSTALLED = 3,
+  DEPRECATED_UPDATE_INSTALLED = 3,
   // This exit code means EITHER that there was no update, OR that there
   // was an update, but that it was not a "critical" update. "Critical" updates
   // are those that have a deadline and require the device to reboot.
-  UPDATE_NOUPDATE = 4,
-  UPDATE_ERROR_CHECKING_FOR_UPDATE = 5,
-  UPDATE_ERROR_UPDATING = 6,
+  DEPRECATED_UPDATE_NOUPDATE = 4,
+  DEPRECATED_UPDATE_ERROR_CHECKING_FOR_UPDATE = 5,
+  DEPRECATED_UPDATE_ERROR_UPDATING = 6,
   USER_IMAGE_SELECTED = 7,
   EULA_ACCEPTED = 8,
   EULA_BACK = 9,
@@ -48,7 +48,7 @@ enum class ScreenExitCode {
   ENABLE_DEBUGGING_CANCELED = 22,
   ARC_TERMS_OF_SERVICE_SKIPPED = 23,
   ARC_TERMS_OF_SERVICE_ACCEPTED = 24,
-  UPDATE_ERROR_UPDATING_CRITICAL_UPDATE = 25,
+  DEPRECATED_UPDATE_ERROR_UPDATING_CRITICAL_UPDATE = 25,
   ENCRYPTION_MIGRATION_FINISHED = 26,
   ENCRYPTION_MIGRATION_SKIPPED = 27,
   SYNC_CONSENT_FINISHED = 32,
@@ -68,7 +68,7 @@ enum class ScreenExitCode {
   MARKETING_OPT_IN_FINISHED = 46,
   ASSISTANT_OPTIN_FLOW_FINISHED = 47,
   MULTIDEVICE_SETUP_FINISHED = 48,
-  UPDATE_REJECT_OVER_CELLULAR = 49,
+  DEPRECATED_UPDATE_REJECT_OVER_CELLULAR = 49,
   SUPERVISION_TRANSITION_FINISHED = 50,
   EXIT_CODES_COUNT  // not a real code, must be the last
 };

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

@@ -104,10 +104,12 @@ UpdateScreen* UpdateScreen::Get(ScreenManager* manager) {
 }
 
 UpdateScreen::UpdateScreen(BaseScreenDelegate* base_screen_delegate,
-                           UpdateView* view)
+                           UpdateView* view,
+                           const ScreenExitCallback& exit_callback)
     : BaseScreen(base_screen_delegate, OobeScreen::SCREEN_OOBE_UPDATE),
       reboot_check_delay_(kWaitForRebootTimeSec),
       view_(view),
+      exit_callback_(exit_callback),
       histogram_helper_(new ErrorScreensHistogramHelper("Update")),
       weak_factory_(this) {
   if (view_)
@@ -145,17 +147,20 @@ void UpdateScreen::SetIgnoreIdleStatus(bool ignore_idle_status) {
   ignore_idle_status_ = ignore_idle_status;
 }
 
-void UpdateScreen::ExitUpdate(ScreenExitCode exit_code) {
+void UpdateScreen::ExitUpdate(Result result) {
   DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);
   network_portal_detector::GetInstance()->RemoveObserver(this);
 
-  Finish(exit_code);
+  exit_callback_.Run(result);
 }
 
 void UpdateScreen::UpdateStatusChanged(
     const UpdateEngineClient::Status& status) {
   if (is_checking_for_update_ &&
-      status.status > UpdateEngineClient::UPDATE_STATUS_CHECKING_FOR_UPDATE) {
+      status.status > UpdateEngineClient::UPDATE_STATUS_CHECKING_FOR_UPDATE &&
+      status.status != UpdateEngineClient::UPDATE_STATUS_ERROR &&
+      status.status !=
+          UpdateEngineClient::UPDATE_STATUS_REPORTING_ERROR_EVENT) {
     is_checking_for_update_ = false;
   }
   if (ignore_idle_status_ &&
@@ -175,7 +180,7 @@ void UpdateScreen::UpdateStatusChanged(
           .SetBoolean(kContextKeyShowEstimatedTimeLeft, false);
       if (!HasCriticalUpdate()) {
         VLOG(1) << "Noncritical update available: " << status.new_version;
-        ExitUpdate(ScreenExitCode::UPDATE_NOUPDATE);
+        ExitUpdate(Result::UPDATE_NOT_REQUIRED);
       } else {
         VLOG(1) << "Critical update available: " << status.new_version;
         GetContextEditor()
@@ -198,7 +203,7 @@ void UpdateScreen::UpdateStatusChanged(
         download_average_speed_ = 0.0;
         if (!HasCriticalUpdate()) {
           VLOG(1) << "Non-critical update available: " << status.new_version;
-          ExitUpdate(ScreenExitCode::UPDATE_NOUPDATE);
+          ExitUpdate(Result::UPDATE_NOT_REQUIRED);
         } else {
           VLOG(1) << "Critical update available: " << status.new_version;
           GetContextEditor()
@@ -239,7 +244,7 @@ void UpdateScreen::UpdateStatusChanged(
                             base::TimeDelta::FromSeconds(reboot_check_delay_),
                             this, &UpdateScreen::OnWaitForRebootTimeElapsed);
       } else {
-        ExitUpdate(ScreenExitCode::UPDATE_NOUPDATE);
+        ExitUpdate(Result::UPDATE_NOT_REQUIRED);
       }
       break;
     case UpdateEngineClient::UPDATE_STATUS_NEED_PERMISSION_TO_UPDATE:
@@ -263,21 +268,19 @@ void UpdateScreen::UpdateStatusChanged(
       // Otherwise, it's possible that the update request has not yet been
       // started.
       if (!ignore_idle_status_)
-        ExitUpdate(ScreenExitCode::UPDATE_NOUPDATE);
+        ExitUpdate(Result::UPDATE_NOT_REQUIRED);
       break;
     case UpdateEngineClient::UPDATE_STATUS_ERROR:
     case UpdateEngineClient::UPDATE_STATUS_REPORTING_ERROR_EVENT:
-      if (is_checking_for_update_) {
-        ExitUpdate(ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE);
-      } else if (HasCriticalUpdate()) {
-        ExitUpdate(ScreenExitCode::UPDATE_ERROR_UPDATING_CRITICAL_UPDATE);
+      // Ignore update errors for non-critical updates to prevent blocking the
+      // user from getting to login screen during OOBE if the pending update is
+      // not critical.
+      if (is_checking_for_update_ || !HasCriticalUpdate()) {
+        ExitUpdate(Result::UPDATE_NOT_REQUIRED);
       } else {
-        ExitUpdate(ScreenExitCode::UPDATE_ERROR_UPDATING);
+        ExitUpdate(Result::UPDATE_ERROR);
       }
       break;
-    default:
-      NOTREACHED();
-      break;
   }
 }
 
@@ -335,7 +338,7 @@ void UpdateScreen::OnPortalDetectionCompleted(
 
 void UpdateScreen::CancelUpdate() {
   VLOG(1) << "Forced update cancel";
-  ExitUpdate(ScreenExitCode::UPDATE_NOUPDATE);
+  ExitUpdate(Result::UPDATE_NOT_REQUIRED);
 }
 
 // TODO(jdufault): This should return a pointer. See crbug.com/672142.
@@ -384,7 +387,7 @@ void UpdateScreen::OnUserAction(const std::string& action_id) {
     GetContextEditor()
         .SetBoolean(kContextKeyShowCurtain, true)
         .SetBoolean(kContextKeyRequiresPermissionForCelluar, false);
-    ExitUpdate(ScreenExitCode::UPDATE_REJECT_OVER_CELLULAR);
+    ExitUpdate(Result::UPDATE_ERROR);
   } else {
     BaseScreen::OnUserAction(action_id);
   }
@@ -402,7 +405,7 @@ void UpdateScreen::RetryUpdateWithUpdateOverCellularPermissionSet(
     GetContextEditor()
         .SetBoolean(kContextKeyShowCurtain, true)
         .SetBoolean(kContextKeyRequiresPermissionForCelluar, false);
-    ExitUpdate(ScreenExitCode::UPDATE_REJECT_OVER_CELLULAR);
+    ExitUpdate(Result::UPDATE_ERROR);
   }
 }
 
@@ -574,7 +577,7 @@ void UpdateScreen::OnUpdateCheckStarted(
     UpdateEngineClient::UpdateCheckResult result) {
   VLOG(1) << "Callback from RequestUpdateCheck, result " << result;
   if (result != UpdateEngineClient::UPDATE_RESULT_SUCCESS)
-    ExitUpdate(ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE);
+    ExitUpdate(Result::UPDATE_NOT_REQUIRED);
 }
 
 void UpdateScreen::OnConnectRequested() {

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

@@ -27,13 +27,46 @@ class ScreenManager;
 class UpdateView;
 
 // Controller for the update screen.
+//
+// The screen will request an update availability check from the update engine,
+// and track the update engine progress. When the UpdateScreen finishes, it will
+// run the |exit_callback| with the screen result.
+//
+// If the update engine reports no updates are found, or the available
+// update is not critical, UpdateScreen will report UPDATE_NOT_REQUIRED result.
+//
+// If the update engine reports an error while performing a critical update,
+// UpdateScreen will report UPDATE_ERROR result.
+//
+// If the update engine reports that update is blocked because it would be
+// performed over a metered network, UpdateScreen will request user consent
+// before proceeding with the update. If the user rejects, UpdateScreen will
+// exit and report UPDATE_ERROR result.
+//
+// If update engine finds a critical update, UpdateScreen will wait for the
+// update engine to apply the update, and then request a reboot (if reboot
+// request times out, a message requesting manual reboot will be shown to the
+// user).
+//
+// Before update check request is made, the screen will ensure that the device
+// has network connectivity - if the current network is not online (e.g. behind
+// a protal), it will request an ErrorScreen to be shown. Update check will be
+// delayed until the Internet connectivity is established.
 class UpdateScreen : public BaseScreen,
                      public UpdateEngineClient::Observer,
                      public NetworkPortalDetector::Observer {
  public:
   static UpdateScreen* Get(ScreenManager* manager);
 
-  UpdateScreen(BaseScreenDelegate* base_screen_delegate, UpdateView* view);
+  enum class Result {
+    UPDATE_NOT_REQUIRED,
+    UPDATE_ERROR,
+  };
+
+  using ScreenExitCallback = base::RepeatingCallback<void(Result result)>;
+  UpdateScreen(BaseScreenDelegate* base_screen_delegate,
+               UpdateView* view,
+               const ScreenExitCallback& exit_callback);
   ~UpdateScreen() override;
 
   // Called when the being destroyed. This should call Unbind() on the
@@ -45,9 +78,6 @@ class UpdateScreen : public BaseScreen,
 
   void SetIgnoreIdleStatus(bool ignore_idle_status);
 
-  // Reports update results to the BaseScreenDelegate.
-  void ExitUpdate(ScreenExitCode exit_code);
-
   // UpdateEngineClient::Observer implementation:
   void UpdateStatusChanged(const UpdateEngineClient::Status& status) override;
 
@@ -61,6 +91,14 @@ class UpdateScreen : public BaseScreen,
 
   base::OneShotTimer& GetErrorMessageTimerForTesting();
 
+  void set_exit_callback_for_testing(const ScreenExitCallback& callback) {
+    exit_callback_ = callback;
+  }
+
+ protected:
+  // Reports update results.
+  void ExitUpdate(Result result);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(UpdateScreenTest, TestBasic);
   FRIEND_TEST_ALL_PREFIXES(UpdateScreenTest, TestUpdateAvailable);
@@ -144,6 +182,7 @@ class UpdateScreen : public BaseScreen,
   bool ignore_idle_status_ = true;
 
   UpdateView* view_ = nullptr;
+  ScreenExitCallback exit_callback_;
 
   // Time of the first notification from the downloading stage.
   base::Time download_start_time_;

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

@@ -90,6 +90,8 @@ class UpdateScreenTest : public InProcessBrowserTest {
     ASSERT_EQ(WizardController::default_controller()->current_screen(),
               update_screen_);
     update_screen_->base_screen_delegate_ = mock_base_screen_delegate_.get();
+    update_screen_->set_exit_callback_for_testing(base::BindRepeating(
+        &UpdateScreenTest::HandleScreenExit, base::Unretained(this)));
   }
 
   void TearDownOnMainThread() override {
@@ -134,7 +136,14 @@ class UpdateScreenTest : public InProcessBrowserTest {
   NetworkPortalDetectorTestImpl* network_portal_detector_ =
       nullptr;  // Unowned.
 
+  base::Optional<UpdateScreen::Result> last_screen_result_;
+
  private:
+  void HandleScreenExit(UpdateScreen::Result result) {
+    EXPECT_FALSE(last_screen_result_.has_value());
+    last_screen_result_ = result;
+  }
+
   DISALLOW_COPY_AND_ASSIGN(UpdateScreenTest);
 };
 
@@ -154,10 +163,11 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestNoUpdate) {
   // UpdateStatusChanged().
   fake_update_engine_client_->set_default_status(status);
 
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
   update_screen_->UpdateStatusChanged(status);
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestUpdateAvailable) {
@@ -198,17 +208,16 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestUpdateAvailable) {
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestErrorIssuingUpdateCheck) {
   // First, cancel the update that is already in progress.
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
   update_screen_->CancelUpdate();
+  last_screen_result_.reset();
 
   fake_update_engine_client_->set_update_check_result(
       chromeos::UpdateEngineClient::UPDATE_RESULT_FAILED);
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE))
-      .Times(1);
   update_screen_->StartNetworkCheck();
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestErrorCheckingForUpdate) {
@@ -218,38 +227,38 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestErrorCheckingForUpdate) {
   // UpdateStatusChanged().
   fake_update_engine_client_->set_default_status(status);
 
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE))
-      .Times(1);
   update_screen_->UpdateStatusChanged(status);
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestErrorUpdating) {
   UpdateEngineClient::Status status;
   status.status = UpdateEngineClient::UPDATE_STATUS_UPDATE_AVAILABLE;
   status.new_version = "latest and greatest";
-  // GetLastStatus() will be called via ExitUpdate() called from
-  // UpdateStatusChanged().
   fake_update_engine_client_->set_default_status(status);
 
   update_screen_->UpdateStatusChanged(status);
 
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
+  last_screen_result_.reset();
+
   status.status = UpdateEngineClient::UPDATE_STATUS_ERROR;
-  // GetLastStatus() will be called via ExitUpdate() called from
-  // UpdateStatusChanged().
   fake_update_engine_client_->set_default_status(status);
-
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_ERROR_UPDATING))
-      .Times(1);
   update_screen_->UpdateStatusChanged(status);
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestTemproraryOfflineNetwork) {
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
   update_screen_->CancelUpdate();
+  last_screen_result_.reset();
 
   // Change ethernet state to portal.
   NetworkPortalDetector::CaptivePortalState portal_state;
@@ -286,18 +295,16 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestTemproraryOfflineNetwork) {
   fake_update_engine_client_->set_update_check_result(
       chromeos::UpdateEngineClient::UPDATE_RESULT_FAILED);
 
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE))
-      .Times(1);
-
   NotifyPortalDetectionCompleted();
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestTwoOfflineNetworks) {
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
   update_screen_->CancelUpdate();
+  last_screen_result_.reset();
 
   // Change ethernet state to portal.
   NetworkPortalDetector::CaptivePortalState portal_state;
@@ -337,16 +344,16 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestTwoOfflineNetworks) {
       .Times(1);
 
   NotifyPortalDetectionCompleted();
+
+  EXPECT_FALSE(last_screen_result_.has_value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestVoidNetwork) {
   SetDefaultNetwork(std::string());
 
   // Cancels pending update request.
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
   update_screen_->CancelUpdate();
+  last_screen_result_.reset();
 
   // First portal detection attempt returns NULL network and undefined
   // results, so detection is restarted.
@@ -367,13 +374,12 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestVoidNetwork) {
   EXPECT_CALL(*mock_base_screen_delegate_, ShowErrorScreen()).Times(1);
   base::RunLoop().RunUntilIdle();
   NotifyPortalDetectionCompleted();
+  EXPECT_FALSE(last_screen_result_.has_value());
 }
 
 IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestAPReselection) {
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
   update_screen_->CancelUpdate();
+  last_screen_result_.reset();
 
   // Change ethernet state to portal.
   NetworkPortalDetector::CaptivePortalState portal_state;
@@ -405,12 +411,13 @@ IN_PROC_BROWSER_TEST_F(UpdateScreenTest, TestAPReselection) {
       .Times(1);
   fake_update_engine_client_->set_update_check_result(
       chromeos::UpdateEngineClient::UPDATE_RESULT_FAILED);
-  EXPECT_CALL(*mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE))
-      .Times(1);
 
   update_screen_->OnConnectRequested();
   base::RunLoop().RunUntilIdle();
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 }  // namespace chromeos

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

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/login/screens/update_screen.h"
 
 #include "base/command_line.h"
+#include "base/optional.h"
 #include "base/test/scoped_mock_time_message_loop_task_runner.h"
 #include "chrome/browser/chromeos/login/screens/mock_base_screen_delegate.h"
 #include "chrome/browser/chromeos/login/screens/mock_error_screen.h"
@@ -42,20 +43,23 @@ class UpdateScreenUnitTest : public testing::Test {
       const std::unique_ptr<UpdateScreen>& update_screen,
       bool available,
       bool critical) {
-    update_engine_status_.status =
+    UpdateEngineClient::Status update_engine_status;
+    update_engine_status.status =
         UpdateEngineClient::UPDATE_STATUS_CHECKING_FOR_UPDATE;
     fake_update_engine_client_->NotifyObserversThatStatusChanged(
-        update_engine_status_);
+        update_engine_status);
+
     if (critical) {
       ASSERT_TRUE(available) << "Does not make sense for an update to be "
                                 "critical if one is not even available.";
       update_screen->is_ignore_update_deadlines_ = true;
     }
-    update_engine_status_.status =
+    update_engine_status.status =
         available ? UpdateEngineClient::UPDATE_STATUS_UPDATE_AVAILABLE
                   : UpdateEngineClient::UPDATE_STATUS_IDLE;
+
     fake_update_engine_client_->NotifyObserversThatStatusChanged(
-        update_engine_status_);
+        update_engine_status);
   }
 
   // testing::Test:
@@ -82,6 +86,11 @@ class UpdateScreenUnitTest : public testing::Test {
     EXPECT_CALL(mock_base_screen_delegate_, GetErrorScreen())
         .Times(AnyNumber())
         .WillRepeatedly(Return(mock_error_screen_.get()));
+
+    update_screen_ = std::make_unique<UpdateScreen>(
+        &mock_base_screen_delegate_, &mock_view_,
+        base::BindRepeating(&UpdateScreenUnitTest::HandleScreenExit,
+                            base::Unretained(this)));
   }
 
   void TearDown() override {
@@ -101,12 +110,18 @@ class UpdateScreenUnitTest : public testing::Test {
   MockBaseScreenDelegate mock_base_screen_delegate_;
   MockUpdateView mock_view_;
   MockNetworkErrorView mock_error_view_;
-  UpdateEngineClient::Status update_engine_status_;
   std::unique_ptr<MockErrorScreen> mock_error_screen_;
   MockNetworkPortalDetector* mock_network_portal_detector_;
   FakeUpdateEngineClient* fake_update_engine_client_;
 
+  base::Optional<UpdateScreen::Result> last_screen_result_;
+
  private:
+  void HandleScreenExit(UpdateScreen::Result result) {
+    EXPECT_FALSE(last_screen_result_.has_value());
+    last_screen_result_ = result;
+  }
+
   // Test versions of core browser infrastructure.
   content::TestBrowserThreadBundle threads_;
   ScopedTestingLocalState local_state_;
@@ -115,15 +130,7 @@ class UpdateScreenUnitTest : public testing::Test {
 };
 
 TEST_F(UpdateScreenUnitTest, HandlesNoUpdate) {
-  // Set expectation that UpdateScreen will exit successfully
-  // with code UPDATE_NOUPDATE.
-  EXPECT_CALL(mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
-
   // DUT reaches UpdateScreen.
-  update_screen_.reset(
-      new UpdateScreen(&mock_base_screen_delegate_, &mock_view_));
   update_screen_->StartNetworkCheck();
 
   // Verify that the DUT checks for an update.
@@ -132,20 +139,14 @@ TEST_F(UpdateScreenUnitTest, HandlesNoUpdate) {
   // No updates are available.
   SimulateUpdateAvailable(update_screen_, false /* available */,
                           false /* critical */);
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 TEST_F(UpdateScreenUnitTest, HandlesNonCriticalUpdate) {
-  // Set expectation that UpdateScreen will exit successfully
-  // with code UPDATE_NOUPDATE. No, this is not a typo.
-  // UPDATE_NOUPDATE means that either there was no update
-  // or there was a non-critical update.
-  EXPECT_CALL(mock_base_screen_delegate_,
-              OnExit(ScreenExitCode::UPDATE_NOUPDATE))
-      .Times(1);
-
   // DUT reaches UpdateScreen.
-  update_screen_.reset(
-      new UpdateScreen(&mock_base_screen_delegate_, &mock_view_));
   update_screen_->StartNetworkCheck();
 
   // Verify that the DUT checks for an update.
@@ -154,16 +155,28 @@ TEST_F(UpdateScreenUnitTest, HandlesNonCriticalUpdate) {
   // A non-critical update is available.
   SimulateUpdateAvailable(update_screen_, true /* available */,
                           false /* critical */);
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_NOT_REQUIRED,
+            last_screen_result_.value());
 }
 
 TEST_F(UpdateScreenUnitTest, HandlesCriticalUpdate) {
-  // Set expectation that UpdateScreen does not exit.
-  // This is the case because a critical update mandates reboot.
-  EXPECT_CALL(mock_base_screen_delegate_, OnExit(_)).Times(0);
+  // DUT reaches UpdateScreen.
+  update_screen_->StartNetworkCheck();
 
+  // Verify that the DUT checks for an update.
+  EXPECT_EQ(fake_update_engine_client_->request_update_check_call_count(), 1);
+
+  // An update is available, and it's critical!
+  SimulateUpdateAvailable(update_screen_, true /* available */,
+                          true /* critical */);
+
+  EXPECT_FALSE(last_screen_result_.has_value());
+}
+
+TEST_F(UpdateScreenUnitTest, HandleCriticalUpdateError) {
   // DUT reaches UpdateScreen.
-  update_screen_.reset(
-      new UpdateScreen(&mock_base_screen_delegate_, &mock_view_));
   update_screen_->StartNetworkCheck();
 
   // Verify that the DUT checks for an update.
@@ -172,6 +185,17 @@ TEST_F(UpdateScreenUnitTest, HandlesCriticalUpdate) {
   // An update is available, and it's critical!
   SimulateUpdateAvailable(update_screen_, true /* available */,
                           true /* critical */);
+
+  EXPECT_FALSE(last_screen_result_.has_value());
+
+  UpdateEngineClient::Status update_engine_status;
+  update_engine_status.status =
+      UpdateEngineClient::UPDATE_STATUS_REPORTING_ERROR_EVENT;
+  fake_update_engine_client_->NotifyObserversThatStatusChanged(
+      update_engine_status);
+
+  ASSERT_TRUE(last_screen_result_.has_value());
+  EXPECT_EQ(UpdateScreen::Result::UPDATE_ERROR, last_screen_result_.value());
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/wizard_controller.cc

@@ -373,7 +373,10 @@ std::unique_ptr<BaseScreen> WizardController::CreateScreen(OobeScreen screen) {
     return std::make_unique<NetworkScreen>(this,
                                            oobe_ui->GetNetworkScreenView());
   } else if (screen == OobeScreen::SCREEN_OOBE_UPDATE) {
-    return std::make_unique<UpdateScreen>(this, oobe_ui->GetUpdateView());
+    return std::make_unique<UpdateScreen>(
+        this, oobe_ui->GetUpdateView(),
+        base::BindRepeating(&WizardController::OnUpdateScreenExit,
+                            weak_factory_.GetWeakPtr()));
   } else if (screen == OobeScreen::SCREEN_USER_IMAGE_PICKER) {
     return std::make_unique<UserImageScreen>(this, oobe_ui->GetUserImageView());
   } else if (screen == OobeScreen::SCREEN_OOBE_EULA) {
@@ -707,8 +710,41 @@ void WizardController::SkipUpdateEnrollAfterEula() {
   skip_update_enroll_after_eula_ = true;
 }
 
+void WizardController::OnScreenExit(OobeScreen screen) {
+  DCHECK_EQ(screen, current_screen_->screen_id());
+
+  if (IsOOBEStepToTrack(screen)) {
+    RecordUMAHistogramForOOBEStepCompletionTime(
+        screen, base::Time::Now() - screen_show_times_[screen]);
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // WizardController, ExitHandlers:
+void WizardController::OnUpdateScreenExit(UpdateScreen::Result result) {
+  VLOG(1) << "Update screen exit: " << static_cast<int>(result);
+  OnScreenExit(OobeScreen::SCREEN_OOBE_UPDATE);
+
+  switch (result) {
+    case UpdateScreen::Result::UPDATE_NOT_REQUIRED:
+      OnUpdateCompleted();
+      break;
+    case UpdateScreen::Result::UPDATE_ERROR:
+      // Ignore update errors if the OOBE flow has already completed - this
+      // prevents the user getting blocked from getting to the login screen.
+      if (is_out_of_box_) {
+        ShowNetworkScreen();
+      } else {
+        OnUpdateCompleted();
+      }
+      break;
+  }
+}
+
+void WizardController::OnUpdateCompleted() {
+  ShowAutoEnrollmentCheckScreen();
+}
+
 void WizardController::OnHIDDetectionCompleted() {
   // Check for tests configuration.
   if (!StartupUtils::IsOobeCompleted())
@@ -776,14 +812,6 @@ void WizardController::OnConnectionFailed() {
   ShowLoginScreen(LoginScreenContext());
 }
 
-void WizardController::OnUpdateCompleted() {
-  ShowAutoEnrollmentCheckScreen();
-}
-
-void WizardController::OnUpdateOverCellularRejected() {
-  ShowNetworkScreen();
-}
-
 void WizardController::OnEulaAccepted() {
   time_eula_accepted_ = base::Time::Now();
   StartupUtils::MarkEulaAccepted();
@@ -823,26 +851,6 @@ void WizardController::OnChangedMetricsReportingState(bool enabled) {
 #endif
 }
 
-void WizardController::OnUpdateErrorCheckingForUpdate() {
-  // TODO(nkostylev): Update should be required during OOBE.
-  // We do not want to block users from being able to proceed to the login
-  // screen if there is any error checking for an update.
-  // They could use "browse without sign-in" feature to set up the network to be
-  // able to perform the update later.
-  OnUpdateCompleted();
-}
-
-void WizardController::OnUpdateErrorUpdating(bool is_critical_update) {
-  // If there was an error while getting or applying the update, return to
-  // network selection screen if the OOBE isn't complete and the update is
-  // deemed critical. Otherwise, similar to OnUpdateErrorCheckingForUpdate(), we
-  // do not want to block users from being able to proceed to the login screen.
-  if (is_out_of_box_ && is_critical_update)
-    ShowNetworkScreen();
-  else
-    OnUpdateCompleted();
-}
-
 void WizardController::OnUserImageSelected() {
   OnOobeFlowFinished();
 }
@@ -1204,7 +1212,7 @@ void WizardController::SetCurrentScreenSmooth(BaseScreen* new_current,
 
   const OobeScreen screen = new_current->screen_id();
   if (IsOOBEStepToTrack(screen))
-    screen_show_times_[GetOobeScreenName(screen)] = base::Time::Now();
+    screen_show_times_[screen] = base::Time::Now();
 
   previous_screen_ = current_screen_;
   current_screen_ = new_current;
@@ -1371,13 +1379,8 @@ void WizardController::SimulateDemoModeSetupForTesting(
 // WizardController, BaseScreenDelegate overrides:
 void WizardController::OnExit(ScreenExitCode exit_code) {
   VLOG(1) << "Wizard screen exit code: " << ExitCodeToString(exit_code);
-  const OobeScreen previous_screen = current_screen_->screen_id();
-  if (IsOOBEStepToTrack(previous_screen)) {
-    RecordUMAHistogramForOOBEStepCompletionTime(
-        previous_screen,
-        base::Time::Now() -
-            screen_show_times_[GetOobeScreenName(previous_screen)]);
-  }
+  OnScreenExit(current_screen_->screen_id());
+
   switch (exit_code) {
     case ScreenExitCode::HID_DETECTION_COMPLETED:
       OnHIDDetectionCompleted();
@@ -1397,22 +1400,6 @@ void WizardController::OnExit(ScreenExitCode exit_code) {
     case ScreenExitCode::CONNECTION_FAILED:
       OnConnectionFailed();
       break;
-    case ScreenExitCode::UPDATE_INSTALLED:
-    case ScreenExitCode::UPDATE_NOUPDATE:
-      OnUpdateCompleted();
-      break;
-    case ScreenExitCode::UPDATE_REJECT_OVER_CELLULAR:
-      OnUpdateOverCellularRejected();
-      return;
-    case ScreenExitCode::UPDATE_ERROR_CHECKING_FOR_UPDATE:
-      OnUpdateErrorCheckingForUpdate();
-      break;
-    case ScreenExitCode::UPDATE_ERROR_UPDATING:
-      OnUpdateErrorUpdating(false /* is_critical_update */);
-      break;
-    case ScreenExitCode::UPDATE_ERROR_UPDATING_CRITICAL_UPDATE:
-      OnUpdateErrorUpdating(true /* is_critical_update */);
-      break;
     case ScreenExitCode::USER_IMAGE_SELECTED:
       OnUserImageSelected();
       break;

chrome/browser/chromeos/login/wizard_controller.h

@@ -25,6 +25,7 @@
 #include "chrome/browser/chromeos/login/screens/eula_screen.h"
 #include "chrome/browser/chromeos/login/screens/hid_detection_screen.h"
 #include "chrome/browser/chromeos/login/screens/reset_screen.h"
+#include "chrome/browser/chromeos/login/screens/update_screen.h"
 #include "chrome/browser/chromeos/login/screens/welcome_screen.h"
 #include "chrome/browser/chromeos/policy/enrollment_config.h"
 
@@ -190,19 +191,20 @@ class WizardController : public BaseScreenDelegate,
   // Shows previous screen. Should only be called if previous screen exists.
   void ShowPreviousScreen();
 
+  // Shared actions to be performed on a screen exit.
+  void OnScreenExit(OobeScreen screen);
+
   // Exit handlers:
+  void OnUpdateScreenExit(UpdateScreen::Result result);
+  void OnUpdateCompleted();
   void OnHIDDetectionCompleted();
   void OnWelcomeContinued();
   void OnNetworkBack();
   void OnNetworkConnected();
   void OnOfflineDemoModeSetup();
   void OnConnectionFailed();
-  void OnUpdateCompleted();
-  void OnUpdateOverCellularRejected();
   void OnEulaAccepted();
   void OnEulaBack();
-  void OnUpdateErrorCheckingForUpdate();
-  void OnUpdateErrorUpdating(bool is_critical_update);
   void OnUserImageSelected();
   void OnEnrollmentDone();
   void OnDeviceModificationCanceled();
@@ -429,7 +431,7 @@ class WizardController : public BaseScreenDelegate,
   std::unique_ptr<DemoSetupController> demo_setup_controller_;
 
   // Maps screen names to last time of their shows.
-  std::map<std::string, base::Time> screen_show_times_;
+  std::map<OobeScreen, base::Time> screen_show_times_;
 
   // Tests check result of timezone resolve.
   bool timezone_resolved_ = false;