MD Settings: Fix subpage visibility and add appearance page tests

chrome/browser/resources/settings/downloads_page/downloads_page.js

 // Copyright 2015 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-downloads-page' is the settings page containing downloads
  * settings.
  *
  * Example:
@@ skipping 15 matching lines @@
      */
     prefs: {
       type: Object,
       notify: true,
     },
 
     /**
      * Dictionary defining page visibility.
      * @type {!DownloadsPageVisibility}
      */
-    pageVisibility: Object,
+    pageVisibility: {
+      type: Object,
+      value: function() {

[dpapad, 2017/05/02 23:45:52]

Can we follow the approach of the previous CL, where in prod code all elements
expect pageVisibility to be passed down from settings-ui? In unit test code
(where no settings-ui exists) we explicitly initialize pageVisibility when the
element is constructed.

[stevenjb, 2017/05/02 23:56:17]

So, if we don't do this, we need to ensure that any subpage that adds a
pageVisibilty property needs to also set it to an empty object in settings-ui.
I'd prefer not to require that.

The reason I changed the other pageVisibility properties to default to undefined
is that they are all bound to the one in settings-ui, which is now always
defined, so I think that would be more clear.

Does that make sense?

[dpapad, 2017/05/03 00:18:48]

Hm, I think it actually made me realize that I should have reviewed the previous
CL a bit more thoroughly. I am now seeing a problem at [1] and similar places
where we call basic-page's getPageVisibility_.

The binding does not depend on this.pageVisibility, which means that the binding
triggers only once and never re-triggers if |this.pageVisibility| is set to a
different value in basic-page. In other words it seems we are breaking the flow
of the top-level settings-ui pageVisibility such that is not guaranteed to reach
all children which is not equivalent to what has happening before [2].

[1]
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/basic_...
[2]
https://codereview.chromium.org/2852433003/diff/20001/chrome/browser/resource...

[stevenjb, 2017/05/03 19:58:40]

Ah, yes, you are correct, that's wrong, and fixing that eliminates the need to
initialize the subpage pageVisibility, so I think all is well.

+        return {};
+      },
+    },
 
     /** @private */
     autoOpenDownloads_: {
       type: Boolean,
       value: false,
     },
   },
 
   /** @private {?settings.DownloadsBrowserProxy} */
   browserProxy_: null,
 
   /** @override */
   attached: function() {
     this.browserProxy_ = settings.DownloadsBrowserProxyImpl.getInstance();
 
     this.addWebUIListener('auto-open-downloads-changed', function(autoOpen) {
       this.autoOpenDownloads_ = autoOpen;
     }.bind(this));
 
     this.browserProxy_.initializeDownloads();
   },
 
+  /**
+   * @param {boolean|undefined} visibility
+   * @return {boolean}
+   * @private
+   */
+  showPage_: function(visibility) {
+    return visibility !== false;
+  },
+
   /** @private */
   selectDownloadLocation_: function() {
     listenOnce(this, 'transitionend', function() {
       this.browserProxy_.selectDownloadLocation();
     }.bind(this));
   },
 
   // <if expr="chromeos">
   /**
    * @param {string} path
@@ skipping 11 matching lines @@
     path = path.replace(/\//g, ' \u203a ');
     return path;
   },
   // </if>
 
   /** @private */
   onClearAutoOpenFileTypesTap_: function() {
     this.browserProxy_.resetAutoOpenFileTypes();
   },
 });