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..67d1bc504f5f532ce5b051743cb8407633cdf82c 100644
--- a/chrome/browser/resources/settings/privacy_page/privacy_page.js
+++ b/chrome/browser/resources/settings/privacy_page/privacy_page.js
@@ -113,9 +113,26 @@ Polymer({
 
 // <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);
   },
 
@@ -142,9 +159,23 @@ Polymer({
 // </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);
   },