Consolidates accessing and setting the UMA pref to be within metrics code

chrome/browser/ui/webui/options/core_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/core_options_handler.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/json/json_reader.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 17 matching lines @@
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 using base::UserMetricsAction;
 
 namespace options {
 
 namespace {
 
-// Only allow changes to the metrics reporting checkbox if we were succesfully
-// able to change the service.
-bool AllowMetricsReportingChange(const base::Value* to_value) {
-  bool enable;
-  if (!to_value->GetAsBoolean(&enable)) {
-    NOTREACHED();
-    return false;
-  }
-
-  return enable == ResolveMetricsReportingEnabled(enable);
-}
-
 // Whether "controlledBy" property of pref value sent to options web UI needs to
 // be set to "extension" when the preference is controlled by an extension.
 bool CanSetExtensionControlledPrefValue(
     const PrefService::Preference* preference) {
 #if defined(OS_WIN)
   // These have more obvious UI than the standard one for extension controlled
   // values (an extension puzzle piece) on the settings page. To avoiding
   // showing the extension puzzle piece for these settings, their "controlledBy"
   // value should never be set to "extension".
   return preference->name() != prefs::kURLsToRestoreOnStartup &&
@@ skipping 14 matching lines @@
 CoreOptionsHandler::~CoreOptionsHandler() {}
 
 void CoreOptionsHandler::InitializeHandler() {
   Profile* profile = Profile::FromWebUI(web_ui());
 
   plugin_status_pref_setter_.Init(
       profile,
       base::Bind(&CoreOptionsHandler::OnPreferenceChanged,
                  base::Unretained(this),
                  profile->GetPrefs()));
-
-  pref_change_filters_[prefs::kMetricsReportingEnabled] =
-      base::Bind(&AllowMetricsReportingChange);
 }
 
 void CoreOptionsHandler::InitializePage() {
   UpdateClearPluginLSOData();
   UpdatePepperFlashSettingsEnabled();
 }
 
 void CoreOptionsHandler::GetLocalizedValues(
     base::DictionaryValue* localized_strings) {
   GetStaticLocalizedValues(localized_strings);
@@ skipping 126 matching lines @@
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback("clearPref",
       base::Bind(&CoreOptionsHandler::HandleClearPref,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback("coreOptionsUserMetricsAction",
       base::Bind(&CoreOptionsHandler::HandleUserMetricsAction,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback("disableExtension",
       base::Bind(&CoreOptionsHandler::HandleDisableExtension,
                  base::Unretained(this)));
+  web_ui()->RegisterMessageCallback("coreOptionsMetricsReportingChange",

[Alexei Svitkine (slow), 2014/09/05 15:22:48]

I know the previous code was in this file, but according to the header comment:

"// Handles resource and JS calls common to all options sub-pages."

So it sounds like a base class used by different pages. It's not clear to me
that this stuff belongs in the base class.

Can you check if there's a more appropriate place for it to live? Maybe
browser_options_handler.cc?

[gayane -on leave until 09-2017, 2014/09/05 18:30:20]

That actually makes sense because other functions that checkbox code in JS works
with are in browser_options_handler


On 2014/09/05 15:22:48, Alexei Svitkine wrote:

+      base::Bind(&CoreOptionsHandler::HandleMetricsReportingChange,
+                 base::Unretained(this)));
 }
 
 void CoreOptionsHandler::HandleInitialize(const base::ListValue* args) {
   DCHECK(handlers_host_);
   handlers_host_->InitializeHandlers();
 }
 
 void CoreOptionsHandler::OnFinishedLoading(const base::ListValue* args) {
   DCHECK(handlers_host_);
   handlers_host_->OnFinishedLoading();
@@ skipping 385 matching lines @@
     ExtensionService* extension_service = extensions::ExtensionSystem::Get(
         Profile::FromWebUI(web_ui()))->extension_service();
     DCHECK(extension_service);
     extension_service->DisableExtension(
         extension_id, extensions::Extension::DISABLE_USER_ACTION);
   } else {
     NOTREACHED();
   }
 }
 
+void CoreOptionsHandler::HandleMetricsReportingChange(
+    const base::ListValue* args) {
+  base::Callback<void(bool)> callback_fn = base::Bind(
+      &CoreOptionsHandler::MetricsReportingChangeCallback,
+      base::Unretained(this));
+  bool enable;
+  if (!args->GetBoolean(0, &enable))  return;

[Alexei Svitkine (slow), 2014/09/05 15:22:48]

Nit: Put return on the next line.

[gayane -on leave until 09-2017, 2014/09/05 18:30:20]

Done.

+
+  InitiateMetricsReportingChange(enable, callback_fn);
+}
+
+void CoreOptionsHandler::MetricsReportingChangeCallback(bool success) {
+  base::DictionaryValue* dict = new base::DictionaryValue;

[Alexei Svitkine (slow), 2014/09/05 15:22:48]

I think you might be leaking this.

Please double check what CallJavascriptFunction does, but I suspect it makes a
copy, in which case the object you allocated here will never be freed.

[gayane -on leave until 09-2017, 2014/09/05 18:30:20]

param removed. not needed anymore.

+  dict->SetBoolean("success", success);
+  web_ui()->CallJavascriptFunction(
+      "BrowserOptions.setMetricsReportingJSCallback", *dict);
+}
+
 void CoreOptionsHandler::UpdateClearPluginLSOData() {
   base::FundamentalValue enabled(
           plugin_status_pref_setter_.IsClearPluginLSODataEnabled());
   web_ui()->CallJavascriptFunction(
       "OptionsPage.setClearPluginLSODataEnabled", enabled);
 }
 
 void CoreOptionsHandler::UpdatePepperFlashSettingsEnabled() {
   base::FundamentalValue enabled(
           plugin_status_pref_setter_.IsPepperFlashSettingsEnabled());
   web_ui()->CallJavascriptFunction(
       "OptionsPage.setPepperFlashSettingsEnabled", enabled);
 }
 
 }  // namespace options