MD Settings: add tests for mischievous default zoom handling

chrome/test/data/webui/settings/appearance_page_test.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.
 
 /**
  * @constructor
  * @implements {settings.AppearanceBrowserProxy}
  * @extends {settings.TestBrowserProxy}
  */
 var TestAppearanceBrowserProxy = function() {
   settings.TestBrowserProxy.call(this, [
+    'getDefaultZoom',
     'getThemeInfo',
     'isSupervised',
     'openWallpaperManager',
     'useDefaultTheme',
     'useSystemTheme',
   ]);
 };
 
 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'});
   },
 
   /** @override */
   isSupervised: function() {
     this.methodCalled('isSupervised');
     return this.isSupervised_;
   },
 
   /** @override */
   openWallpaperManager: function() {
     this.methodCalled('openWallpaperManager');
   },
 
   /** @override */
   useDefaultTheme: function() {
     this.methodCalled('useDefaultTheme');
   },
 
   /** @override */
   useSystemTheme: function() {
     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;
   },
 };
 
 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(); });
 
   if (cr.isChromeOS) {
     test('wallpaperManager', function() {
       var button = appearancePage.$.wallpaperButton;
       assertTrue(!!button);
       MockInteractions.tap(button);
       return appearanceBrowserProxy.whenCalled('openWallpaperManager');
@@ skipping 72 matching lines @@
       Polymer.dom.flush();
 
       // With a custom theme installed, "RESET TO DEFAULT" should show.
       var button = appearancePage.$$('#useDefault');
       assertTrue(!!button);
 
       MockInteractions.tap(button);
       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());
+    });
+  });
 });