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

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

Index: chrome/browser/resources/settings/about_page/about_page.js
diff --git a/chrome/browser/resources/settings/about_page/about_page.js b/chrome/browser/resources/settings/about_page/about_page.js
index 4bcac8cc686f37c9b957259808b84096f843b37f..14b392f2ad9356a5e589b997abf0d0de0395a6fc 100644
--- a/chrome/browser/resources/settings/about_page/about_page.js
+++ b/chrome/browser/resources/settings/about_page/about_page.js
@@ -24,11 +24,11 @@ Polymer({
     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,
@@ -55,8 +55,9 @@ Polymer({
       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>
@@ -69,14 +70,23 @@ Polymer({
   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;
   },
@@ -86,6 +96,11 @@ Polymer({
     this.browserProxy_.openHelpPage();
   },
 
+  /** @private */
+  onRelaunchTap_: function() {
+    this.browserProxy_.relaunchNow();
+  },
+
   /**
    * @return {boolean}
    * @private
@@ -95,6 +110,23 @@ Polymer({
   },
 
   /**
+   * @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
    */
@@ -104,7 +136,7 @@ Polymer({
         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');
@@ -112,14 +144,14 @@ Polymer({
         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.

     }
   },
 
@@ -156,12 +188,57 @@ Polymer({
   },
 
 <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">