MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

chrome/browser/resources/settings/appearance_page/appearance_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.
 
 /**
  * 'settings-appearance-page' is the settings page containing appearance
  * settings.
  *
  * Example:
  *
@@ skipping 13 matching lines @@
     browserProxy_: Object,
 
     /**
      * Preferences state.
      */
     prefs: {
       type: Object,
       notify: true,
     },
 
-    /**
-     * @private
-     */
+    /** @private */
     allowResetTheme_: {
       notify: true,
       type: Boolean,
       value: false,
     },
 
-    /**
-     * @private
-     */
+    /** @private */
     defaultZoomLevel_: {
       notify: true,
       type: Object,
-      value: function() {
-        return {
-          type: chrome.settingsPrivate.PrefType.NUMBER,
-        };
-      },
+      value: {type: chrome.settingsPrivate.PrefType.NUMBER},

[Dan Beam, 2016/10/11 19:03:23]

can we just omit the default value and use:

defaultZoomLevel_: {
  ...
  observer: 'zoomLevelChanged_',
},

and

zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
  if (!oldZoomLevel)  { // or === undefined or something
    // Ignore the first time this is called.
    return;
  }
}

[dpapad, 2016/10/11 19:33:06]

Unfortunately that only works when you are setting the entire object, but not
when setting defaultZoomLevel.value directly (which is what the
settings-dropdown-menu does).

[Dan Beam, 2016/10/11 19:34:48]

and the problem with that is what?

[Dan Beam, 2016/10/11 19:35:50]

i.e. why not just change this code:

[dpapad, 2016/10/11 19:43:24]

The problem is that settings-dropdown-menu does not do that, so when you change
the dropdown option with the mouse, nothing happens (no observer fires), see 

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....

Perhaps I am misunderstanding the suggestion? FWIW, I tried it before
responding. Yes, it fixed the issue of not calling chrome.settingsPrivate on
startup, at the expense of never calling it (even when user changes the value).

[Dan Beam, 2016/10/12 23:56:20]

does this work?

this.defaultZoomLevel_ = {...};
this.notifyPath('defaultZoomLevel_.value');

     },
 
     /**
+     * Needed such that the |defaultZoomLevel_| pref can be initialized
+     * without unnecessarily invoking
+     * chrome.settingsPrivate.setDefaultZoomPercent().
+     * @private
+     */
+    defaultZoomLevelInitialized_: {

[Dan Beam, 2016/10/11 19:03:23]

fyi, some polymer peeps asked me to use vanilla prototype properties instead of
properties: {} recently, so I suppose we should start doing that for things we
don't observe

[dpapad, 2016/10/11 19:33:06]

Done. I was thinking about this myself.

+      type: Boolean,
+      value: false,
+    },
+
+    /**
      * List of options for the font size drop-down menu.
      * @type {!DropdownMenuOptionList}
      */
     fontSizeOptions_: {
       readOnly: true,
       type: Array,
       value: function() {
         return [
           {value: 9, name: loadTimeData.getString('verySmall')},
           {value: 12, name: loadTimeData.getString('small')},
@@ skipping 48 matching lines @@
   ],
 
   created: function() {
     this.browserProxy_ = settings.AppearanceBrowserProxyImpl.getInstance();
   },
 
   ready: function() {
     this.$.defaultFontSize.menuOptions = this.fontSizeOptions_;
     this.$.pageZoom.menuOptions = this.pageZoomOptions_;
     // TODO(dschuyler): Look into adding a listener for the
     // default zoom percent.

[Dan Beam, 2016/10/11 19:03:23]

i suppose this is more work than we want to do right now?  what happens if 2
settings pages are open?

[dpapad, 2016/10/11 19:33:06]

Definitely not in this CL, but I think we should address.
Only the tab that initiates the change is updated, the other one displays an
obsolete value until refreshed.

Note that Options somehow manages to updated the zoom value on both tabs, so MD
Settings is currently not on par with it. I think this deserves a dedicated
crbug.

     chrome.settingsPrivate.getDefaultZoomPercent(
         this.zoomPrefChanged_.bind(this));
   },
 
   /** @override */
   attached: function() {
     // Query the initial state.
     this.browserProxy_.getResetThemeEnabled().then(
         this.setResetThemeEnabled.bind(this));
 
@@ skipping 59 matching lines @@
     } else {
       this.themeSublabel_ = this.i18n('chooseFromWebStore');
     }
   },
 
   /**
    * @param {number} percent The integer percentage of the page zoom.
    * @private
    */
   zoomPrefChanged_: function(percent) {
     this.set('defaultZoomLevel_.value', percent);

[Dan Beam, 2016/10/11 19:35:50]

this.defaultZoomLevel_ = {
  type: chrome.settingsPrivate.PrefType.NUMBER,
  value: percent
};

[michaelpg, 2016/10/11 22:01:10]

but then you're relying on a mildly obscure fact about Polymer observers, and
someone could easily give defaultZoomLevel_ a default value without testing for
what this CL is fixing

[Dan Beam, 2016/10/12 23:17:43]

I think this is defined and intentional, not mildly obscure.  it's very easy to
add a comment to this effect if you're worried people don't know about it.
i don't care about people writing code without testing it.  they're screwed no
matter what.

   },
 
   /**
    * @param {number} percent The integer percentage of the page zoom.
    * @private
    */
   zoomLevelChanged_: function(percent) {
     // The |percent| may be undefined on startup.
     if (percent === undefined)
       return;
+
+    if (!this.defaultZoomLevelInitialized_) {
+      // Simply mark this pref as initialized, no actual change was requested by
+      // the user.
+      this.defaultZoomLevelInitialized_ = true;
+      return;
+    }
+
     chrome.settingsPrivate.setDefaultZoomPercent(percent);
   },
 
   /**
    * @param {boolean} bookmarksBarVisible if bookmarks bar option is visible.
    * @return {string} 'first' if the argument is false or empty otherwise.
    * @private
    */
   getFirst_: function(bookmarksBarVisible) {
     return !bookmarksBarVisible ? 'first' : '';
   }
 });