Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
506663003:
Consolidates accessing and setting the UMA pref to be within metrics code (Closed)
|
Created:
6 years, 4 months ago by gayane -on leave until 09-2017 Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionConsolidates accessing and setting the UMA pref
to be within metrics code
MetricsReportingEnabled checkbox changed code refactored for not-chromeos:
-prefs removed from HTML
-Handler and callback functions added to core_options_handler
-Major refactoring in metrics_reporting_state
-Simple histogram added in metrics_reporting_state without distinguishing the source of the change
-browser_options_handler changed to handle new way of forcing company policies of disabling the checkbox
BUG=401233
NOPRESUBMIT=true
R=asvitkine@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/9f044063e9d771aae5995b6bb9894480e8c292e4
Patch Set 1 #
Total comments: 16
Patch Set 2 : access to prefs moved from browser_options_handle to metrics_reposting_state #
Total comments: 22
Patch Set 3 : Minor changes #
Total comments: 16
Patch Set 4 : more comments. using accesor class. #
Total comments: 26
Patch Set 5 : MetricsReporting handling moved to browser_options_handler. JS callback is not called if the… #
Total comments: 22
Patch Set 6 : Return updated state instead of success identifier + minor changes #
Total comments: 26
Patch Set 7 : Always pass all params to setMetricsReportingCheckboxState, minor changes #
Total comments: 14
Patch Set 8 : minor changes and fixes #
Total comments: 12
Patch Set 9 : fixes, more comments #
Total comments: 6
Patch Set 10 : forward declaration. minor fixes #Patch Set 11 : fix and rollback unnecessary changes #
Total comments: 12
Patch Set 12 : typedef for callback type #
Total comments: 9
Patch Set 13 : callback arguments as const refs instead of const #Patch Set 14 : inline cllback definition #Patch Set 15 : indent fix #Patch Set 16 : sync #Patch Set 17 : checkbox setup only for official build #Patch Set 18 : ChromeOS and Android support in IsMetricsReportingUserChangable #
Total comments: 8
Patch Set 19 : undo changes for IsMetricsReportingUserChangable. Checks for andrioid added #
Total comments: 1
Patch Set 20 : android check removed #Messages
Total messages: 59 (13 generated)
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
I tried to run trybots before submitting for a review. Some are failing but it seems unrelated.
I think this is on the right track, though I have some initial comments for you below. https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:648: &WizardController::InitiateMetricsReportingChangeCallback, Nit: Indent 2 more. https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:649: base::Unretained(this)); Pass a weak ptr via weak_ptr_factory_ instead of base::Unretained() here. https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:665: { Nit: Should go on previous line. https://codereview.chromium.org/506663003/diff/1/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_reporting_state.h:12: bool SetGoogleUpdateSettings(bool enabled); Functions that are only called internally by the .cc file shouldn't be included in the header and should entirely live in anon namespace at the top of the .cc file. https://codereview.chromium.org/506663003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:43: if (!success) Nit: {}'s https://codereview.chromium.org/506663003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:45: !$('metricsReportingEnabled').checked; Nit: Indent 2 more. https://codereview.chromium.org/506663003/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/browser_options_handler.cc:881: prefs::kMetricsReportingEnabled, This code shouldn't be registering this pref. That's the responsibility of (and already done in) MetricsStateManager. https://codereview.chromium.org/506663003/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/browser_options_handler.cc:1881: prefs::kMetricsReportingEnabled)); This part should be done in chrome/browser/metrics and queried via an API, so this code doesn't use the pref directly.
https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:648: &WizardController::InitiateMetricsReportingChangeCallback, On 2014/08/28 14:46:48, Alexei Svitkine wrote: > Nit: Indent 2 more. Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:649: base::Unretained(this)); On 2014/08/28 14:46:48, Alexei Svitkine wrote: > Pass a weak ptr via weak_ptr_factory_ instead of base::Unretained() here. Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/wizard_controller.cc:665: { On 2014/08/28 14:46:48, Alexei Svitkine wrote: > Nit: Should go on previous line. Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/metrics/metri... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/metrics/metri... chrome/browser/metrics/metrics_reporting_state.h:12: bool SetGoogleUpdateSettings(bool enabled); On 2014/08/28 14:46:48, Alexei Svitkine wrote: > Functions that are only called internally by the .cc file shouldn't be included > in the header and should entirely live in anon namespace at the top of the .cc > file. Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:43: if (!success) On 2014/08/28 14:46:48, Alexei Svitkine wrote: > Nit: {}'s Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:45: !$('metricsReportingEnabled').checked; On 2014/08/28 14:46:48, Alexei Svitkine wrote: > Nit: Indent 2 more. Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/browser_options_handler.cc:881: prefs::kMetricsReportingEnabled, On 2014/08/28 14:46:49, Alexei Svitkine wrote: > This code shouldn't be registering this pref. That's the responsibility of (and > already done in) MetricsStateManager. Done. https://codereview.chromium.org/506663003/diff/1/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/browser_options_handler.cc:1881: prefs::kMetricsReportingEnabled)); On 2014/08/28 14:46:48, Alexei Svitkine wrote: > This part should be done in chrome/browser/metrics and queried via an API, so > this code doesn't use the pref directly. Done.
https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:630: if (success) { Nit: Prefer an early return. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:851: base::Callback<void(bool)> callback_fn; Nit: Just inline this into the call. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:18: namespace { Nit: Add a blank line after this. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:41: // Does the necessary changes for MetricsReportingEnabled changes which needs Nit: Add a blank line before this. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:44: bool enabled, bool success) { Nit: 1 param per line. Align params. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:57: prefs::kMetricsReportingEnabled, enabled); Nit: Indent 2 more. But also, I think before this is reached, there should be a check somewhere (in this file) to ensure IsMetricsReportingUserChangable() is true. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:65: } // namespace Nit: Add a blank line before this. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:68: base::Callback<void(bool)> callback_fn, bool enabled) { Nit: 1 param per line. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:69: content::BrowserThread::PostTaskAndReplyWithResult( Add a comment why this is posted to the FILE thread. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:12: // Calls |SetGoogleUpdateSettings| function in FILE thread with a callback This is describing an implementation detail. Instead, have the comment describe the purpose of this function. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:15: base::Callback<void(bool)> callback_fn, bool enabled); Nit: Make the callback the last param.
https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:630: if (success) { On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Prefer an early return. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:851: base::Callback<void(bool)> callback_fn; On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Just inline this into the call. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:18: namespace { On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Add a blank line after this. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:41: // Does the necessary changes for MetricsReportingEnabled changes which needs On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Add a blank line before this. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:44: bool enabled, bool success) { On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: 1 param per line. Align params. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:57: prefs::kMetricsReportingEnabled, enabled); Added a check in InitiateMetricsReportingChange On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Indent 2 more. > > But also, I think before this is reached, there should be a check somewhere (in > this file) to ensure IsMetricsReportingUserChangable() is true. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:65: } // namespace On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Add a blank line before this. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:68: base::Callback<void(bool)> callback_fn, bool enabled) { On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: 1 param per line. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:69: content::BrowserThread::PostTaskAndReplyWithResult( On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Add a comment why this is posted to the FILE thread. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:12: // Calls |SetGoogleUpdateSettings| function in FILE thread with a callback On 2014/09/04 13:58:03, Alexei Svitkine wrote: > This is describing an implementation detail. Instead, have the comment describe > the purpose of this function. Done. https://codereview.chromium.org/506663003/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:15: base::Callback<void(bool)> callback_fn, bool enabled); On 2014/09/04 13:58:03, Alexei Svitkine wrote: > Nit: Make the callback the last param. Done.
https://codereview.chromium.org/506663003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:630: if (!success) { Nit: No need for {}'s for single line body. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:62: RecordMetricsReportingHistogramValue(enabled? METRICS_REPORTING_ENABLED Nit: Spaces around : and ?. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:63: :METRICS_REPORTING_DISABLED); Nit: Put ":" on previous line and align the two constants. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:73: if (!IsMetricsReportingUserChangable() && !callback_fn.is_null()) { If the pref is changeable, we should do an early return even if callback is null. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:94: const PrefService::Preference* pref = pref_service->FindPreference( Nit: Wrap after = instead. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:18: bool IsMetricsReportingEnabled(); Hmm, why not just use the version from ChromeMetricsServiceAccessor instead of having another function? https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:641: if (args->GetBoolean(0, &enable)) { Nit: prefer an early return. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/core_options_handler.h (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.h:163: void HandleMetricsReportingChange(const base::ListValue* args); Add comments for each method.
https://codereview.chromium.org/506663003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:630: if (!success) { On 2014/09/04 18:45:25, Alexei Svitkine wrote: > Nit: No need for {}'s for single line body. Done. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:62: RecordMetricsReportingHistogramValue(enabled? METRICS_REPORTING_ENABLED On 2014/09/04 18:45:26, Alexei Svitkine wrote: > Nit: Spaces around : and ?. Done. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:63: :METRICS_REPORTING_DISABLED); On 2014/09/04 18:45:26, Alexei Svitkine wrote: > Nit: Put ":" on previous line and align the two constants. Done. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:73: if (!IsMetricsReportingUserChangable() && !callback_fn.is_null()) { On 2014/09/04 18:45:26, Alexei Svitkine wrote: > If the pref is changeable, we should do an early return even if callback is > null. Done. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:94: const PrefService::Preference* pref = pref_service->FindPreference( On 2014/09/04 18:45:26, Alexei Svitkine wrote: > Nit: Wrap after = instead. Done. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:18: bool IsMetricsReportingEnabled(); On 2014/09/04 18:45:26, Alexei Svitkine wrote: > Hmm, why not just use the version from ChromeMetricsServiceAccessor instead of > having another function? Do you want to move IsMetricsReportingUserChangable to Accessor? https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:641: if (args->GetBoolean(0, &enable)) { On 2014/09/04 18:45:26, Alexei Svitkine wrote: > Nit: prefer an early return. Done. https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/core_options_handler.h (right): https://codereview.chromium.org/506663003/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.h:163: void HandleMetricsReportingChange(const base::ListValue* args); On 2014/09/04 18:45:26, Alexei Svitkine wrote: > Add comments for each method. Done.
https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:630: if (!success) return; Nit: Put return on the next line. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:851: InitiateMetricsReportingChange(new_value, base::Callback<void(bool)>()); Is this correct? This is from CrOS code, but I thought the function currently doesn't handle CrOS? Also, this code has a bunch of TODOs - can you check with the author to see if it might make sense to "Remove this" per the comment? https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:22: METRICS_REPORTING_DISABLED, I'd put this entry first. I expect over time for us to expand this histogram to break down the different ways the user can enable UMA, while I expect the disable option will always be just one, so better keep it first in the list. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:29: "MetricsReporting", value, METRICS_REPORTING_MAX); Histograms are generally named of the form Foo.Bar. In this case, I'd use the prefix "UMA." since its about the core UMA system. You also need to modify histograms.xml to document the histogram. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:13: // thread with IO access. Please document the params. Also, please document which platforms this currently doesn't work on with a TODO. (Android and CrOS) https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:17: // Returns weather MetricsReporting can be modified by user. Nit: "by the user." https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:41: BrowserOptions.setMetricsReportingJSCallback = function(success) { Please add a comment explaining this function and what the param means. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:43: if (!success) { If there's nothing to do when !success, how about not calling into JS at all in that case? https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:406: chrome.send('coreOptionsMetricsReportingChange', I'd name this something more meaningful than coreOptionsMetricsReportingChange. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (left): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:878: Nit: Put this blank line back to avoid the extra diff. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:227: web_ui()->RegisterMessageCallback("coreOptionsMetricsReportingChange", 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? https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:641: if (!args->GetBoolean(0, &enable)) return; Nit: Put return on the next line. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:647: base::DictionaryValue* dict = new base::DictionaryValue; 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.
https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:630: if (!success) return; On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Nit: Put return on the next line. Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/settings/device_settings_provider.cc:851: InitiateMetricsReportingChange(new_value, base::Callback<void(bool)>()); Actually InitiateMetricsReportingChange function would do the same thing for ChromeOS as before except that it would run on the FILE thread. For the TODOs I have contacted the author. On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Is this correct? > > This is from CrOS code, but I thought the function currently doesn't handle > CrOS? > > Also, this code has a bunch of TODOs - can you check with the author to see if > it might make sense to "Remove this" per the comment? https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:22: METRICS_REPORTING_DISABLED, On 2014/09/05 15:22:48, Alexei Svitkine wrote: > I'd put this entry first. > > I expect over time for us to expand this histogram to break down the different > ways the user can enable UMA, while I expect the disable option will always be > just one, so better keep it first in the list. Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:29: "MetricsReporting", value, METRICS_REPORTING_MAX); On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Histograms are generally named of the form Foo.Bar. > > In this case, I'd use the prefix "UMA." since its about the core UMA system. > > You also need to modify histograms.xml to document the histogram. Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:13: // thread with IO access. On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Please document the params. > > Also, please document which platforms this currently doesn't work on with a > TODO. (Android and CrOS) Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:17: // Returns weather MetricsReporting can be modified by user. On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Nit: "by the user." Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:41: BrowserOptions.setMetricsReportingJSCallback = function(success) { On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Please add a comment explaining this function and what the param means. Description for the function added, but param removed. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:43: if (!success) { On 2014/09/05 15:22:48, Alexei Svitkine wrote: > If there's nothing to do when !success, how about not calling into JS at all in > that case? Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:406: chrome.send('coreOptionsMetricsReportingChange', On 2014/09/05 15:22:48, Alexei Svitkine wrote: > I'd name this something more meaningful than coreOptionsMetricsReportingChange. changed to metricsReportingCheckboxChanged https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (left): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:878: On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Nit: Put this blank line back to avoid the extra diff. Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:227: web_ui()->RegisterMessageCallback("coreOptionsMetricsReportingChange", 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: > 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? https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:641: if (!args->GetBoolean(0, &enable)) return; On 2014/09/05 15:22:48, Alexei Svitkine wrote: > Nit: Put return on the next line. Done. https://codereview.chromium.org/506663003/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/core_options_handler.cc:647: base::DictionaryValue* dict = new base::DictionaryValue; On 2014/09/05 15:22:48, Alexei Svitkine wrote: > 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. param removed. not needed anymore.
https://codereview.chromium.org/506663003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:635: if (usage_statistics_reporting_) { This seems to be a subtle change in the previous code. I'd keep the logic exactly the same, since you're not intending to change this. I believe the identical logic should be (but please verify to make sure I didn't make a mistake): bool uma_enabled = (usage_statistics_reporting_ == success); CrosSettings::Get()->SetBoolean(kStatsReportingPref, uma_enabled); if (uma_enabled) { ... } Although, I'm now thinking that it might be better to have the result of the callback be changed from "success" to "uma_enabled", which makes it so the above tricky logic wouldn't be necessary. WDYT? https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:11: #include "chrome/browser/chromeos/settings/cros_settings.h" Do you need the CrOS includes (this one and below) with your current change? https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:38: DVLOG(1) << "Unable to set crash report status to " << enabled; Nit: Update this message while you're changing this. "crash report" -> "metrics reporting". Also, I suggest adding another enum entry to the histogram to indicate this error and logging it. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:46: base::Callback<void(bool)> callback_fn, Nit: pass the callback by const ref. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:72: base::Callback<void(bool)> callback_fn) { Nit: pass the callback by const ref. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:79: // Posts on FILE thread as SetGoogleUpdateSettings modifies the local state. This comment is incorrect. "Local State" in fact needs to be modified on the UI thread (a background task actually does the write to disk) and you're doing that in the correct place. The other function does IO, which is why it needs to be done on the file thread. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:87: bool IsMetricsReportingUserChangable() { This isn't correct for CrOS or Android. 1. We shouldn't use it above for those platforms. 2. We should document this in the .h file. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:14: // MetricsReportingEnabled should be true or false and |callback_fn| is a How about this wording which also talks about metrics service: // Initiates a change to metrics reporting state to the new value of |enabled|. // Starts or stops the metrics service based on the new state and then runs // |callback_fn| (which can be null) with the updated state (as the operation // may fail). On platforms other than CrOS and Android, also updates the // underlying pref. // TODO(gayane): Support setting the pref on all platforms. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:17: // TODO(gayane) Currently this function doesn't change the preference for CrOS. Mention that it doesn't support Android either. (But this is OK since Android doesn't call this since it has its own settings page.) Also, add a : after the TODO(gayane) (This is addressed in my suggestion above for the wording.) https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1886: base::Callback<void(bool)> callback_fn = base::Bind( Nit: Move this to be right above where it's used. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.h:333: // Passes forward the request to change MetricsReportingEnabled. Nit: I suggest using a comment similar to the one for VirtualKeyboardChangeCallback() function above.
https://codereview.chromium.org/506663003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/wizard_controller.cc:635: if (usage_statistics_reporting_) { On 2014/09/05 20:50:24, Alexei Svitkine wrote: > This seems to be a subtle change in the previous code. I'd keep the logic > exactly the same, since you're not intending to change this. > > I believe the identical logic should be (but please verify to make sure I didn't > make a mistake): > > bool uma_enabled = (usage_statistics_reporting_ == success); > > CrosSettings::Get()->SetBoolean(kStatsReportingPref, uma_enabled); > if (uma_enabled) { > ... > } > > Although, I'm now thinking that it might be better to have the result of the > callback be changed from "success" to "uma_enabled", which makes it so the above > tricky logic wouldn't be necessary. WDYT? Done. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:11: #include "chrome/browser/chromeos/settings/cros_settings.h" On 2014/09/05 20:50:24, Alexei Svitkine wrote: > Do you need the CrOS includes (this one and below) with your current change? Done. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:38: DVLOG(1) << "Unable to set crash report status to " << enabled; On 2014/09/05 20:50:24, Alexei Svitkine wrote: > Nit: Update this message while you're changing this. "crash report" -> "metrics > reporting". > > Also, I suggest adding another enum entry to the histogram to indicate this > error and logging it. Done. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:46: base::Callback<void(bool)> callback_fn, On 2014/09/05 20:50:24, Alexei Svitkine wrote: > Nit: pass the callback by const ref. Done. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:72: base::Callback<void(bool)> callback_fn) { On 2014/09/05 20:50:24, Alexei Svitkine wrote: > Nit: pass the callback by const ref. Done. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:79: // Posts on FILE thread as SetGoogleUpdateSettings modifies the local state. Done. On 2014/09/05 20:50:24, Alexei Svitkine wrote: > This comment is incorrect. "Local State" in fact needs to be modified on the UI > thread (a background task actually does the write to disk) and you're doing that > in the correct place. The other function does IO, which is why it needs to be > done on the file thread. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.cc:87: bool IsMetricsReportingUserChangable() { - Comment added in the .h file for this function and the one which is calling it. - The call of the function which calls this one (SetupMetricsReportingCheckbox() in BrowserOptionHandler) moved under #if !defined(OS_CHROMEOS). On 2014/09/05 20:50:24, Alexei Svitkine wrote: > This isn't correct for CrOS or Android. > > 1. We shouldn't use it above for those platforms. > 2. We should document this in the .h file. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:14: // MetricsReportingEnabled should be true or false and |callback_fn| is a Done. On 2014/09/05 20:50:25, Alexei Svitkine wrote: > How about this wording which also talks about metrics service: > > // Initiates a change to metrics reporting state to the new value of |enabled|. > // Starts or stops the metrics service based on the new state and then runs > // |callback_fn| (which can be null) with the updated state (as the operation > // may fail). On platforms other than CrOS and Android, also updates the > // underlying pref. > // TODO(gayane): Support setting the pref on all platforms. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_reporting_state.h:17: // TODO(gayane) Currently this function doesn't change the preference for CrOS. On 2014/09/05 20:50:25, Alexei Svitkine wrote: > Mention that it doesn't support Android either. (But this is OK since Android > doesn't call this since it has its own settings page.) > > Also, add a : after the TODO(gayane) > > (This is addressed in my suggestion above for the wording.) Acknowledged. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:1886: base::Callback<void(bool)> callback_fn = base::Bind( On 2014/09/05 20:50:25, Alexei Svitkine wrote: > Nit: Move this to be right above where it's used. Done. https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/506663003/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.h:333: // Passes forward the request to change MetricsReportingEnabled. On 2014/09/05 20:50:25, Alexei Svitkine wrote: > Nit: I suggest using a comment similar to the one for > VirtualKeyboardChangeCallback() function above. Done.
https://codereview.chromium.org/506663003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.h:218: // Callback function after setting MetricsReporting Nit: Full sentences (add a .) https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:64: else { Nit: Put on same line as } https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:74: bool enabled, const base::Callback<void(bool)> callback_fn) { If you're wrapping the first param, then it should be 1 param per line. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:81: // Posts on FILE thread as SetGoogleUpdateSettings does IO operations. Nit: "Posts on" -> "Posts to". https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:9: #include "components/metrics/metrics_service.h" Is this include needed? https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:10: Nit: Remove extra blank line. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1526: if (typeof disabled !== 'undefined') { I would just require the callers to always pass this param and then remove this check. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1528: if (disabled) { Add a comment why it's OK to not handle the else of this if (i.e. that once it's disabled by policy, it wouldn't ever get re-enabled). https://codereview.chromium.org/506663003/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:938: SetupMetricsReportingCheckbox(); Put the ifdef inside the function itself with a comment and call it unconditionally from here. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1877: LOG(ERROR) << ChromeMetricsServiceAccessor::IsMetricsReportingEnabled(); Remove extra log message. https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34895: +<histogram name="UMA.MetricsReporting" enum="MetricsReportingChange"> I'd use use a name that suggests the action the user is doing, i.e. UMA.MetricsReporting.Toggle https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34898: + Log whether MetricsReporting is enabled. I would expand this description and make it clearer. Logged when the user attempts to toggle the metrics reporting state and whether the action succeeded. https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51608: + <int value="1" label="MetricsReporting is disabled"/> Nit: I'd use shorter strings here (e.g. Error, Disabled successfully, Enabled successfully). You can provide more info within the tags (i.e. <int value="0" label="...">More info...</int>) if you want.
I also suggest expanding the CL description to have a short paragraph about the motivation for this change (i.e. consolidates accessing and setting the UMA pref to be within metrics code) in addition to your current bullet points about what's being changed.
https://codereview.chromium.org/506663003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.h (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.h:218: // Callback function after setting MetricsReporting On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Nit: Full sentences (add a .) Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:64: else { On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Nit: Put on same line as } Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:74: bool enabled, const base::Callback<void(bool)> callback_fn) { On 2014/09/09 15:27:15, Alexei Svitkine wrote: > If you're wrapping the first param, then it should be 1 param per line. Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:81: // Posts on FILE thread as SetGoogleUpdateSettings does IO operations. On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Nit: "Posts on" -> "Posts to". Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:9: #include "components/metrics/metrics_service.h" On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Is this include needed? Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:10: On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Nit: Remove extra blank line. Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1526: if (typeof disabled !== 'undefined') { On 2014/09/09 15:27:15, Alexei Svitkine wrote: > I would just require the callers to always pass this param and then remove this > check. Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1528: if (disabled) { On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Add a comment why it's OK to not handle the else of this if (i.e. that once it's > disabled by policy, it wouldn't ever get re-enabled). Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:938: SetupMetricsReportingCheckbox(); On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Put the ifdef inside the function itself with a comment and call it > unconditionally from here. Done. https://codereview.chromium.org/506663003/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1877: LOG(ERROR) << ChromeMetricsServiceAccessor::IsMetricsReportingEnabled(); On 2014/09/09 15:27:15, Alexei Svitkine wrote: > Remove extra log message. Done. https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34895: +<histogram name="UMA.MetricsReporting" enum="MetricsReportingChange"> On 2014/09/09 15:27:16, Alexei Svitkine wrote: > I'd use use a name that suggests the action the user is doing, i.e. > UMA.MetricsReporting.Toggle Done. https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34898: + Log whether MetricsReporting is enabled. On 2014/09/09 15:27:16, Alexei Svitkine wrote: > I would expand this description and make it clearer. > > Logged when the user attempts to toggle the metrics reporting state and whether > the action succeeded. Done. https://codereview.chromium.org/506663003/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51608: + <int value="1" label="MetricsReporting is disabled"/> On 2014/09/09 15:27:16, Alexei Svitkine wrote: > Nit: I'd use shorter strings here (e.g. Error, Disabled successfully, Enabled > successfully). You can provide more info within the tags (i.e. <int value="0" > label="...">More info...</int>) if you want. Done.
Almost there, a few more comments and then we can add owners to the reviewers list. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:46: bool updated_pref) { Do we need both updated_pref and enabled params? I think we should just use the last param and remove the first one. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:77: callback_fn.Run(false); This isn't correct anymore since the value is now the uma_enabled state, not success. Please fix. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1528: // if checkbox gets disabled then add a attribute for displaying the Nit: Capitalize; also, "a attribute" -> "an attribute" https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:937: SetupMetricsReportingCheckbox(); Move this to be right after the SetupMetricsReportingSettingVisibility() call above, since they're related. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1877: #if !defined(OS_CHROMEOS) Add a comment about why this isn't enabled for CrOS. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1903: base::FundamentalValue checkedValue(checked); Nit: This should use camel_case since it's on the C++ side. Or you can just inline them into the call as base::FundamentalValue(checked) and so forth. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.h:342: void SetMetricsReportingCheckbox(bool checked, bool disabled); Add a blank line after this.
https://codereview.chromium.org/506663003/diff/120001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:46: bool updated_pref) { On 2014/09/09 20:30:04, Alexei Svitkine wrote: > Do we need both updated_pref and enabled params? > > I think we should just use the last param and remove the first one. We need it for understanding the success of the change to record to histogram. Other option would be to record the error in the previous function and here record whatever is the new value. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:77: callback_fn.Run(false); On 2014/09/09 20:30:05, Alexei Svitkine wrote: > This isn't correct anymore since the value is now the uma_enabled state, not > success. Please fix. Done. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1528: // if checkbox gets disabled then add a attribute for displaying the On 2014/09/09 20:30:05, Alexei Svitkine wrote: > Nit: Capitalize; also, "a attribute" -> "an attribute" Done. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:937: SetupMetricsReportingCheckbox(); On 2014/09/09 20:30:05, Alexei Svitkine wrote: > Move this to be right after the SetupMetricsReportingSettingVisibility() call > above, since they're related. Done. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1877: #if !defined(OS_CHROMEOS) On 2014/09/09 20:30:05, Alexei Svitkine wrote: > Add a comment about why this isn't enabled for CrOS. Done. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1903: base::FundamentalValue checkedValue(checked); On 2014/09/09 20:30:05, Alexei Svitkine wrote: > Nit: This should use camel_case since it's on the C++ side. > > Or you can just inline them into the call as base::FundamentalValue(checked) and > so forth. Done. https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/506663003/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.h:342: void SetMetricsReportingCheckbox(bool checked, bool disabled); On 2014/09/09 20:30:05, Alexei Svitkine wrote: > Add a blank line after this. Done.
Haven't taken a look yet, but one tip for future: It's generally better to upload a new patchset after you do a sync but before you address comments. That way, the Codereview view that shows diffs between two different patches will exclude unrelated changes that were picked up by the sync. No worries this time, just for the future.
Almost there, just a few last comments. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:45: void SetMetricsReporting(bool enabled, If you're keeping both params, please document somewhere the difference. And/or rename the params to make that clearer. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:59: prefs::kMetricsReportingEnabled, enabled); This should be using updated_pref. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:77: if (!callback_fn.is_null()) Nit: Add {}'s since body is multi-line now. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:17: base::Callback<void(bool)> callback_fn); Pass the callback by const ref. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1524: // if checkbox gets disabled then add an attribute for displaying the Nit: Capitalize if. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/core_options_handler.cc:9: #include "base/callback.h" Remove this line, since you're only now deleting code from this file.
https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:45: void SetMetricsReporting(bool enabled, On 2014/09/11 15:33:43, Alexei Svitkine wrote: > If you're keeping both params, please document somewhere the difference. And/or > rename the params to make that clearer. Done. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:59: prefs::kMetricsReportingEnabled, enabled); On 2014/09/11 15:33:43, Alexei Svitkine wrote: > This should be using updated_pref. Done. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:77: if (!callback_fn.is_null()) On 2014/09/11 15:33:43, Alexei Svitkine wrote: > Nit: Add {}'s since body is multi-line now. Done. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:17: base::Callback<void(bool)> callback_fn); On 2014/09/11 15:33:43, Alexei Svitkine wrote: > Pass the callback by const ref. Done. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:1524: // if checkbox gets disabled then add an attribute for displaying the On 2014/09/11 15:33:43, Alexei Svitkine wrote: > Nit: Capitalize if. Done. https://codereview.chromium.org/506663003/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/core_options_handler.cc:9: #include "base/callback.h" On 2014/09/11 15:33:43, Alexei Svitkine wrote: > Remove this line, since you're only now deleting code from this file. Done.
LGTM % remaining comments. Once you've addressed them, you can add owners for review. (Let me know if you need help choosing owners, I can give you some tips.) https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_accessor.h (right): https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_accessor.h:14: #include "chrome/browser/ui/webui/options/browser_options_handler.h" Please forward declare the class instead. https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:14: #include "chromeos/settings/cros_settings_names.h" Is this header used? If not, remove. https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:67: RecordMetricsReportingHistogramValue(updated_pref ? METRICS_REPORTING_ENABLED : Nit: Make it fit within 80 chars/line.
https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_accessor.h (right): https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_accessor.h:14: #include "chrome/browser/ui/webui/options/browser_options_handler.h" On 2014/09/11 17:17:51, Alexei Svitkine wrote: > Please forward declare the class instead. Done. https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:14: #include "chromeos/settings/cros_settings_names.h" On 2014/09/11 17:17:51, Alexei Svitkine wrote: > Is this header used? If not, remove. Done. https://codereview.chromium.org/506663003/diff/150001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:67: RecordMetricsReportingHistogramValue(updated_pref ? METRICS_REPORTING_ENABLED : On 2014/09/11 17:17:51, Alexei Svitkine wrote: > Nit: Make it fit within 80 chars/line. Done.
gayane@chromium.org changed reviewers: + sky@chromium.org, stevenjb@chromium.org
stevenjb@chromium.org: Please review changes in chrome/browser/chromeos/ chrome/browser/resources/options/ chrome/browser/ui/webui/ sky@chromium.org: Please review changes in chrome/browser/ui/views/session_crashed_bubble_view.cc
chrome/browser/ui/views/session_crashed_bubble_view.cc LGTM
https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:589: weak_factory_.GetWeakPtr()); No need for local, just inline this. https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:602: if (enabled) { if() could be in #if, but even better, early exit: if (!enabled) return; https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:851: InitiateMetricsReportingChange(new_value, base::Callback<void(bool)>()); We should typedef the callback https://codereview.chromium.org/506663003/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:18: const base::Callback<void(bool)> callback_fn); Use callback typedef here also. https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... File components/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... components/metrics/metrics_state_manager.cc:222: base::ThreadRestrictions::ScopedAllowIO allow_io; We really shouldn't be using this in production code. We should really pass an IO thread to this method or to the constructor and use that.
https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... File components/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... components/metrics/metrics_state_manager.cc:222: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2014/09/11 20:31:44, stevenjb wrote: > We really shouldn't be using this in production code. We should really pass an > IO thread to this method or to the constructor and use that. Right. Previously we had this statement in metrics_reporting_state.cc which was also doing IO on UI thread. The code in that file is being fixed by this CL so that it doesn't do IO on UI thread, but unfortunately this bit of MetricsService still does (which was previously affected by the other ScopedAllowIO block but now narrowed down to just this function). So this ScopedAllowedIO is still needed until we fix the MetricsService code too, but that should be a separate CL.
https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... File components/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... components/metrics/metrics_state_manager.cc:222: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2014/09/11 20:39:43, Alexei Svitkine wrote: > On 2014/09/11 20:31:44, stevenjb wrote: > > We really shouldn't be using this in production code. We should really pass an > > IO thread to this method or to the constructor and use that. > > Right. Previously we had this statement in metrics_reporting_state.cc which was > also doing IO on UI thread. The code in that file is being fixed by this CL so > that it doesn't do IO on UI thread, but unfortunately this bit of MetricsService > still does (which was previously affected by the other ScopedAllowIO block but > now narrowed down to just this function). > > So this ScopedAllowedIO is still needed until we fix the MetricsService code > too, but that should be a separate CL. OK, that's fine, but please add a TODO: with the crbug issue# for fixing this, thanks!
https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:589: weak_factory_.GetWeakPtr()); On 2014/09/11 20:31:44, stevenjb wrote: > No need for local, just inline this. Done. https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:602: if (enabled) { On 2014/09/11 20:31:44, stevenjb wrote: > if() could be in #if, but even better, early exit: if (!enabled) return; Done. https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/506663003/diff/190001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:851: InitiateMetricsReportingChange(new_value, base::Callback<void(bool)>()); On 2014/09/11 20:31:44, stevenjb wrote: > We should typedef the callback Done. https://codereview.chromium.org/506663003/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:18: const base::Callback<void(bool)> callback_fn); On 2014/09/11 20:31:44, stevenjb wrote: > Use callback typedef here also. Done. https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... File components/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/506663003/diff/190001/components/metrics/metr... components/metrics/metrics_state_manager.cc:222: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2014/09/11 20:47:19, stevenjb wrote: > On 2014/09/11 20:39:43, Alexei Svitkine wrote: > > On 2014/09/11 20:31:44, stevenjb wrote: > > > We really shouldn't be using this in production code. We should really pass > an > > > IO thread to this method or to the constructor and use that. > > > > Right. Previously we had this statement in metrics_reporting_state.cc which > was > > also doing IO on UI thread. The code in that file is being fixed by this CL so > > that it doesn't do IO on UI thread, but unfortunately this bit of > MetricsService > > still does (which was previously affected by the other ScopedAllowIO block but > > now narrowed down to just this function). > > > > So this ScopedAllowedIO is still needed until we fix the MetricsService code > > too, but that should be a separate CL. > OK, that's fine, but please add a TODO: with the crbug issue# for fixing this, > thanks! > Done.
lgtm, couple more nits https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:20: const OnMetricsReportingCallbackType callback_fn); Nit: Pass this by const ref, not just const. Both here and in SetMetricsReporting. https://codereview.chromium.org/506663003/diff/210001/components/metrics/metr... File components/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/506663003/diff/210001/components/metrics/metr... components/metrics/metrics_state_manager.cc:221: // TODO(gayane): Eliminate use of ScopedAllowIO. crbug.com/13783 Nit: Put this comment inside the function above the allow_io line.
lgtm w/nits https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_accessor.h (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_accessor.h:58: bool, const OnMetricsReportingCallbackType); nit: const& https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:20: const OnMetricsReportingCallbackType callback_fn); On 2014/09/12 17:51:52, Alexei Svitkine wrote: > Nit: Pass this by const ref, not just const. Both here and in > SetMetricsReporting. +1 https://codereview.chromium.org/506663003/diff/210001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1897: base::Unretained(this)); nit: no need for local here
https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_accessor.h (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_accessor.h:58: bool, const OnMetricsReportingCallbackType); On 2014/09/12 18:07:41, stevenjb wrote: > nit: const& Done. https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.h (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.h:20: const OnMetricsReportingCallbackType callback_fn); On 2014/09/12 17:51:52, Alexei Svitkine wrote: > Nit: Pass this by const ref, not just const. Both here and in > SetMetricsReporting. Done. https://codereview.chromium.org/506663003/diff/210001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/210001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1897: base::Unretained(this)); On 2014/09/12 18:07:41, stevenjb wrote: > nit: no need for local here Done. https://codereview.chromium.org/506663003/diff/210001/components/metrics/metr... File components/metrics/metrics_state_manager.cc (right): https://codereview.chromium.org/506663003/diff/210001/components/metrics/metr... components/metrics/metrics_state_manager.cc:221: // TODO(gayane): Eliminate use of ScopedAllowIO. crbug.com/13783 On 2014/09/12 17:51:52, Alexei Svitkine wrote: > Nit: Put this comment inside the function above the allow_io line. Done.
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/506663003/270001
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/506663003/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/506663003/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I have made some changes in metrics_reporting_state to support Android and ChromeOS. Would you please have a look?
https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:105: #if defined(OS_ANDROID) Why is all of this needed? I thought this function only gets called on non-CrOS non-Android platforms currently (and your comment for this function mentions that it shouldn't be used on those). Is that not the case? (If so, can you clarify when it gets called on those other platforms). https://codereview.chromium.org/506663003/diff/330001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/330001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1897: // casesChromeOS a different preference is used. It is tied to HTML element. Nit: casesChromeOS? This comment isn't quite right. Unofficial builds just don't support metrics reporting, there's no other preference used.
https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:105: #if defined(OS_ANDROID) So the thing is that initially the plan was to use this function for Checkbox setup for non-Chrome non-Andriod, but then we added a check in InitiateMetricsReportingChange. This function can be called and is called from ChromeOS right now. I am not sure about Andrioid. I added it just in case. Changed the comment for this function. https://codereview.chromium.org/506663003/diff/330001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/330001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1897: // casesChromeOS a different preference is used. It is tied to HTML element. On 2014/09/15 20:59:43, Alexei Svitkine wrote: > Nit: casesChromeOS? > > This comment isn't quite right. Unofficial builds just don't support metrics > reporting, there's no other preference used. Done.
https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:62: #if !defined(OS_CHROMEOS) I would also expand this ifdef to be !ANDROID too. https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:105: #if defined(OS_ANDROID) On 2014/09/15 21:09:17, gayane wrote: > So the thing is that initially the plan was to use this function for Checkbox > setup for non-Chrome non-Andriod, but then we added a check in > InitiateMetricsReportingChange. This function can be called and is called from > ChromeOS right now. I am not sure about Andrioid. I added it just in case. > > Changed the comment for this function. I see. But InitiateMetricsReportingChange() doesn't touch the pref on those platforms yet. So I suggest just ifdefing the code in InitiateMetricsReportingChange() to correspond to the ifdef around setting prefs::kMetricsReportingEnabled and revert this function back to the simpler version. (In future CLs when we make this code support those platforms completely, we can expand this - but I'm hoping that can be done with fewer ifdefs.)
Patchset #20 (id:370001) has been deleted
Patchset #19 (id:350001) has been deleted
Patchset #19 (id:390001) has been deleted
gayane@chromium.org changed reviewers: - sky@chromium.org, stevenjb@chromium.org
https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:62: #if !defined(OS_CHROMEOS) On 2014/09/15 21:36:44, Alexei Svitkine wrote: > I would also expand this ifdef to be !ANDROID too. Done. https://codereview.chromium.org/506663003/diff/330001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:105: #if defined(OS_ANDROID) On 2014/09/15 21:36:44, Alexei Svitkine wrote: > On 2014/09/15 21:09:17, gayane wrote: > > So the thing is that initially the plan was to use this function for Checkbox > > setup for non-Chrome non-Andriod, but then we added a check in > > InitiateMetricsReportingChange. This function can be called and is called from > > ChromeOS right now. I am not sure about Andrioid. I added it just in case. > > > > Changed the comment for this function. > > I see. But InitiateMetricsReportingChange() doesn't touch the pref on those > platforms yet. So I suggest just ifdefing the code in > InitiateMetricsReportingChange() to correspond to the ifdef around setting > prefs::kMetricsReportingEnabled and revert this function back to the simpler > version. > > (In future CLs when we make this code support those platforms completely, we can > expand this - but I'm hoping that can be done with fewer ifdefs.) Done.
LGTM % comment https://codereview.chromium.org/506663003/diff/410001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/506663003/diff/410001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1897: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && defined(GOOGLE_CHROME_BUILD) The !defined(OS_ANDROID) check isn't needed here, since I don't think the Android build runs this code.
Patchset #20 (id:430001) has been deleted
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/506663003/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset 20 (id:??) landed as https://crrev.com/9f044063e9d771aae5995b6bb9894480e8c292e4 Cr-Commit-Position: refs/heads/master@{#295124}
Message was sent while issue was closed.
Committed patchset #20 (id:450001) manually as 9f04406. |
