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

chrome/browser/resources/settings/privacy_page/privacy_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.
 
 /**
  * @fileoverview
  * 'settings-privacy-page' is the settings page containing privacy and
  * security settings.
  */
 Polymer({
@@ skipping 95 matching lines @@
   },
 
   /** @private */
   onHelpTap_: function() {
     window.open(
         'https://support.google.com/chrome/?p=settings_manage_exceptions');
   },
 
 // <if expr="_google_chrome and not chromeos">
   /** @private */
-  onMetricsReportingControlTap_: function() {
+  onMetricsReportingTap_: function(e) {
+    // Both event types must match for stopPropagation() to work.

[Dan Beam, 2017/02/08 23:12:04]

this is mildly unfortunate

[dpapad, 2017/02/08 23:46:05]

This entire handler looks a bit unfortunate. I would like to understand more
what exactly are we solving with this, and look for alternatives, before
adopting this as an OK pattern.

[Dan Beam, 2017/02/09 00:27:40]

<settings-toggle-button> assumes a settings pref object to be bound to pref="". 
metrics reporting, SBER, and other things don't expose prefs directly because
the C++ disallows it (or at least discourages it).

generally, this is because there are accessors to ensure you're Doing It Right
from the C++.

metrics reporting has:

ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled()
IsMetricsReportingPolicyManaged()

which we use here

https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/metrics...

I consulted gayane@ and other from the metrics team about how we should inquire
/ observe these prefs, and the answer was "you should probably use the accessors
and potentially observe the prefs for change notifications only", so that's what
we're doing.

I can potentially make a fake pref object with the type of
chrome.settingsPrivate.PrefObject, ex:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search...

and use Polymer's mutation API (i.e. .set()), but that's also a little weird.

[Dan Beam, 2017/02/09 00:50:02]

let me try this ^ and see how it looks

+    assert(e.type == 'tap');
+
+    if (this.metricsReporting_.managed)
+      return;
+
     var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();
-    var enabled = this.$.metricsReportingControl.checked;
+    var control = this.$.metricsReportingControl;
+    var enabled = control.checked;
+
+    if (e.target == control) {
+      // Prevent the on-tap of the parent from double-changing the value.

[Dan Beam, 2017/02/08 23:12:04]

also mildly unfortunate

+      e.stopPropagation();
+    } else {
+      // If the event came from somewhere other than the control, invert the
+      // value to toggle the control.
+      enabled = !enabled;

[Dan Beam, 2017/02/08 23:12:04]

not amazing

[scottchen, 2017/02/09 00:56:43]

Instead of checking for parent double-changing and then possibly flipping the
state again here..

can we instead just say:
"browserProxy.setMetricsReportingEnabled(!safeBrowsingExtendedReportingEnabled_)"?

Basically, ignore what the toggle control says before/after - just flip the
original value.
(Similar to what I did for this:
https://codereview.chromium.org/2683583008/patch/20001/30002)

so the pseudo-code of this handler would just be:

onMetricsReportingTap_: function(){
  if(should not be clickable) 
    return
  else 
    somethingsValue = !somethingsValue; // basically you know if you get to this
point of the code, you're just trying to flip the value, regardless of where the
event comes from
}

+    }
+
     browserProxy.setMetricsReportingEnabled(enabled);
   },
 
   /**
    * @param {!MetricsReporting} metricsReporting
    * @private
    */
   setMetricsReporting_: function(metricsReporting) {
     // TODO(dbeam): remember whether metrics reporting was enabled when Chrome
     // started.
     if (metricsReporting.managed) {
       this.showRestart_ = false;
     } else if (this.metricsReporting_ &&
                metricsReporting.enabled != this.metricsReporting_.enabled) {
       this.showRestart_ = true;
     }
     this.metricsReporting_ = metricsReporting;
   },
 
   /** @private */
   onRestartTap_: function() {
     settings.LifetimeBrowserProxyImpl.getInstance().restart();
   },
 // </if>
 
   /** @private */
-  onSafeBrowsingExtendedReportingControlTap_: function() {
+  onSafeBrowsingExtendedReportingTap_: function(e) {
+    // Both event types must match for stopPropagation() to work.
+    assert(e.type == 'tap');
+
     var browserProxy = settings.PrivacyPageBrowserProxyImpl.getInstance();
-    var enabled = this.$.safeBrowsingExtendedReportingControl.checked;
+    var control = this.$.safeBrowsingExtendedReportingControl;
+    var enabled = control.checked;
+
+    if (e.target == control) {
+      // Prevent the on-tap of the parent from double-changing the value.
+      e.stopPropagation();
+    } else {
+      // If the event came from somewhere other than the control, invert the
+      // value to toggle the control.
+      enabled = !enabled;
+    }

[Dan Beam, 2017/02/08 23:12:04]

the fact that all of this is copy pasta'd also isn't super

+
     browserProxy.setSafeBrowsingExtendedReportingEnabled(enabled);
   },
 
   /** @param {boolean} enabled Whether reporting is enabled or not.
     * @private
     */
   setSafeBrowsingExtendedReporting_: function(enabled) {
     this.safeBrowsingExtendedReportingEnabled_ = enabled;
   },
 
@@ skipping 20 matching lines @@
    * Works like a ternary operator. E.g. (value ? trueLabel: falseLabel).
    * @param {boolean} value
    * @param {string} trueLabel True label (for example, 'Allow DRM').
    * @param {string} falseLabel False label (for example, 'Blocked').
    * @private
    */
   getStringTernary_: function(value, trueLabel, falseLabel) {
     return value ? trueLabel : falseLabel;
   },
 });