MD Settings: About page, updating buttons based on current update status.

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({
   is: 'settings-about-page',
 
   behaviors: [WebUIListenerBehavior, RoutableBehavior, I18nBehavior],
 
   properties: {
     /**
      * The current active route.
      */
     currentRoute: {
       type: Object,
       notify: true,
     },
 
     /** @private {?UpdateStatusChangedEvent} */
     currentUpdateStatusEvent_: Object,
 
 <if expr="chromeos">
-    /**
-     * Whether the current and target channel is different.
-     * @private
-     */
-    channelsDiffer_: Boolean,
+    /** private */

[tommycli, 2016/05/20 22:53:18]

nit: @private {boolean} ?

[dpapad, 2016/05/20 23:25:35]

I've been told in other code reviews that the compiler's Polymer pass
understands the type from the following line, so only @private is needed here.

[tommycli, 2016/05/20 23:32:28]

Sounds good. I think you still need to add an "@" character then.

+    hasCheckedForUpdates_: Boolean,
+
+    /** @private {!BrowserChannel} */
+    currentChannel_: String,
 
     /** @private {!BrowserChannel} */
     targetChannel_: String,
 </if>
   },
 
   /** @private {?settings.AboutPageBrowserProxy} */
   browserProxy_: null,
 
   /**
    * @type {string} Selector to get the sections.
    * TODO(michaelpg): replace duplicate docs with @override once b/24294625
    * is fixed.
    */
   sectionSelector: 'settings-section',
 
   /** @override */
   ready: function() {
     this.browserProxy_ = settings.AboutPageBrowserProxyImpl.getInstance();
     this.browserProxy_.pageReady();
 
 <if expr="chromeos">
     Promise.all([
       this.browserProxy_.getCurrentChannel(),
       this.browserProxy_.getTargetChannel(),
     ]).then(function(channels) {
+      this.currentChannel_ = channels[0];
       this.targetChannel_ = channels[1];
-      this.channelsDiffer_ = channels[0] != this.targetChannel_;
+
       this.startListening_();
     }.bind(this));
 </if>
 <if expr="not chromeos">
     this.startListening_();
 </if>
   },
 
   /** @private */
   startListening_: function() {
     this.addWebUIListener(
         'update-status-changed',
-        /** @param {!UpdateStatusChangedEvent} event */
-        function(event) {
-          this.currentUpdateStatusEvent_ = event;
-        }.bind(this));
+        this.onUpdateStatusChanged_.bind(this));
     this.browserProxy_.refreshUpdateStatus();
   },
 
-   /** @override */
+  /**
+   * @param {!UpdateStatusChangedEvent} event
+   * @private
+   */
+  onUpdateStatusChanged_: function(event) {
+<if expr="chromeos">
+    if (event.status == UpdateStatus.CHECKING)
+      this.hasCheckedForUpdates_ = true;
+</if>
+    this.currentUpdateStatusEvent_ = event;
+  },
+
+  /** @override */
   attached: function() {
     this.scroller = this.parentElement;
   },
 
   /** @private */
   onHelpTap_: function() {
     this.browserProxy_.openHelpPage();
   },
 
+  /** @private */
+  onRelaunchTap_: function() {
+    this.browserProxy_.relaunchNow();
+  },
+
   /**
    * @return {boolean}
    * @private
    */
   shouldShowUpdateStatus_: function() {
     return this.currentUpdateStatusEvent_.status != UpdateStatus.DISABLED;
   },
 
   /**
+   * @return {boolean}
+   * @private
+   */
+  shouldShowRelaunch_: function() {
+    var shouldShow = false;

[tommycli, 2016/05/20 22:53:18]

nit: eliminate var in favor of direct returns?

[dpapad, 2016/05/20 23:25:35]

Can't avoid the variable here, that was my first version. The compiler does not
understand <if> statements, all it does is to ignore them, so if I do direct
returns, I get the "unreachadle code" error because all the compiler sees is

return 'foo';
return 'bar'; // unreachable code

[tommycli, 2016/05/20 23:32:28]

Ahh... that makes sense. Thanks for the clarification.

+<if expr="not chromeos">
+    shouldShow =
+        this.currentUpdateStatusEvent_.status == UpdateStatus.NEARLY_UPDATED;
+</if>
+<if expr="chromeos">
+    shouldShow = !this.isTargetChannelMoreStable_() &&
+        this.currentUpdateStatusEvent_.status == UpdateStatus.NEARLY_UPDATED;
+</if>
+    return shouldShow;
+  },
+
+  /**
    * @return {string}
    * @private
    */
   getUpdateStatusMessage_: function() {
     switch (this.currentUpdateStatusEvent_.status) {
       case UpdateStatus.CHECKING:
         return this.i18n('aboutUpgradeCheckStarted');
       case UpdateStatus.NEARLY_UPDATED:
 <if expr="chromeos">
-        if (this.channelsDiffer_)
+        if (this.currentChannel_ != this.targetChannel_)
           return this.i18n('aboutUpgradeSuccessChannelSwitch');
 </if>
         return this.i18n('aboutUpgradeRelaunch');
       case UpdateStatus.UPDATED:
         return this.i18n('aboutUpgradeUpToDate');
       case UpdateStatus.UPDATING:
 <if expr="chromeos">
-        if (this.channelsDiffer_) {
+        if (this.currentChannel_ != this.targetChannel_) {
           return this.i18n('aboutUpgradeUpdatingChannelSwitch',
               this.i18n(settings.browserChannelToI18nId(this.targetChannel_)));
         }
 </if>
         return this.i18n('aboutUpgradeUpdating');
       default:
-        return this.currentUpdateStatusEvent_.message;
+        return this.currentUpdateStatusEvent_.message || '';

[tommycli, 2016/05/20 22:53:18]

Looking at the C++ handler, is it possible for the event to be defined but the
message to be undefined?

I have two questions:

What happens when you access a property of an undefined object? Does it also
give undefined? or is it an exception?

Also - do computed bindings get called while the object is undefined? I thought
I heard it didn't, but i'm not sure.

This is also kind of a nitty comment, i'm sure this works just fine.

[dpapad, 2016/05/20 23:25:35]

The C++ is actually passing an empty string whenever the "message" does not
apply to the current state. AFAIU UpdateStatus.FAILED state is the only state
where "message" is not empty. But, I changed the typedef in the proxy to declare
the message as optional, such that I don't have to repeat this meaningless field
when I create an UpdateStatusEvent payload myself (either in tests, or at line
230 here.

Accessing an undefined member of an object simply returns undefined. You can try
in the console
var a = {};
console.log(a.foo);

Regarding computed bindings, nothing depends on
this.currentUpdateStatusEvent_.message. All bindings depend on
this.currentUpdateStatusEvent_ itself, and the code never modifies a subproperty
of that member variable, it is always re-assigned as a whole.

     }
   },
 
   /**
    * @return {?string}
    * @private
    */
   getIcon_: function() {
     switch (this.currentUpdateStatusEvent_.status) {
       case UpdateStatus.DISABLED_BY_ADMIN:
@@ skipping 16 matching lines @@
     switch (this.currentUpdateStatusEvent_.status) {
       case UpdateStatus.CHECKING:
       case UpdateStatus.UPDATING:
         return 'chrome://resources/images/throbber_small.svg';
       default:
         return null;
     }
   },
 
 <if expr="chromeos">
+  /**
+   * @return {boolean}
+   * @private
+   */
+  isTargetChannelMoreStable_: function() {

[tommycli, 2016/05/20 22:53:18]

should this have some asserts at the top that the current and target channels
are non-empty defined strings?

[dpapad, 2016/05/20 23:25:35]

Done.

+    // List of channels in increasing stability order.
+    var channelList = [
+      BrowserChannel.DEV,
+      BrowserChannel.BETA,
+      BrowserChannel.STABLE,
+    ];
+    var currentIndex = channelList.indexOf(this.currentChannel_);
+    var targetIndex = channelList.indexOf(this.targetChannel_);
+    return currentIndex < targetIndex;
+  },
+
   /** @private */
   onDetailedBuildInfoTap_: function() {
     var animatedPages = /** @type {!SettingsAnimatedPagesElement} */ (
         this.$.pages);
     animatedPages.setSubpageChain(['detailed-build-info']);
   },
+
+  /** @private */
+  onRelaunchAndPowerwashTap_: function() {
+    // TODO(dpapad): Implement this.
+  },
+
+  /**
+   * @return {boolean}
+   * @private
+   */
+  shouldShowRelaunchAndPowerwash_: function() {
+    return this.isTargetChannelMoreStable_() &&
+        this.currentUpdateStatusEvent_.status == UpdateStatus.NEARLY_UPDATED;
+  },
+
+  /** @private */
+  onCheckUpdatesTap_: function() {
+    this.onUpdateStatusChanged_({status: UpdateStatus.CHECKING});
+    this.browserProxy_.requestUpdate();
+  },
+
+  /**
+   * @return {boolean}
+   * @private
+   */
+  shouldShowCheckUpdates_: function() {
+    return !this.hasCheckedForUpdates_ ||
+        this.currentUpdateStatusEvent_.status == UpdateStatus.FAILED;
+  },
 </if>
 
 <if expr="_google_chrome">
   /** @private */
   onReportIssueTap_: function() {
     this.browserProxy_.openFeedbackDialog();
   },
 </if>
 });