MD Settings: Fix subpage visibility and add appearance page tests

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

Index: chrome/browser/resources/settings/downloads_page/downloads_page.js
diff --git a/chrome/browser/resources/settings/downloads_page/downloads_page.js b/chrome/browser/resources/settings/downloads_page/downloads_page.js
index 76bb8d7108f79afdc8ad965476364d99e3b73729..945af95668c5782a0be625bff08b9ce6d0de1f9f 100644
--- a/chrome/browser/resources/settings/downloads_page/downloads_page.js
+++ b/chrome/browser/resources/settings/downloads_page/downloads_page.js
@@ -33,7 +33,12 @@ Polymer({
      * 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_: {
@@ -56,6 +61,15 @@ Polymer({
     this.browserProxy_.initializeDownloads();
   },
 
+  /**
+   * @param {boolean|undefined} visibility
+   * @return {boolean}
+   * @private
+   */
+  showPage_: function(visibility) {
+    return visibility !== false;
+  },
+
   /** @private */
   selectDownloadLocation_: function() {
     listenOnce(this, 'transitionend', function() {