Use kMetricsReportingEnabled instead of kStatsReporingPref on metrics side

chrome/browser/ui/webui/options/browser_options_handler.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/webui/options/browser_options_handler.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
@@ skipping 551 matching lines @@
 
   values->SetString("username", username);
 #endif
   // Pass along sync status early so it will be available during page init.
   values->Set("syncData", GetSyncStateDictionary().release());
 
   values->SetString("privacyLearnMoreURL", chrome::kPrivacyLearnMoreURL);
 
   values->SetString("doNotTrackLearnMoreURL", chrome::kDoNotTrackLearnMoreURL);
 
-#if !defined(OS_CHROMEOS)
   values->SetBoolean(
       "metricsReportingEnabledAtStart",
       ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled());
-#endif
 
 #if defined(OS_CHROMEOS)
   // TODO(pastarmovj): replace this with a call to the CrosSettings list
   // handling functionality to come.
   values->Set("timezoneList", chromeos::system::GetTimezoneList().release());
 
   values->SetString("accessibilityLearnMoreURL",
                     chrome::kChromeAccessibilityHelpURL);
 
   std::string settings_url = std::string("chrome-extension://") +
@@ skipping 1553 matching lines @@
   extension = extensions::GetExtensionOverridingProxy(
       Profile::FromWebUI(web_ui()));
   AppendExtensionData("proxy", extension, &extension_controlled);
 
   web_ui()->CallJavascriptFunction("BrowserOptions.toggleExtensionIndicators",
                                    extension_controlled);
 #endif  // defined(OS_WIN)
 }
 
 void BrowserOptionsHandler::SetupMetricsReportingCheckbox() {
-  // This function does not work for ChromeOS and non-official builds.
-#if !defined(OS_CHROMEOS) && defined(GOOGLE_CHROME_BUILD)
+// As the metrics and crash reporting checkbox only exists for official builds
+// it doesn't need to be set up for non-official builds.
+#if defined(GOOGLE_CHROME_BUILD)
   bool checked =
       ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled();
-  bool disabled = !IsMetricsReportingUserChangable();
-
-  SetMetricsReportingCheckbox(checked, disabled);
-#endif
+  bool policy_managed = IsMetricsReportingPolicyManaged();
+  bool owner_managed = false;
+#if defined(OS_CHROMEOS)
+  owner_managed = !IsDeviceOwnerProfile();
+#endif  // defined(OS_CHROMEOS)
+  SetMetricsReportingCheckbox(checked, policy_managed, owner_managed);
+#endif  // defined(GOOGLE_CHROME_BUILD)
 }
 
 void BrowserOptionsHandler::HandleMetricsReportingChange(
     const base::ListValue* args) {
   bool enable;
   if (!args->GetBoolean(0, &enable))
     return;
+  // Decline the change if current user shouldn't be able to change metrics
+  // reporting.
+  if (!IsDeviceOwnerProfile() || IsMetricsReportingPolicyManaged()) {
+    NotifyUIOfMetricsReportingChange(
+        ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled());
+    return;
+  }
 
+#if defined(OS_CHROMEOS)
+  chromeos::CrosSettings::Get()->SetBoolean(chromeos::kStatsReportingPref,
+                                            enable);
+#endif  // defined(OS_CHROMEOS)
   InitiateMetricsReportingChange(

[Alexei Svitkine (slow), 2015/11/03 23:18:23]

So now, how this works is that this will be called twice on CrOS - once because
of the observer and a second time here (which is needed for the callback),
right?

And the idea is that this is OK because the second call should be a no-op
because the pref hasn't changed at that point, right?

If the above is correct, please add a comment explaining this subtle behavior.

Probably worth adding a TODO to clean this up as well. I'm imagining (not in
this CL) to have the metrics code to allow registering an observer on the
change. Then, it wouldn't be necessary to pass the callback to the function
below - since the options handler can just have an observer in general. (So add
a TODO for this please.)

[gayane -on leave until 09-2017, 2015/11/05 18:53:32]

You are right InitiateMetricsReportingChange will be called twice for Chrome OS
but it should be fine. I added a TODO to clean this up.  

       enable,
-      base::Bind(&BrowserOptionsHandler::MetricsReportingChangeCallback,
+      base::Bind(&BrowserOptionsHandler::NotifyUIOfMetricsReportingChange,
                  base::Unretained(this)));
 }
 
-void BrowserOptionsHandler::MetricsReportingChangeCallback(bool enabled) {
-  SetMetricsReportingCheckbox(enabled, !IsMetricsReportingUserChangable());
+void BrowserOptionsHandler::NotifyUIOfMetricsReportingChange(bool enabled) {
+  SetMetricsReportingCheckbox(enabled, IsMetricsReportingPolicyManaged(),
+                              !IsDeviceOwnerProfile());
 }
 
 void BrowserOptionsHandler::SetMetricsReportingCheckbox(bool checked,
-                                                        bool disabled) {
+                                                        bool policy_managed,
+                                                        bool owner_managed) {
   web_ui()->CallJavascriptFunction(
       "BrowserOptions.setMetricsReportingCheckboxState",
-      base::FundamentalValue(checked),
-      base::FundamentalValue(disabled));
+      base::FundamentalValue(checked), base::FundamentalValue(policy_managed),
+      base::FundamentalValue(owner_managed));
 }
 
 void BrowserOptionsHandler::OnPolicyUpdated(const policy::PolicyNamespace& ns,
                                             const policy::PolicyMap& previous,
                                             const policy::PolicyMap& current) {
   std::set<std::string> different_keys;
   current.GetDifferingKeys(previous, &different_keys);
   if (ContainsKey(different_keys, policy::key::kMetricsReportingEnabled))
     SetupMetricsReportingCheckbox();
 }
 
+bool BrowserOptionsHandler::IsDeviceOwnerProfile() {
+#if defined(OS_CHROMEOS)
+  return chromeos::ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
+#else
+  return true;
+#endif
+}
+
 }  // namespace options