Show proper message in about Chrome OS page

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview 'settings-about-page' contains version and OS related
  * information.
  */
 
 Polymer({
@@ skipping 153 matching lines @@
 // </if>
     this.aboutBrowserProxy_.refreshUpdateStatus();
   },
 
   /**
    * @param {!UpdateStatusChangedEvent} event
    * @private
    */
   onUpdateStatusChanged_: function(event) {
 // <if expr="chromeos">
-    if (event.status == UpdateStatus.CHECKING)
+    if (event.status == UpdateStatus.CHECKING) {
       this.hasCheckedForUpdates_ = true;
-    else if (event.status == UpdateStatus.NEED_PERMISSION_TO_UPDATE) {
+    } else if (event.status == UpdateStatus.NEED_PERMISSION_TO_UPDATE) {
       this.showUpdateWarningDialog_ = true;
       this.updateInfo_ = {version: event.version, size: event.size};
+      // Shows 'check for updates' button in case that the user cancels the
+      // dialog and then intends to check for update again.
+      this.hasCheckedForUpdates_ = false;

[xiyuan, 2017/05/22 18:14:08]

Should this be moved to onUpdateWarningDialogClose_ ?

[weidongg, 2017/05/22 19:02:54]

There will be a tiny issue if I put it there: although the update
warning dialog is shown above the page, the page shows 'your chromebook is up to
date' in the background (See https://screenshot.googleplex.com/Ts7vdSHHKYO).
Then page turns that message off the dialog closes.
The reason is that both currentUpdateStatusEvent_ and hasCheckedForUpdates_
trigger the message toggling, currentUpdateStatusEvent_ turns the message on
here after receiving UPDATED and hasCheckedForUpdates_ turns the message off
when dialog closes.

[xiyuan, 2017/05/22 19:28:03]

Why would the page gets an UPDATED status while the dialog is open?

[weidongg, 2017/05/22 19:38:03]

The state transition for update engine is
IDLE->CHECKING_FOR_UPDATE->NEED_PERMISSION_TO_UPDATE->REPORTING_ERROR_EVENT->IDLE,
but JS maps IDLE to UPDATED state and version updater[1] blocks the error from
showing to user by map error state to UPDATED. So for JS the state transition
will be UPDATED->CHECKING->NEED_PERMISSION_TO_UPDATE->UPDATED->UPDATED.

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/help/version_upd...

[xiyuan, 2017/05/22 20:10:28]

Can we use fix this? Either not sending UPDATED on C++ side (at lest for
need-permission-to-update case). Or don't show the UPDATED message when update
warning dialog is shown.

Setting |this.hasCheckedForUpdates_| feels like a work around and easy to be
broken later.

[weidongg, 2017/05/22 21:13:26]

Sure, I uploaded a new patch set to prevent updated status being shown to user
when the dialog is shown.

     }
 // </if>
     this.currentUpdateStatusEvent_ = event;
   },
 
 // <if expr="_google_chrome and is_macosx">
   /**
    * @param {!PromoteUpdaterStatus} status
    * @private
    */
@@ skipping 75 matching lines @@
 // </if>
   },
 
   /**
    * @return {string}
    * @private
    */
   getUpdateStatusMessage_: function() {
     switch (this.currentUpdateStatusEvent_.status) {
       case UpdateStatus.CHECKING:
+      case UpdateStatus.NEED_PERMISSION_TO_UPDATE:
         return this.i18n('aboutUpgradeCheckStarted');
-      case UpdateStatus.NEED_PERMISSION_TO_UPDATE:
-        // This status is immediately followed by an reporting error status.
-        // When update engine reports error, UI just shows that your device is
-        // up to date. This is a bug that needs to be fixed in the future.
-        // TODO(weidongg/581071): Show proper message when update engine aborts
-        // due to cellular connection.
-        return '';
       case UpdateStatus.NEARLY_UPDATED:
 // <if expr="chromeos">
         if (this.currentChannel_ != this.targetChannel_)
           return this.i18n('aboutUpgradeSuccessChannelSwitch');
 // </if>
         return this.i18n('aboutUpgradeRelaunch');
       case UpdateStatus.UPDATED:
         return this.i18n('aboutUpgradeUpToDate');
       case UpdateStatus.UPDATING:
         assert(typeof this.currentUpdateStatusEvent_.progress == 'number');
@@ skipping 121 matching lines @@
   /**
    * @return {boolean}
    * @private
    */
   computeShowCheckUpdates_: function() {
     // Enable the update button if we are in a stale 'updated' status or
     // update has failed. Disable it otherwise.
     var staleUpdatedStatus = !this.hasCheckedForUpdates_ &&
         this.checkStatus_(UpdateStatus.UPDATED);
 
-    return staleUpdatedStatus || this.checkStatus_(UpdateStatus.FAILED) ||
-        this.checkStatus_(UpdateStatus.NEED_PERMISSION_TO_UPDATE);
+    return staleUpdatedStatus || this.checkStatus_(UpdateStatus.FAILED);
   },
 
   /**
    * @return {boolean}
    * @private
    */
   shouldShowRegulatoryInfo_: function() {
     return this.regulatoryInfo_ !== null;
   },
 
@@ skipping 13 matching lines @@
     });
   },
 
 // <if expr="_google_chrome">
   /** @private */
   onReportIssueTap_: function() {
     this.aboutBrowserProxy_.openFeedbackDialog();
   },
 // </if>
 });