Split getChannelInfo() into two calls

Previously, get ChannelInfo() was obtaining the version of the current
channel and was checking from policy/profile whether the user can modify
the channel. Sometimes, when the update daemon was slow, the ui will not
receive the data if the user can modify the channel, leading to it being
shown by default and then, disabled.

Old CL: https://crrev.com/c/1831769

Bug: 848750
Change-Id: Ia0aa6d3353e2e6c310642865f4dc437c9d449c22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106852
Auto-Submit: Miriam Polzer <mpolzer@google.com>
Commit-Queue: Miriam Polzer <mpolzer@google.com>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753147}

chrome/browser/resources/settings/about_page/about_page_browser_proxy.js

@@ -25,7 +25,6 @@ let RegulatoryInfo;
  * @typedef {{
  *   currentChannel: BrowserChannel,
  *   targetChannel: BrowserChannel,
- *   canChangeChannel: boolean,
  * }}
  */
 let ChannelInfo;
@@ -214,9 +213,16 @@ cr.define('settings', function() {
      */
     setChannel(channel, isPowerwashAllowed) {}
 
-    /** @return {!Promise<!ChannelInfo>} */
+    /**
+     * Requests channel info from the version updater. This may have latency if
+     * the version updater is busy, for example with downloading updates.
+     * @return {!Promise<!ChannelInfo>}
+     */
     getChannelInfo() {}
 
+    /** @return {!Promise<!boolean>} */
+    canChangeChannel() {}
+
     /** @return {!Promise<!VersionInfo>} */
     getVersionInfo() {}
 
@@ -325,6 +331,11 @@ cr.define('settings', function() {
       return cr.sendWithPromise('getChannelInfo');
     }
 
+    /** @override */
+    canChangeChannel() {
+      return cr.sendWithPromise('canChangeChannel');
+    }
+
     /** @override */
     getVersionInfo() {
       return cr.sendWithPromise('getVersionInfo');

chrome/browser/resources/settings/chromeos/os_about_page/detailed_build_info.js

@@ -49,13 +49,21 @@ Polymer({
   /** @private */
   updateChannelInfo_() {
     const browserProxy = settings.AboutPageBrowserProxyImpl.getInstance();
+
+    // canChangeChannel() call is expected to be low-latency, so fetch this
+    // value by itself to ensure UI consistency (see https://crbug.com/848750).
+    browserProxy.canChangeChannel().then(canChangeChannel => {
+      this.canChangeChannel_ = canChangeChannel;
+    });
+
+    // getChannelInfo() may have considerable latency due to updates. Fetch this
+    // metadata as part of a separate request.
     browserProxy.getChannelInfo().then(info => {
       this.channelInfo_ = info;
       // Display the target channel for the 'Currently on' message.
       this.currentlyOnChannelText_ = this.i18n(
           'aboutCurrentlyOnChannel',
           this.i18n(settings.browserChannelToI18nId(info.targetChannel)));
-      this.canChangeChannel_ = info.canChangeChannel;
     });
   },
 

chrome/browser/ui/webui/settings/about_handler.cc

@@ -411,6 +411,10 @@ void AboutHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "getChannelInfo", base::BindRepeating(&AboutHandler::HandleGetChannelInfo,
                                             base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "canChangeChannel",
+      base::BindRepeating(&AboutHandler::HandleCanChangeChannel,
+                          base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "refreshTPMFirmwareUpdateStatus",
       base::BindRepeating(&AboutHandler::HandleRefreshTPMFirmwareUpdateStatus,
@@ -636,6 +640,15 @@ void AboutHandler::HandleGetChannelInfo(const base::ListValue* args) {
                  callback_id));
 }
 
+void AboutHandler::HandleCanChangeChannel(const base::ListValue* args) {
+  CHECK_EQ(1U, args->GetSize());
+  std::string callback_id;
+  CHECK(args->GetString(0, &callback_id));
+  ResolveJavascriptCallback(
+      base::Value(callback_id),
+      base::Value(CanChangeChannel(Profile::FromWebUI(web_ui()))));
+}
+
 void AboutHandler::OnGetCurrentChannel(std::string callback_id,
                                        const std::string& current_channel) {
   version_updater_->GetChannel(
@@ -651,8 +664,6 @@ void AboutHandler::OnGetTargetChannel(std::string callback_id,
       new base::DictionaryValue);
   channel_info->SetString("currentChannel", current_channel);
   channel_info->SetString("targetChannel", target_channel);
-  channel_info->SetBoolean("canChangeChannel",
-                           CanChangeChannel(Profile::FromWebUI(web_ui())));
 
   ResolveJavascriptCallback(base::Value(callback_id), *channel_info);
 }

chrome/browser/ui/webui/settings/about_handler.h

@@ -106,8 +106,12 @@ class AboutHandler : public settings::SettingsPageUIHandler,
       std::string callback_id,
       std::unique_ptr<base::DictionaryValue> version_info);
 
-  // Retrieves combined channel info.
+  // Retrieves channel info.
   void HandleGetChannelInfo(const base::ListValue* args);
+
+  // Checks whether we can change the current channel.
+  void HandleCanChangeChannel(const base::ListValue* args);
+
   // Callbacks for version_updater_->GetChannel calls.
   void OnGetCurrentChannel(std::string callback_id,
                            const std::string& current_channel);

chrome/test/data/webui/settings/chromeos/os_about_page_tests.js

@@ -527,6 +527,7 @@ cr.define('settings_about_page', function() {
 
         await Promise.all([
           browserProxy.whenCalled('pageReady'),
+          browserProxy.whenCalled('canChangeChannel'),
           browserProxy.whenCalled('getChannelInfo'),
           browserProxy.whenCalled('getVersionInfo'),
         ]);
@@ -544,7 +545,7 @@ cr.define('settings_about_page', function() {
         browserProxy.setCanChangeChannel(canChangeChannel);
         page = document.createElement('settings-detailed-build-info');
         document.body.appendChild(page);
-        await browserProxy.whenCalled('getChannelInfo');
+        await browserProxy.whenCalled('canChangeChannel');
         const changeChannelButton = page.$$('cr-button');
         assertTrue(!!changeChannelButton);
         assertEquals(canChangeChannel, !changeChannelButton.disabled);
@@ -558,12 +559,40 @@ cr.define('settings_about_page', function() {
         return checkChangeChannelButton(false);
       });
 
+      /**
+       * Checks whether the "change channel" button state (enabled/disabled)
+       * is correct before getChannelInfo() returns
+       * (see https://crbug.com/848750). Here, getChannelInfo() is blocked
+       * manually until after the button check.
+       */
+      async function checkChangeChannelButtonWithDelayedChannelState(
+          canChangeChannel) {
+        const resolver = new PromiseResolver();
+        browserProxy.getChannelInfo = async function() {
+          await resolver.promise;
+          this.methodCalled('getChannelInfo');
+          return Promise.resolve(this.channelInfo_);
+        };
+        const result = await checkChangeChannelButton(canChangeChannel);
+        resolver.resolve();
+        return result;
+      }
+
+      test('ChangeChannel_EnabledWithDelayedChannelState', function() {
+        return checkChangeChannelButtonWithDelayedChannelState(true);
+      });
+
+      test('ChangeChannel_DisabledWithDelayedChannelState', function() {
+        return checkChangeChannelButtonWithDelayedChannelState(false);
+      });
+
       async function checkCopyBuildDetailsButton() {
         page = document.createElement('settings-detailed-build-info');
         document.body.appendChild(page);
         const copyBuildDetailsButton = page.$$('cr-icon-button');
         await browserProxy.whenCalled('getVersionInfo');
         await browserProxy.whenCalled('getChannelInfo');
+        await browserProxy.whenCalled('canChangeChannel');
 
         const expectedClipBoardText =
             `${loadTimeData.getString('application_label')}: ` +

chrome/test/data/webui/settings/chromeos/test_about_page_browser_proxy_chromeos.js

@@ -13,6 +13,7 @@
       'refreshUpdateStatus',
       'openHelpPage',
       'openFeedbackDialog',
+      'canChangeChannel',
       'getChannelInfo',
       'getVersionInfo',
       'getRegulatoryInfo',
@@ -39,9 +40,11 @@
     this.channelInfo_ = {
       currentChannel: BrowserChannel.BETA,
       targetChannel: BrowserChannel.BETA,
-      canChangeChannel: true,
     };
 
+    /** @private {!boolean} */
+    this.canChangeChannel_ = true;
+
     /** @private {?RegulatoryInfo} */
     this.regulatoryInfo_ = null;
 
@@ -101,7 +104,7 @@
 
   /** @param {boolean} canChangeChannel */
   setCanChangeChannel(canChangeChannel) {
-    this.channelInfo_.canChangeChannel = canChangeChannel;
+    this.canChangeChannel_ = canChangeChannel;
   }
 
   /**
@@ -145,6 +148,12 @@
     return Promise.resolve(this.channelInfo_);
   }
 
+  /** @override */
+  canChangeChannel() {
+    this.methodCalled('canChangeChannel');
+    return Promise.resolve(this.canChangeChannel_);
+  }
+
   /** @override */
   getEnabledReleaseNotes() {
     this.methodCalled('getEnabledReleaseNotes');