MD Settings: change how tap is handled on custom toggle rows

chrome/browser/resources/settings/privacy_page/privacy_page.js

Index: chrome/browser/resources/settings/privacy_page/privacy_page.js
diff --git a/chrome/browser/resources/settings/privacy_page/privacy_page.js b/chrome/browser/resources/settings/privacy_page/privacy_page.js
index 8e0c4b029c5ad5253e57388a4c64b732355b14aa..017e38db5f96f78b1a6845c9bf43cd617398e331 100644
--- a/chrome/browser/resources/settings/privacy_page/privacy_page.js
+++ b/chrome/browser/resources/settings/privacy_page/privacy_page.js
@@ -31,14 +31,26 @@ Polymer({
     pageVisibility: Object,
 
 // <if expr="_google_chrome and not chromeos">
-    /** @type {MetricsReporting} */
-    metricsReporting_: Object,
+    /** @private {chrome.settingsPrivate.PrefObject} */
+    metricsReportingPref_: {
+      type: Object,
+      value: function() {
+        // TODO(dbeam): this is basically only to appease PrefControlBehavior.
+        // Maybe add a no-validate attribute instead? This makes little sense.
+        return /** @type {chrome.settingsPrivate.PrefObject} */({});
+      },
+    },
 
     showRestart_: Boolean,
 // </if>
 
-    /** @private */
-    safeBrowsingExtendedReportingEnabled_: Boolean,
+    /** @private {chrome.settingsPrivate.PrefObject} */
+    safeBrowsingExtendedReportingPref_: {
+      type: Object,
+      value: function() {
+        return /** @type {chrome.settingsPrivate.PrefObject} */({});
+      },
+    },
 
     /** @private */
     showClearBrowsingDataDialog_: Boolean,
@@ -47,19 +59,19 @@ Polymer({
   ready: function() {
     this.ContentSettingsTypes = settings.ContentSettingsTypes;
 
-// <if expr="_google_chrome and not chromeos">
-    var boundSetMetricsReporting = this.setMetricsReporting_.bind(this);
-    this.addWebUIListener('metrics-reporting-change', boundSetMetricsReporting);
-
     var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();
-    browserProxy.getMetricsReporting().then(boundSetMetricsReporting);
+
+// <if expr="_google_chrome and not chromeos">
+    var setMetricsReportingPref = this.setMetricsReportingPref_.bind(this);
+    this.addWebUIListener('metrics-reporting-change', setMetricsReportingPref);
+    browserProxy.getMetricsReporting().then(setMetricsReportingPref);
 // </if>
 
-    var boundSetSber = this.setSafeBrowsingExtendedReporting_.bind(this);
-    this.addWebUIListener('safe-browsing-extended-reporting-change',
-                          boundSetSber);
-    settings.PrivacyPageBrowserProxyImpl.getInstance()
-        .getSafeBrowsingExtendedReporting().then(boundSetSber);
+    var setSber = this.setSafeBrowsingExtendedReporting_.bind(this);
+    this.addWebUIListener('safe-browsing-extended-reporting-change', setSber);
+    browserProxy.getSafeBrowsingExtendedReporting().then(setSber);
+
+    this.listen(this, 'iron-change', 'onIronChange_');

[dpapad, 2017/02/09 19:51:45]

I recall an issue we discovered in the past, were relying on the iron-change
event coming from the internals was racy (it fired before settings-toggle-button
itself has updated its state). See context at
https://bugs.chromium.org/p/chromium/issues/detail?id=665979. Not sure if this
is affecting us here, but worth checking.

[Dan Beam, 2017/02/09 20:48:45]

that bug talks about settings-*checkbox*, not settings-*toggle-button*

dragging the toggle only dispatches an iron-change (but not a 'change')

[dpapad, 2017/02/09 23:53:02]

The related code linked in the bug is inside
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro...,
which is used by both elements. Also the issue is indeed happening with
'iron-change' not 'change', which both elements seem to fire, so I am still not
so sure that settings-toggle-button is fine using iron-change.

Per our previous discussion (see
https://bugs.chromium.org/p/chromium/issues/detail?id=665979#c2), we were
considering calling stopPropagation() on the iron-change events to prevent
anyone from using those events (and potentially add our own event that is fired
at the right moment).

   },
 
   /** @protected */
@@ -111,48 +123,75 @@ Polymer({
         'https://support.google.com/chrome/?p=settings_manage_exceptions');
   },
 
+  /**
+   * @param {Event} e
+   * @private
+   */
+  onIronChange_: function(e) {

[dpapad, 2017/02/09 19:51:45]

Can we not add dedicated listeners to each button?

[Dan Beam, 2017/02/09 20:48:45]

you can but it doesn't buy much because you still have to .path.indexOf() in
both

i originally did this but this version's code is simpler, imo

do you want me to show you what 2 separate listeners looks like?

[dpapad, 2017/02/09 23:53:02]

I am curious. Polymer.dom(e).localTarget gives you the local element, so why is
a call path.indexOf() needed. As another data point, I don't see any other call
to that method in our code, so would like to understand what makes this case
unique.

[Dan Beam, 2017/02/09 23:57:53]

because the localTarget is actually the paper-toggle-button inside of
this.$.safeBrowsingExtendedReportingControl

as well as
this.$.safeBrowsingExtendedReportingControl.contains(Polymer.dom(e).localTarget)
does not work (because .contains() doesn't work across shadow)

i know because i tried those 2 first

if you'd like i can stop the iron-change inside of
SettingsBooleanPrefBehaviorWhatever and do
this.fire('boolean-pref-change-or-whatever', this.checked) but it didn't really
seem much better (although it wouldn't break if we changed the implementation)

+    if (e.path.indexOf(this.$.safeBrowsingExtendedReportingControl) >= 0) {
+      var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();

[dpapad, 2017/02/09 19:51:45]

Nit: PrivacyPageBrowserProxyImpl.getInstance() is used quite a few times in this
file, I think is worth caching it in a member var.

[Dan Beam, 2017/02/09 20:48:45]

Done.

+      var enabled = this.$.safeBrowsingExtendedReportingControl.checked;
+      browserProxy.setSafeBrowsingExtendedReportingEnabled(enabled);
 // <if expr="_google_chrome and not chromeos">
-  /** @private */
-  onMetricsReportingControlTap_: function() {
-    var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();
-    var enabled = this.$.metricsReportingControl.checked;
-    browserProxy.setMetricsReportingEnabled(enabled);
+    } else if (e.path.indexOf(this.$.metricsReportingControl) >= 0) {
+      var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();
+      var enabled = this.$.metricsReportingControl.checked;
+      browserProxy.setMetricsReportingEnabled(enabled);
+// </if>
+    }
   },
 
+// <if expr="_google_chrome and not chromeos">
   /**
    * @param {!MetricsReporting} metricsReporting
    * @private
    */
-  setMetricsReporting_: function(metricsReporting) {
+  setMetricsReportingPref_: function(metricsReporting) {
+    var hadPreviousPref = this.metricsReportingPref_.value !== undefined;
+    var pref = {
+      key: '',
+      type: chrome.settingsPrivate.PrefType.BOOLEAN,
+      value: metricsReporting.enabled,
+    };
+    if (metricsReporting.managed) {
+      pref.enforcement = chrome.settingsPrivate.Enforcement.ENFORCED;
+      pref.controlledBy = chrome.settingsPrivate.ControlledBy.USER_POLICY;
+    }
+
+    this.unlisten(this, 'iron-change', 'onIronChange_');
+    this.metricsReportingPref_ = pref;
+    this.listen(this, 'iron-change', 'onIronChange_');
+
     // TODO(dbeam): remember whether metrics reporting was enabled when Chrome
     // started.
-    if (metricsReporting.managed) {
+    if (metricsReporting.managed)
       this.showRestart_ = false;
-    } else if (this.metricsReporting_ &&
-               metricsReporting.enabled != this.metricsReporting_.enabled) {
+    else if (hadPreviousPref)
       this.showRestart_ = true;
-    }
-    this.metricsReporting_ = metricsReporting;
   },
 
-  /** @private */
-  onRestartTap_: function() {
+  /**
+   * @param {Event} e
+   * @private
+   */
+  onRestartTap_: function(e) {
+    e.stopPropagation();
     settings.LifetimeBrowserProxyImpl.getInstance().restart();
   },
 // </if>
 
-  /** @private */
-  onSafeBrowsingExtendedReportingControlTap_: function() {
-    var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();
-    var enabled = this.$.safeBrowsingExtendedReportingControl.checked;
-    browserProxy.setSafeBrowsingExtendedReportingEnabled(enabled);
-  },
-
-  /** @param {boolean} enabled Whether reporting is enabled or not.
-    * @private
-    */
+  /**
+   * @param {boolean} enabled Whether reporting is enabled or not.
+   * @private
+   */
   setSafeBrowsingExtendedReporting_: function(enabled) {
-    this.safeBrowsingExtendedReportingEnabled_ = enabled;
+    this.unlisten(this, 'iron-change', 'onIronChange_');

[dpapad, 2017/02/09 19:51:45]

This probably needs a comment explaining why are we unlistening and
re-listening.

[Dan Beam, 2017/02/09 20:48:45]

Done.

+    this.safeBrowsingExtendedReportingPref_ = {
+      key: '',
+      type: chrome.settingsPrivate.PrefType.BOOLEAN,
+      value: enabled,
+    };
+    this.listen(this, 'iron-change', 'onIronChange_');
   },
 
   /**