Improve the Supervision Onboarding webview host interface.

With this change we add basic error reporting (to be improved with UMA
later) as well as more UI configuration options.

This essentially decouples the webview loading from the presentation of
the flow, so we don't flicker when we make async decisions. Example:
1: WebviewHost finishes load and reports header value.
2: WebviewHost shows empty webview contents.
3: Controller processes the value and decides to exit the flow.
4: The screen flickers since steps 2 and 3 happen too fast and are
   racing.

Introducing a presentation struct solves this issue and also helps to
implement another requirement, optional buttons for flow pages.

This leaves us with the Controller holding all the important state for
the flow, which allows us to write pretty detailed unit tests that cover
most issues. This will remove some of the burden from the browser_tests,
which are already too complex.

Bug: 958995
Change-Id: I5929688ee6a753f817b023937f5d56be597b9fb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628035Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Lucas Tenório <ltenorio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665052}

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

@@ -331,15 +331,6 @@ IN_PROC_BROWSER_TEST_F(SupervisionOnboardingTest, NextButtonExitsScreen) {
   WaitForScreenExit();
 }
 
-IN_PROC_BROWSER_TEST_F(SupervisionOnboardingTest, BackButtonExitsScreen) {
-  ShowScreen();
-  WaitForScreen();
-  EXPECT_EQ(1u, supervision_server()->GetReceivedRequestsCount());
-
-  ClickButton("supervision-onboarding-back-button");
-  WaitForScreenExit();
-}
-
 IN_PROC_BROWSER_TEST_F(SupervisionOnboardingTest, SkipButtonExitsScreen) {
   ShowScreen();
   WaitForScreen();

chrome/browser/chromeos/supervision/mojom/onboarding_controller.mojom

@@ -40,13 +40,39 @@ struct OnboardingPage {
   string? custom_header_name;
 };
 
+enum OnboardingPresentationState {
+  // We're loading data and preparing the flow. The webview should visible and
+  // the user should see a loading indicator to indicate this inactive state.
+  kLoading,
+
+  // The flow is ready, the webview can be shown.
+  kReady,
+
+  // TODO(crbug.com/958995): Add an error state, where we can show retry
+  // actions and stop showing the webview. At the moment we simply end the flow
+  // when we find errors.
+};
+
+// Describes how the flow should be presented at a given point.
+struct OnboardingPresentation {
+  OnboardingPresentationState state;
+
+  // Properties that inform which actions can be presented to the user.
+  bool can_show_next_page;
+  bool can_show_previous_page;
+  bool can_skip_flow;
+};
+
 // Represents a webview host, responsible for displaying supervision
 // onboarding pages. This will usually be a WebUI page that contains a
 // webview tag and manages its properties.
-// TODO(958995): Complete this interface.
 interface OnboardingWebviewHost {
+  // Sets the presentation for the flow. Guaranteed to be called at least once
+  // after this webview host is bound.
+  SetPresentation(OnboardingPresentation presentation);
+
   // Requests the host to load the given page.
-  LoadPage(OnboardingPage page) => (string? custom_header_value);
+  LoadPage(OnboardingPage page) => (OnboardingLoadPageResult result);
 
   // Requests that the host exit the flow immediately. This might mean
   // different things depending on the type of host. If we are running in OOBE
@@ -55,8 +81,20 @@ interface OnboardingWebviewHost {
   ExitFlow();
 };
 
+// Result of a LoadPage call. Contains data about errors and custom data
+// found during the load.
+struct OnboardingLoadPageResult {
+  // Error code for this operation. If net::OK, the load was successful.
+  // See net/base/net_errors.h for possible list of values.
+  int32 net_error;
+
+  // Value for the custom HTTP header that was extracted while the page was
+  // loading. Will be missing if we couldn't find the requested header or if
+  // the page didn't request one.
+  string? custom_header_value;
+};
+
 // Interface responsible for managing the whole onboarding flow.
-// TODO(958995): Complete this interface.
 interface OnboardingController {
   // Binds the given webview host to this controller. The host will start
   // receiving commands as soon as this is called.

chrome/browser/chromeos/supervision/onboarding_controller_impl.cc

@@ -54,6 +54,10 @@ void OnboardingControllerImpl::BindWebviewHost(
     mojom::OnboardingWebviewHostPtr webview_host) {
   webview_host_ = std::move(webview_host);
 
+  auto presentation = mojom::OnboardingPresentation::New();
+  presentation->state = mojom::OnboardingPresentationState::kLoading;
+  webview_host_->SetPresentation(std::move(presentation));
+
   identity::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile_);
 
@@ -104,20 +108,41 @@ void OnboardingControllerImpl::AccessTokenCallback(
 }
 
 void OnboardingControllerImpl::LoadPageCallback(
-    const base::Optional<std::string>& custom_header_value) {
+    mojom::OnboardingLoadPageResultPtr result) {
   DCHECK(webview_host_);
 
-  bool eligible =
-      custom_header_value.has_value() &&
-      base::EqualsCaseInsensitiveASCII(custom_header_value.value(),
-                                       kDeviceOnboardingExperimentName);
-  if (!eligible ||
-      !base::FeatureList::IsEnabled(features::kSupervisionOnboardingScreens)) {
+  // TODO(crbug.com/958995): Log the load page callback results to UMA. We want
+  // to see how many users get errors, have missing header values or actually
+  // end up seeing the page.
+
+  if (result->net_error != net::Error::OK) {
+    // TODO(crbug.com/958995): Fail here more gracefully. We should provide a
+    // way to retry the fetch if the error is recoverable.
+    LOG(ERROR) << "Supervision Onboarding webview failed to load with error: "
+               << net::ErrorToString(result->net_error);
+    webview_host_->ExitFlow();
+    return;
+  }
+
+  if (!result->custom_header_value.has_value() ||
+      !base::EqualsCaseInsensitiveASCII(result->custom_header_value.value(),
+                                        kDeviceOnboardingExperimentName)) {
     webview_host_->ExitFlow();
+    return;
+  }
+
+  profile_->GetPrefs()->SetBoolean(ash::prefs::kKioskNextShellEligible, true);
+
+  if (!base::FeatureList::IsEnabled(features::kSupervisionOnboardingScreens)) {
+    webview_host_->ExitFlow();
+    return;
   }
 
-  profile_->GetPrefs()->SetBoolean(ash::prefs::kKioskNextShellEligible,
-                                   eligible);
+  auto presentation = mojom::OnboardingPresentation::New();
+  presentation->state = mojom::OnboardingPresentationState::kReady;
+  presentation->can_show_next_page = true;
+  presentation->can_skip_flow = true;
+  webview_host_->SetPresentation(std::move(presentation));
 }
 
 }  // namespace supervision

chrome/browser/chromeos/supervision/onboarding_controller_impl.h

@@ -41,7 +41,7 @@ class OnboardingControllerImpl : public mojom::OnboardingController {
                            identity::AccessTokenInfo access_token_info);
 
   // Callback to OnboardingWebviewHost::LoadPage.
-  void LoadPageCallback(const base::Optional<std::string>& custom_header_value);
+  void LoadPageCallback(mojom::OnboardingLoadPageResultPtr result);
 
   Profile* profile_;
   mojom::OnboardingWebviewHostPtr webview_host_;

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

@@ -20,30 +20,30 @@
     <link rel="stylesheet" href="supervision_onboarding.css">
     <div id="supervision-onboarding-flow-container">
       <oobe-dialog id="loading-dialog" role="dialog" no-header no-footer-padding
-          hidden="[[pageLoaded_]]">
+          hidden="[[isReady_]]">
         <div slot="footer" class="flex layout vertical center center-justified">
           <throbber-notice text="$i18n{supervisionOnboardingWaitMessage}">
           </throbber-notice>
         </div>
       </oobe-dialog>
       <oobe-dialog id="supervision-onboarding-content" role="dialog" no-header
-          no-footer-padding has-buttons hidden="[[!pageLoaded_]]">
+          no-footer-padding has-buttons hidden="[[!isReady_]]">
         <webview id="supervisionOnboardingWebview" slot="footer">
         </webview>
         <div id="supervision-onboarding-flow-buttons" slot="bottom-buttons"
             class="layout horizontal end-justified">
           <oobe-back-button id="supervision-onboarding-back-button"
-              on-tap="onBack_">
+              hidden="[[hideBackButton_]]" on-tap="onBack_">
             <div i18n-content="supervisionOnboardingBackButtonLabel"></div>
           </oobe-back-button>
           <div class="flex">
           </div>
           <oobe-text-button id="supervision-onboarding-skip-button" border
-              on-tap="onSkip_">
+              hidden="[[hideSkipButton_]]" on-tap="onSkip_">
             <div i18n-content="supervisionOnboardingSkipButtonLabel"></div>
           </oobe-text-button>
           <oobe-text-button id="supervision-onboarding-next-button" inverse
-              on-tap="onNext_">
+              hidden="[[hideNextButton_]]" on-tap="onNext_">
             <div i18n-content="supervisionOnboardingNextButtonLabel"></div>
           </oobe-text-button>
         </div>

chrome/browser/resources/chromeos/login/supervision_onboarding.js

@@ -20,21 +20,18 @@
        */
       this.page_ = null;
 
-      /**
-       * Custom header values found in responses to requests made by the
-       * webview.
-       * @private {?string}
-       */
-      this.customHeaderValue_ = null;
-
       /**
        * Pending callback for a webview page load. It will be called with the
        * list of custom header values if asked by the controller, or an empty
        * array otherwise.
-       * @private {?function({customHeaderValue: ?string})}
+       * @private{?function({result:
+       *     !chromeos.supervision.mojom.OnboardingLoadPageResult})}
        */
       this.pendingLoadPageCallback_ = null;
 
+      /** @private {?chromeos.supervision.mojom.OnboardingLoadPageResult} */
+      this.pendingLoadPageResult_ = null;
+
       this.webviewListener_ = this.webviewFinishedLoading_.bind(this);
       this.webviewHeaderListener_ = this.onHeadersReceived_.bind(this);
       this.webviewAuthListener_ = this.authorizeRequest_.bind(this);
@@ -42,19 +39,21 @@
 
     /**
      * @param {!chromeos.supervision.mojom.OnboardingPage} page
-     * @return {!Promise<{customHeaderValue: ?string}>}
+     * @return {Promise<{
+     *     result: !chromeos.supervision.mojom.OnboardingLoadPageResult,
+     *  }>}
      */
     loadPage(page) {
       // TODO(958995): Handle the case where we are still loading the previous
       // page but the controller wants to load the next one. For now we just
       // resolve the previous callback.
       if (this.pendingLoadPageCallback_) {
-        this.pendingLoadPageCallback_({customHeaderValue: null});
+        this.pendingLoadPageCallback_({result: {netError: 0}});
       }
 
       this.page_ = page;
-      this.customHeaderValue_ = null;
       this.pendingLoadPageCallback_ = null;
+      this.pendingLoadPageResult_ = {netError: 0};
 
       this.webview_.request.onBeforeSendHeaders.addListener(
           this.webviewAuthListener_,
@@ -65,9 +64,6 @@
           {urls: [page.urlFilterPattern], types: ['main_frame']},
           ['responseHeaders', 'extraHeaders']);
 
-      // TODO(958995): Report load errors through the mojo interface.
-      // At the moment we are treating any loadstop/abort event as a successful
-      // load.
       this.webview_.addEventListener('loadstop', this.webviewListener_);
       this.webview_.addEventListener('loadabort', this.webviewListener_);
       this.webview_.src = page.url.url;
@@ -91,7 +87,9 @@
       const header = responseEvent.responseHeaders.find(
           h => h.name.toUpperCase() ==
               this.page_.customHeaderName.toUpperCase());
-      this.customHeaderValue_ = header ? header.value : null;
+      if (header) {
+        this.pendingLoadPageResult_.customHeaderValue = header.value;
+      }
 
       return {};
     }
@@ -124,9 +122,11 @@
       this.webview_.removeEventListener('loadstop', this.webviewListener_);
       this.webview_.removeEventListener('loadabort', this.webviewListener_);
 
-      this.pendingLoadPageCallback_({
-        customHeaderValue: this.customHeaderValue_,
-      });
+      if (e.type == 'loadabort') {
+        this.pendingLoadPageResult_.netError = e.code;
+      }
+
+      this.pendingLoadPageCallback_({result: this.pendingLoadPageResult_});
     }
   }
 
@@ -137,7 +137,11 @@
 
     properties: {
       /** True if the webview loaded the page. */
-      pageLoaded_: {type: Boolean, value: false},
+      isReady_: {type: Boolean, value: false},
+
+      hideBackButton_: {type: Boolean, value: true},
+      hideSkipButton_: {type: Boolean, value: true},
+      hideNextButton_: {type: Boolean, value: true},
     },
 
     /** Overridden from LoginScreenBehavior. */
@@ -167,13 +171,10 @@
       this.hostCallbackRouter_ =
           new chromeos.supervision.mojom.OnboardingWebviewHostCallbackRouter();
 
-      this.hostCallbackRouter_.loadPage.addListener(p => {
-        this.pageLoaded_ = false;
-        return this.webviewLoader_.loadPage(p).then(response => {
-          this.pageLoaded_ = true;
-          return response;
-        });
-      });
+      this.hostCallbackRouter_.setPresentation.addListener(
+          this.setPresentation_.bind(this));
+      this.hostCallbackRouter_.loadPage.addListener(
+          this.webviewLoader_.loadPage.bind(this.webviewLoader_));
       this.hostCallbackRouter_.exitFlow.addListener(this.exitFlow_.bind(this));
 
       this.controller_.bindWebviewHost(this.hostCallbackRouter_.createProxy());
@@ -187,6 +188,19 @@
       });
     },
 
+    /**
+     * @param {!chromeos.supervision.mojom.OnboardingPresentation} presentation
+     * @private
+     */
+    setPresentation_: function(presentation) {
+      this.isReady_ = presentation.state ==
+          chromeos.supervision.mojom.OnboardingPresentationState.kReady;
+
+      this.hideBackButton_ = !presentation.canShowPreviousPage;
+      this.hideSkipButton_ = !presentation.canSkipFlow;
+      this.hideNextButton_ = !presentation.canShowNextPage;
+    },
+
     /** @private */
     onBack_: function() {
       this.controller_.handleAction(