MD Settings: add tests for mischievous default zoom handling

chrome/test/data/webui/settings/appearance_page_test.js

Index: chrome/test/data/webui/settings/appearance_page_test.js
diff --git a/chrome/test/data/webui/settings/appearance_page_test.js b/chrome/test/data/webui/settings/appearance_page_test.js
index 2e08fb5db526986c3ceac32d2a30c7989a6d18b4..4aceea22cb37833e3faf0ac51f97d71e9fb00f96 100644
--- a/chrome/test/data/webui/settings/appearance_page_test.js
+++ b/chrome/test/data/webui/settings/appearance_page_test.js
@@ -9,6 +9,7 @@
  */
 var TestAppearanceBrowserProxy = function() {
   settings.TestBrowserProxy.call(this, [
+    'getDefaultZoom',
     'getThemeInfo',
     'isSupervised',
     'openWallpaperManager',
@@ -21,9 +22,18 @@ TestAppearanceBrowserProxy.prototype = {
   __proto__: settings.TestBrowserProxy.prototype,
 
   /** @private */
+  defaultZoom_: 1,

[dpapad, 2016/11/10 02:38:34]

Can me move this within the constructor instead? I know this also works because
we only have one TestAppearanceBrowserProxy at a time, but generally speaking
data members make more sense in the constructor.

[Dan Beam, 2016/11/10 02:52:38]

Done, but is this request based on a style rule or statistically motivated
precedent?

[dpapad, 2016/11/10 17:57:46]

Both.
https://google.github.io/styleguide/javascriptguide.xml?showone=Method_and_pr...

[Dan Beam, 2016/11/10 18:54:36]

interesting that the style guide calls this out explicitly.  i would say their
rationale doesn't actually apply in this case (neither method we're considering
changes shape) and is super implementation specific / has changed over the
years.

the most interesting bit, in my opinion, is how this actually works when you do:

  function Class() {}
  Class.prototype = {foo: 1};

  var c = new Class();  // c.foo == 1
  c.foo = 3;  // c.foo == 3

is actually fairly dirty, as if you do something like:

  // I'm done with |foo|, let's remove it!
  delete c.foo;  // delete returns true, c.foo == 1

as it's actually assigning a property on the instance, closer on the __proto__
chain.

tl;dr - I'm fine to do this.* = initialization in the ctor, but the statistics
question was a bit loaded: I know there's tons of webui that relies on
statically initializing prototype POD members :(.  I don't really want to change
all the existing code, but i'll keep it in mind.

thanks for the info!

+
+  /** @private */
   isSupervised_: false,
 
   /** @override */
+  getDefaultZoom: function() {
+    this.methodCalled('getDefaultZoom');
+    return Promise.resolve(this.defaultZoom_);
+  },
+
+  /** @override */
   getThemeInfo: function(themeId) {
     this.methodCalled('getThemeInfo', themeId);
     return Promise.resolve({name: 'Sports car red'});
@@ -50,6 +60,11 @@ TestAppearanceBrowserProxy.prototype = {
     this.methodCalled('useSystemTheme');
   },
 
+  /** @param {number} defaultZoom */
+  setDefaultZoom: function(defaultZoom) {
+    this.defaultZoom_ = defaultZoom;
+  },
+
   /** @param {boolean} Whether the user is supervised */
   setIsSupervised: function(isSupervised) {
     this.isSupervised_ = isSupervised;
@@ -61,28 +76,32 @@ var appearancePage = null;
 /** @type {?TestAppearanceBrowserProxy} */
 var appearanceBrowserProxy = null;
 
+function createAppearancePage() {
+  PolymerTest.clearBody();
+
+  appearancePage = document.createElement('settings-appearance-page');

[dpapad, 2016/11/10 02:38:34]

Nit(optional): Would "appearancePage.prefs = " also trigger Polymer observers?

[Dan Beam, 2016/11/10 02:52:38]

there's a few things I could do to attempt to simulate the side effects we want
(set appearancePage.defaultZoom_ directly, for instance), but it felt like
recreating the page completely was closest to "refresh" that I knew of, and
that's the repro steps on the bug (and what i've been doing in local testing).

it also seems the least likely to need to update the tests if the implementation
changes.

+  appearancePage.set('prefs', {
+    extensions: {
+      theme: {
+        id: {
+          value: '',
+        },
+        use_system: {
+          value: false,
+        },
+      },
+    },
+  });
+
+  document.body.appendChild(appearancePage);
+  Polymer.dom.flush();
+}
+
 suite('AppearanceHandler', function() {
   setup(function() {
     appearanceBrowserProxy = new TestAppearanceBrowserProxy();
     settings.AppearanceBrowserProxyImpl.instance_ = appearanceBrowserProxy;
-
-    PolymerTest.clearBody();
-
-    appearancePage = document.createElement('settings-appearance-page');
-    appearancePage.set('prefs', {
-      extensions: {
-        theme: {
-          id: {
-            value: '',
-          },
-          use_system: {
-            value: false,
-          },
-        },
-      },
-    });
-    document.body.appendChild(appearancePage);
-    Polymer.dom.flush();
+    createAppearancePage();
   });
 
   teardown(function() { appearancePage.remove(); });
@@ -175,4 +194,36 @@ suite('AppearanceHandler', function() {
       return appearanceBrowserProxy.whenCalled('useDefaultTheme');
     });
   }
+
+  test('default zoom handling', function() {
+    function getDefaultZoomText() {
+      var zoomLevel = appearancePage.$.zoomLevel;
+      return zoomLevel.options[zoomLevel.selectedIndex].textContent.trim();
+    }
+
+    return appearanceBrowserProxy.whenCalled('getDefaultZoom').then(function() {
+      assertEquals('100%', getDefaultZoomText());
+
+      appearanceBrowserProxy.setDefaultZoom(2 / 3);
+      appearanceBrowserProxy.reset();

[dpapad, 2016/11/10 02:38:33]

Perhaps move that call within createAppearancePage()? I can't see a scenario
where you would create a new page, but don't want to reset() the proxy.

[Dan Beam, 2016/11/10 02:52:38]

Done.

+      createAppearancePage();
+      return appearanceBrowserProxy.whenCalled('getDefaultZoom');
+    }).then(function() {
+      assertEquals('67%', getDefaultZoomText());
+
+      appearanceBrowserProxy.setDefaultZoom(11 / 10);
+      appearanceBrowserProxy.reset();
+      createAppearancePage();
+      return appearanceBrowserProxy.whenCalled('getDefaultZoom');
+    }).then(function() {
+      assertEquals('110%', getDefaultZoomText());
+
+      appearanceBrowserProxy.setDefaultZoom(1.7499999999999);
+      appearanceBrowserProxy.reset();
+      createAppearancePage();
+      return appearanceBrowserProxy.whenCalled('getDefaultZoom');
+    }).then(function() {
+      assertEquals('175%', getDefaultZoomText());
+    });
+  });
 });