Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411863002:
Use kMetricsReportingEnabled instead of kStatsReporingPref on metrics side (Closed)
|
Created:
5 years, 2 months ago by gayane -on leave until 09-2017 Modified:
5 years, 1 month ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, oshima+watch_chromium.org, dzhioev+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, davemoore+watch_chromium.org, bartfab (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse kMetricsReportingEnabled instead of kStatsReporingPref on metrics side
Main changes:
- mapping kMetricsReportingEnabled to platform specific policy
- change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref
- For ChromeOS, initialize kMetricsReportingEnabled with kStatsReporingPref
- Update both kMetricsReportingEnabled and kStatsReporingPref, when user toggles the checkbox
- remove outdated migration code for kStatsReporingPref
Here are the scenarios that I tested manually:
- Start Chrome OS on fresh dir. Checkbox shows checked or unchecked based on what user selected during EULA accept. Check that its the same after restarting Chrome OS
- Start Chrome OS on fresh user dir, test with non owner. Shows disabled checkbox with "controled by owner" icon. Its checked or unchecked based on what user selected on EULA accept
- Start on Chrome OS on fresh dir, enroll to enterprise policy. Regardless of what was selected during EULA phase the checkbox is checked and disabled with "controled by policy" icon
- Start Chrome OS without current changes on clean user dir, uncheck the checkbox. Start Chrome OS which includes current changes. Check that checkbox is still unchecked.
BUG=532084
Committed: https://crrev.com/e7e32dc0a88082d9fa8a85cca1eb36d74cc1966d
Cr-Commit-Position: refs/heads/master@{#360657}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Delete settings pref ref from priviate_api + Added comments #
Total comments: 9
Patch Set 3 : Added TODO #Patch Set 4 : Restore kStatsReportingPref and tie with kMetricsReportingEnabled #Patch Set 5 : sync #Patch Set 6 : sync fix #Patch Set 7 : fix unittests #
Total comments: 12
Patch Set 8 : fix comments #
Total comments: 10
Patch Set 9 : add observer for kStatsReportingPref #
Total comments: 17
Patch Set 10 : nits #
Total comments: 6
Patch Set 11 : fix callback #
Total comments: 8
Patch Set 12 : sync #Patch Set 13 : nits comments TODOs #
Total comments: 4
Patch Set 14 : extend comments and todo #Patch Set 15 : Add default value for device setting #Messages
Total messages: 59 (19 generated)
Patchset #1 (id:1) has been deleted
gayane@chromium.org changed reviewers: + asvitkine@google.com, mnissler@chromium.org
Could you please have an initial look at my changes.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org - asvitkine@google.com
Thanks, really excited by this clean up! Can you expand your CL description to mention how you tested the different parts? https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:483: #if defined(OS_CHROMEOS) I think this could use a comment about the discrepancy. https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:2148: // This function works for official builds. This is a weird comment. Please give some context here for why we only do this for official builds. https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:2168: ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled()); Nit: Indent 2 more - or just use git cl format. https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.h:113: bool IsMetricsReportingOwnerManaged(); This doesn't seem to be an override - if so, it should be in a separate section with a comment. https://codereview.chromium.org/1411863002/diff/20001/chromeos/settings/cros_... File chromeos/settings/cros_settings_names.cc (left): https://codereview.chromium.org/1411863002/diff/20001/chromeos/settings/cros_... chromeos/settings/cros_settings_names.cc:71: const char kStatsReportingPref[] = "cros.metrics.reportingEnabled"; I see a few more places that reference this in codesearch that I don't see in your CL: https://code.google.com/p/chromium/codesearch#search/&q=cros.metrics.reportin...
Description was changed from ========== Moving kStatsReporingPref functionality to kMetricsReportingEnabled Main changes for merging these two prefs: - mapping kMetricsReportingEnabled to platform specific policy - change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref - remove kStatsReporingPref from chromeos device settings Note: I didn't add migration from kStatsReporingPref to kMetricsReportingEnabled as kMetricsReportingEnabled gets initialized either based on existance of consent file or based on policy which was the same behavior as for kStatsReporingPref. I didn't change the unittests yet. BUG=532084 ========== to ========== Moving kStatsReporingPref functionality to kMetricsReportingEnabled Main changes for merging these two prefs: - mapping kMetricsReportingEnabled to platform specific policy - change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref - remove kStatsReporingPref from chromeos device settings Note: I didn't add migration from kStatsReporingPref to kMetricsReportingEnabled as kMetricsReportingEnabled gets initialized either based on existance of consent file or based on policy which was the same behavior as for kStatsReporingPref. I didn't change the unittests yet. Here are the scenarios that I tested manually: - Start Chrome OS on fresh dir. Checkbox shows checked or unchecked based on what user selected during EULA accept. Check that its the same after restarting Chrome OS - Start Chrome OS on fresh user dir, test with non owner. Shows disabled checkbox with "controled by owner" icon. Its checked or unchecked based on what user selected on EULA accept - Start on Chrome OS on fresh dir, enroll to enterprise policy. Regardless of what was selected during EULA phase the checkbox is checked and disabled with "controled by policy" icon - Start Chrome OS without current changes on clean user dir, uncheck the checkbox. Start Chrome OS which includes current changes. Check that checkbox is still unchecked. BUG=532084 ==========
Thanks Alexei for the comments. Please have one more look https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:483: #if defined(OS_CHROMEOS) On 2015/10/16 21:27:16, Alexei Svitkine wrote: > I think this could use a comment about the discrepancy. Done. https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:2148: // This function works for official builds. On 2015/10/16 21:27:16, Alexei Svitkine wrote: > This is a weird comment. Please give some context here for why we only do this > for official builds. Done. https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:2168: ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled()); On 2015/10/16 21:27:16, Alexei Svitkine wrote: > Nit: Indent 2 more - or just use git cl format. Done. https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/1411863002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.h:113: bool IsMetricsReportingOwnerManaged(); On 2015/10/16 21:27:16, Alexei Svitkine wrote: > This doesn't seem to be an override - if so, it should be in a separate section > with a comment. Done. https://codereview.chromium.org/1411863002/diff/20001/chromeos/settings/cros_... File chromeos/settings/cros_settings_names.cc (left): https://codereview.chromium.org/1411863002/diff/20001/chromeos/settings/cros_... chromeos/settings/cros_settings_names.cc:71: const char kStatsReportingPref[] = "cros.metrics.reportingEnabled"; On 2015/10/16 21:27:16, Alexei Svitkine wrote: > I see a few more places that reference this in codesearch that I don't see in > your CL: > > https://code.google.com/p/chromium/codesearch#search/&q=cros.metrics.reportin... Thanks Alexei for pointing this out. The ones that were not in this Cl described below: - policy_test_cases.json will do that at the same time with unittests within this Cl, if this change will seem reasonable to reviewers - prefs_util.cc removed the reference, but didn't add anything instead. Not sure about this - privacy_page.html according to previous conversations I had, a new privacy page was getting designed however the development was stopped and there are no plans. So I disregarded this file.
Looks good for the most part. The aspect I'm not clear about is how a consumer owner on Chrome OS gets to write the device setting. Your change removes the code that handles this and I don't get what replaces it. https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:612: CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); Why did you remove this? I think this is required to store the device setting for consumer devices during OOBE. https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:87: if (IsMetricsReportingPolicyManaged()) { It's weird to have an |enabled| parameter but then restrict it - the canonical solution would be to just take the value obtained from PrefService, and have PrefService worry about sorting out policy lockdown. Then, you could register for change notifications to the pref key with PrefService and just update your state when PrefService signals a pref change. This is mostly background information in case it is useful, you may not be able to follow this advice just yet, as long as there are special cases for some platforms that need to pass the desired value.
Thanks Mattias for having a look. "The aspect I'm not clear about is how a consumer owner on Chrome OS gets to write the device setting." is this about the change in wizard_controller.cc? If so please see the my reply on that file. Also please note that I am not making use of em::MetricsEnabledProto any more? If you can confirm that there is no scenario that I overlooked where this would be useful I can remove this all together. https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:612: CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); On 2015/10/20 08:33:32, Mattias Nissler wrote: > Why did you remove this? I think this is required to store the device setting > for consumer devices during OOBE. Previously here we set kStatsReportingPref as InitiateMetricsReportingChange was not taking care of that. But now above in OnEulaAccepted() InitiateMetricsReportingChange function is called which will set kMetricsReportingEnabled to be usage_statistics_reporting_. This function is a callback afterwards which will start crashreporter if needed. https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:87: if (IsMetricsReportingPolicyManaged()) { On 2015/10/20 08:33:32, Mattias Nissler wrote: > It's weird to have an |enabled| parameter but then restrict it - the canonical > solution would be to just take the value obtained from PrefService, and have > PrefService worry about sorting out policy lockdown. Then, you could register > for change notifications to the pref key with PrefService and just update your > state when PrefService signals a pref change. > > This is mostly background information in case it is useful, you may not be able > to follow this advice just yet, as long as there are special cases for some > platforms that need to pass the desired value. I agree. If you don't mind I will do it in separate CL.
https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:612: CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); On 2015/10/20 14:55:39, gayane wrote: > On 2015/10/20 08:33:32, Mattias Nissler wrote: > > Why did you remove this? I think this is required to store the device setting > > for consumer devices during OOBE. > > Previously here we set kStatsReportingPref as InitiateMetricsReportingChange was > not taking care of that. But now above in OnEulaAccepted() > InitiateMetricsReportingChange function is called which will set > kMetricsReportingEnabled to be usage_statistics_reporting_. > > This function is a callback afterwards which will start crashreporter if needed. AFAICT, InitiateMetricsReportingChange only updates local_state. This insufficient on Chrome OS, where settings affecting all users on the device should be stored in device_settings, to prevent non-owners to mess with these settings. https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/metrics_reporting_state.cc:87: if (IsMetricsReportingPolicyManaged()) { On 2015/10/20 14:55:39, gayane wrote: > On 2015/10/20 08:33:32, Mattias Nissler wrote: > > It's weird to have an |enabled| parameter but then restrict it - the canonical > > solution would be to just take the value obtained from PrefService, and have > > PrefService worry about sorting out policy lockdown. Then, you could register > > for change notifications to the pref key with PrefService and just update your > > state when PrefService signals a pref change. > > > > This is mostly background information in case it is useful, you may not be > able > > to follow this advice just yet, as long as there are special cases for some > > platforms that need to pass the desired value. > > I agree. If you don't mind I will do it in separate CL. Yes, no action required here.
https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:612: CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); On 2015/10/21 09:53:24, Mattias Nissler wrote: > On 2015/10/20 14:55:39, gayane wrote: > > On 2015/10/20 08:33:32, Mattias Nissler wrote: > > > Why did you remove this? I think this is required to store the device > setting > > > for consumer devices during OOBE. > > > > Previously here we set kStatsReportingPref as InitiateMetricsReportingChange > was > > not taking care of that. But now above in OnEulaAccepted() > > InitiateMetricsReportingChange function is called which will set > > kMetricsReportingEnabled to be usage_statistics_reporting_. > > > > This function is a callback afterwards which will start crashreporter if > needed. > > AFAICT, InitiateMetricsReportingChange only updates local_state. This > insufficient on Chrome OS, where settings affecting all users on the device > should be stored in device_settings, to prevent non-owners to mess with these > settings. Can we just guard against this (non-owners of the device messing with the setting) from Chrome code and not have the extra device setting?
https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:612: CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); On 2015/10/21 09:53:24, Mattias Nissler wrote: > On 2015/10/20 14:55:39, gayane wrote: > > On 2015/10/20 08:33:32, Mattias Nissler wrote: > > > Why did you remove this? I think this is required to store the device > setting > > > for consumer devices during OOBE. > > > > Previously here we set kStatsReportingPref as InitiateMetricsReportingChange > was > > not taking care of that. But now above in OnEulaAccepted() > > InitiateMetricsReportingChange function is called which will set > > kMetricsReportingEnabled to be usage_statistics_reporting_. > > > > This function is a callback afterwards which will start crashreporter if > needed. > > AFAICT, InitiateMetricsReportingChange only updates local_state. This > insufficient on Chrome OS, where settings affecting all users on the device > should be stored in device_settings, to prevent non-owners to mess with these > settings. Is there any way for users to change the settings other than UI? For UI I disable the checkbox for non-owner. See BrowserOptionsHandler::IsMetricsReportingOwnerManaged()
https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/1411863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:612: CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled); On 2015/10/21 17:02:51, gayane wrote: > On 2015/10/21 09:53:24, Mattias Nissler wrote: > > On 2015/10/20 14:55:39, gayane wrote: > > > On 2015/10/20 08:33:32, Mattias Nissler wrote: > > > > Why did you remove this? I think this is required to store the device > > setting > > > > for consumer devices during OOBE. > > > > > > Previously here we set kStatsReportingPref as InitiateMetricsReportingChange > > was > > > not taking care of that. But now above in OnEulaAccepted() > > > InitiateMetricsReportingChange function is called which will set > > > kMetricsReportingEnabled to be usage_statistics_reporting_. > > > > > > This function is a callback afterwards which will start crashreporter if > > needed. > > > > AFAICT, InitiateMetricsReportingChange only updates local_state. This > > insufficient on Chrome OS, where settings affecting all users on the device > > should be stored in device_settings, to prevent non-owners to mess with these > > settings. > > Is there any way for users to change the settings other than UI? > > For UI I disable the checkbox for non-owner. See > BrowserOptionsHandler::IsMetricsReportingOwnerManaged() The deal with device settings is that they're carrying a cryptographic signature to prevent non-owner users on the device from changing them. Storing in local_state would subverts that, even if there is no regular way to manipulate the value through the UI. The goal is to contain damage for other users even when one user's session gets attacked and Chrome gets compromised. I therefore suggest you keep the device setting as the ground truth on Chrome OS for all cases. If you want to use the local state pref as a vehicle for communicating the value to/from UI and/or the metrics subsystem, that's fine - but make sure we're not making decisions based on that originates from the local_state file but sync it on startup to what the device setting says.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #8 (id:200001) has been deleted
Patchset #7 (id:180001) has been deleted
Description was changed from ========== Moving kStatsReporingPref functionality to kMetricsReportingEnabled Main changes for merging these two prefs: - mapping kMetricsReportingEnabled to platform specific policy - change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref - remove kStatsReporingPref from chromeos device settings Note: I didn't add migration from kStatsReporingPref to kMetricsReportingEnabled as kMetricsReportingEnabled gets initialized either based on existance of consent file or based on policy which was the same behavior as for kStatsReporingPref. I didn't change the unittests yet. Here are the scenarios that I tested manually: - Start Chrome OS on fresh dir. Checkbox shows checked or unchecked based on what user selected during EULA accept. Check that its the same after restarting Chrome OS - Start Chrome OS on fresh user dir, test with non owner. Shows disabled checkbox with "controled by owner" icon. Its checked or unchecked based on what user selected on EULA accept - Start on Chrome OS on fresh dir, enroll to enterprise policy. Regardless of what was selected during EULA phase the checkbox is checked and disabled with "controled by policy" icon - Start Chrome OS without current changes on clean user dir, uncheck the checkbox. Start Chrome OS which includes current changes. Check that checkbox is still unchecked. BUG=532084 ========== to ========== Use kMetricsReportingEnabled instead of kStatsReporingPref on metrics side Main changes: - mapping kMetricsReportingEnabled to platform specific policy - change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref - For ChromeOS, initialize kMetricsReportingEnabled with kStatsReporingPref - remove outdated migration code for kStatsReporingPref Here are the scenarios that I tested manually: - Start Chrome OS on fresh dir. Checkbox shows checked or unchecked based on what user selected during EULA accept. Check that its the same after restarting Chrome OS - Start Chrome OS on fresh user dir, test with non owner. Shows disabled checkbox with "controled by owner" icon. Its checked or unchecked based on what user selected on EULA accept - Start on Chrome OS on fresh dir, enroll to enterprise policy. Regardless of what was selected during EULA phase the checkbox is checked and disabled with "controled by policy" icon - Start Chrome OS without current changes on clean user dir, uncheck the checkbox. Start Chrome OS which includes current changes. Check that checkbox is still unchecked. BUG=532084 ==========
Description was changed from ========== Use kMetricsReportingEnabled instead of kStatsReporingPref on metrics side Main changes: - mapping kMetricsReportingEnabled to platform specific policy - change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref - For ChromeOS, initialize kMetricsReportingEnabled with kStatsReporingPref - remove outdated migration code for kStatsReporingPref Here are the scenarios that I tested manually: - Start Chrome OS on fresh dir. Checkbox shows checked or unchecked based on what user selected during EULA accept. Check that its the same after restarting Chrome OS - Start Chrome OS on fresh user dir, test with non owner. Shows disabled checkbox with "controled by owner" icon. Its checked or unchecked based on what user selected on EULA accept - Start on Chrome OS on fresh dir, enroll to enterprise policy. Regardless of what was selected during EULA phase the checkbox is checked and disabled with "controled by policy" icon - Start Chrome OS without current changes on clean user dir, uncheck the checkbox. Start Chrome OS which includes current changes. Check that checkbox is still unchecked. BUG=532084 ========== to ========== Use kMetricsReportingEnabled instead of kStatsReporingPref on metrics side Main changes: - mapping kMetricsReportingEnabled to platform specific policy - change metrics code to use kMetricsReportingEnabled instead of kStatsReporingPref - For ChromeOS, initialize kMetricsReportingEnabled with kStatsReporingPref - Update both kMetricsReportingEnabled and kStatsReporingPref, when user toggles the checkbox - remove outdated migration code for kStatsReporingPref Here are the scenarios that I tested manually: - Start Chrome OS on fresh dir. Checkbox shows checked or unchecked based on what user selected during EULA accept. Check that its the same after restarting Chrome OS - Start Chrome OS on fresh user dir, test with non owner. Shows disabled checkbox with "controled by owner" icon. Its checked or unchecked based on what user selected on EULA accept - Start on Chrome OS on fresh dir, enroll to enterprise policy. Regardless of what was selected during EULA phase the checkbox is checked and disabled with "controled by policy" icon - Start Chrome OS without current changes on clean user dir, uncheck the checkbox. Start Chrome OS which includes current changes. Check that checkbox is still unchecked. BUG=532084 ==========
I brought back the kStatsReportingPref and tied it with kMetricsReportingEnabled
https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:739: // it is more reliable for Chrome OS. suggestion: s/it is more reliable for Chrome OS/it is the source of truth on Chrome OS/. https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:742: &enable_metrics); I'd expect an observer to be installed here to re-sync the pref after the device setting changes? https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:245: provider_->SetPrefForTesting(kStatsReportingPref, false); This seems fishy - what failure do you get if you remove this line? https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:65: #endif //! defined(OS_ANDROID) nit: misplaced !
Thanks Mattias for having a look. Please have a look on my comments below https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:739: // it is more reliable for Chrome OS. On 2015/10/28 09:51:15, Mattias Nissler wrote: > suggestion: s/it is more reliable for Chrome OS/it is the source of truth on > Chrome OS/. Done. https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:742: &enable_metrics); On 2015/10/28 09:51:15, Mattias Nissler wrote: > I'd expect an observer to be installed here to re-sync the pref after the device > setting changes? you mean besides initialization? My understanding is that there is no scenario when kStatsReportingPref will change which needs to also affect kMetricsReportingEnabled. Now kStatsReportingPref changes its value only in two cases. - First when policy is applied. in this case kMetricsReportingEnabled gets directly changed by policy - Second when user changes kMetricsReportingEnabled then kStatsReportingPref gets updated accordingly. https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:245: provider_->SetPrefForTesting(kStatsReportingPref, false); On 2015/10/28 09:51:15, Mattias Nissler wrote: > This seems fishy - what failure do you get if you remove this line? The problem I had was that the test will fail at line 260: ASSERT_TRUE(saved_value); Besides that I thought that in the end it checks that the pref value is false, but this is true only if the pref value was false before this test started. https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:65: #endif //! defined(OS_ANDROID) On 2015/10/28 09:51:15, Mattias Nissler wrote: > nit: misplaced ! Done.
https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:742: &enable_metrics); On 2015/10/28 15:00:06, gayane wrote: > On 2015/10/28 09:51:15, Mattias Nissler wrote: > > I'd expect an observer to be installed here to re-sync the pref after the > device > > setting changes? > > you mean besides initialization? My understanding is that there is no scenario > when kStatsReportingPref will change which needs to also affect > kMetricsReportingEnabled. > > Now kStatsReportingPref changes its value only in two cases. > - First when policy is applied. in this case kMetricsReportingEnabled gets > directly changed by policy > - Second when user changes kMetricsReportingEnabled then kStatsReportingPref > gets updated accordingly. I guess you're referring to the CrosSettings::SetBoolean call in browser_options_handler.cc here, correct? While in theory what you say is correct, the assumption that whatever Chrome writes is fragile. I'd prefer a solution that enables/disables metrics reporting based on the actual ground truth value reported by device settings, and not just Chrome's idea what it should be. For example, we do have some code in session_manager that may sanitize certain device settings. While it currently doesn't touch the metrics reporting flag, the existence of that code shows that second-guessing the behavior of device settings is not the best of ideas. https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:245: provider_->SetPrefForTesting(kStatsReportingPref, false); On 2015/10/28 15:00:07, gayane wrote: > On 2015/10/28 09:51:15, Mattias Nissler wrote: > > This seems fishy - what failure do you get if you remove this line? > > The problem I had was that the test will fail at line 260: > ASSERT_TRUE(saved_value); > > Besides that I thought that in the end it checks that the pref value is false, > but this is true only if the pref value was false before this test started. OK, I get it now. The previous code would set kStatsReportingPref based on the migration fallback, but that is gone now, so the value is missing. The proper way to set the initial value for the test is to put it in device settings. For an example of how to do that, see SetUploadLogSettings() above.
https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:742: &enable_metrics); On 2015/10/29 10:06:14, Mattias Nissler wrote: > On 2015/10/28 15:00:06, gayane wrote: > > On 2015/10/28 09:51:15, Mattias Nissler wrote: > > > I'd expect an observer to be installed here to re-sync the pref after the > > device > > > setting changes? > > > > you mean besides initialization? My understanding is that there is no scenario > > when kStatsReportingPref will change which needs to also affect > > kMetricsReportingEnabled. > > > > Now kStatsReportingPref changes its value only in two cases. > > - First when policy is applied. in this case kMetricsReportingEnabled gets > > directly changed by policy > > - Second when user changes kMetricsReportingEnabled then kStatsReportingPref > > gets updated accordingly. > > I guess you're referring to the CrosSettings::SetBoolean call in > browser_options_handler.cc here, correct? While in theory what you say is > correct, the assumption that whatever Chrome writes is fragile. I'd prefer a > solution that enables/disables metrics reporting based on the actual ground > truth value reported by device settings, and not just Chrome's idea what it > should be. > > For example, we do have some code in session_manager that may sanitize certain > device settings. While it currently doesn't touch the metrics reporting flag, > the existence of that code shows that second-guessing the behavior of device > settings is not the best of ideas. Can the observer change be done in a later CL? I think it would be a bit non-trivial, since it also requires toggling UMA service state and other things, and for that kind of more complicated logic we probably need test coverage. How about having a TODO comment with an associated crbug for this CL? Especially that you say it can't change right with the current code right now. https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:744: OnMetricsReportingCallbackType()); Can this code be somewhere in chrome/browser/metrics? e.g. in this ctor: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met... https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_accessor.cc:24: // This is only possible during unittests. If the unittest didn't set the Nit: "unit tests", two words. https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:481: if (!cr.isChromeOS && !cr.isMac) { Instead of duplicating these checks with the ones above, can you make some local boolean vars for these that are named semantically and use them instead? e.g. var togglingMetricsRequiresRestart = !cr.isMac; ... if (togglingMetricsRequiresRestart) https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2149: // it doesn't need setup for non-official builds. Nit: "setup" -> "to be set up" https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2206: bool BrowserOptionsHandler::IsMetricsReportingOwnerManaged() { This method name is confusing. I think you're trying to have the semantics of "it's owned managed and I'm not the owner", but the method name omits the last part. Please rename so there's no such confusion.
https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:742: &enable_metrics); On 2015/10/29 17:04:14, Alexei Svitkine (slow) wrote: > On 2015/10/29 10:06:14, Mattias Nissler wrote: > > On 2015/10/28 15:00:06, gayane wrote: > > > On 2015/10/28 09:51:15, Mattias Nissler wrote: > > > > I'd expect an observer to be installed here to re-sync the pref after the > > > device > > > > setting changes? > > > > > > you mean besides initialization? My understanding is that there is no > scenario > > > when kStatsReportingPref will change which needs to also affect > > > kMetricsReportingEnabled. > > > > > > Now kStatsReportingPref changes its value only in two cases. > > > - First when policy is applied. in this case kMetricsReportingEnabled gets > > > directly changed by policy > > > - Second when user changes kMetricsReportingEnabled then > kStatsReportingPref > > > gets updated accordingly. > > > > I guess you're referring to the CrosSettings::SetBoolean call in > > browser_options_handler.cc here, correct? While in theory what you say is > > correct, the assumption that whatever Chrome writes is fragile. I'd prefer a > > solution that enables/disables metrics reporting based on the actual ground > > truth value reported by device settings, and not just Chrome's idea what it > > should be. > > > > For example, we do have some code in session_manager that may sanitize certain > > device settings. While it currently doesn't touch the metrics reporting flag, > > the existence of that code shows that second-guessing the behavior of device > > settings is not the best of ideas. > > Can the observer change be done in a later CL? I think it would be a bit > non-trivial, since it also requires toggling UMA service state and other things, > and for that kind of more complicated logic we probably need test coverage. How > about having a TODO comment with an associated crbug for this CL? Especially > that you say it can't change right with the current code right now. Note that the old code does handle device settings changes in device_settings_provider.cc and switches metrics reporting state appropriately, so this CL would essentially be a step back on this aspect. For this reason, I'm feeling a bit uncomfortable with a bug + TODO - how about we make the functional change as part of this CL and then postpone test coverage to a follow-up?
I have added an observer on kStatsReportingPref and address all the other comments. https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:744: OnMetricsReportingCallbackType()); On 2015/10/29 17:04:14, Alexei Svitkine (slow) wrote: > Can this code be somewhere in chrome/browser/metrics? > > e.g. in this ctor: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met... Moved to the constructor https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_accessor.cc:24: // This is only possible during unittests. If the unittest didn't set the On 2015/10/29 17:04:14, Alexei Svitkine (slow) wrote: > Nit: "unit tests", two words. Done. https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:481: if (!cr.isChromeOS && !cr.isMac) { On 2015/10/29 17:04:14, Alexei Svitkine (slow) wrote: > Instead of duplicating these checks with the ones above, can you make some local > boolean vars for these that are named semantically and use them instead? > > e.g. > var togglingMetricsRequiresRestart = !cr.isMac; > ... > if (togglingMetricsRequiresRestart) Done. https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2149: // it doesn't need setup for non-official builds. On 2015/10/29 17:04:14, Alexei Svitkine (slow) wrote: > Nit: "setup" -> "to be set up" Done. https://codereview.chromium.org/1411863002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2206: bool BrowserOptionsHandler::IsMetricsReportingOwnerManaged() { On 2015/10/29 17:04:14, Alexei Svitkine (slow) wrote: > This method name is confusing. > > I think you're trying to have the semantics of "it's owned managed and I'm not > the owner", but the method name omits the last part. Please rename so there's no > such confusion. Couldn't come up with nice name with negation in the name, so renamed to IsDeviceOwnerProfile, but this now doesn't include the logic that when user is not owner then cannot change device settings
https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:40: void SetupMetricsStateForChromeOS() { Suggest moving this to metrics_reporting_state.cc and then OnDeviceSettingChange() can be a local method to that file in the anon namespace. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:120: void onDeviceSettingsChange() { Nit: OnDeviceSettingChange() https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:1756: policy_managed, Nit: camelCase for js vars https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2148: // As the Metrics and crash reporting checkbox only exists for official builds Nit: Don't cap metrics. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.h:395: // Checks whether on Chrome OS current user is the device owner. Returns true Nit: "current user" -> "the current user" https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.h:396: // for the other platforms. Nit: "for the other platforms" -> "on other platforms"
https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:245: SetLogUploadSettings(false); It's fine to use log upload as the test pref, but I think you need to switch kStartsReportingPref to kSystemLogUploadEnabled below for this to work. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2173: InitiateMetricsReportingChange( You shouldn't call this for OS_CHROMEOS as the CrosSettings::SetBoolean() call will update the value, and once they system updates the value, the observer you added will pick up the new state.
Thanks. I have addressed all the comments https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:245: SetLogUploadSettings(false); On 2015/11/03 13:24:06, Mattias Nissler wrote: > It's fine to use log upload as the test pref, but I think you need to switch > kStartsReportingPref to kSystemLogUploadEnabled below for this to work. I have created another function which changes the correct proto https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:40: void SetupMetricsStateForChromeOS() { On 2015/11/02 21:50:32, Alexei Svitkine (slow) wrote: > Suggest moving this to metrics_reporting_state.cc and then > OnDeviceSettingChange() can be a local method to that file in the anon > namespace. Done. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:120: void onDeviceSettingsChange() { On 2015/11/02 21:50:33, Alexei Svitkine (slow) wrote: > Nit: OnDeviceSettingChange() Done. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:1756: policy_managed, On 2015/11/02 21:50:33, Alexei Svitkine (slow) wrote: > Nit: camelCase for js vars Done. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2148: // As the Metrics and crash reporting checkbox only exists for official builds On 2015/11/02 21:50:33, Alexei Svitkine (slow) wrote: > Nit: Don't cap metrics. Done. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2173: InitiateMetricsReportingChange( On 2015/11/03 13:24:06, Mattias Nissler wrote: > You shouldn't call this for OS_CHROMEOS as the CrosSettings::SetBoolean() call > will update the value, and once they system updates the value, the observer you > added will pick up the new state. Done. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.h:395: // Checks whether on Chrome OS current user is the device owner. Returns true On 2015/11/02 21:50:33, Alexei Svitkine (slow) wrote: > Nit: "current user" -> "the current user" Done. https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.h:396: // for the other platforms. On 2015/11/02 21:50:33, Alexei Svitkine (slow) wrote: > Nit: "for the other platforms" -> "on other platforms" Done.
https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2167: if (!IsDeviceOwnerProfile() || IsMetricsReportingPolicyManaged()) { Add a comment please. https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2175: enable); How will MetricsReportingChangeCallback() be invoked in this case? Or is it okay not to invoke it on CrOS for some reason? (If so, there needs to be a comment.) https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.h:379: void MetricsReportingChangeCallback(bool enabled); I find the name of this method not very intuitive. I know it was like that before, but can you change it to be more meaningful? e.g. NotifyPageOfMetricsReportingChange()?
https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2167: if (!IsDeviceOwnerProfile() || IsMetricsReportingPolicyManaged()) { On 2015/11/03 19:55:24, Alexei Svitkine (slow) wrote: > Add a comment please. Done. https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2175: enable); On 2015/11/03 19:55:25, Alexei Svitkine (slow) wrote: > How will MetricsReportingChangeCallback() be invoked in this case? Or is it okay > not to invoke it on CrOS for some reason? (If so, there needs to be a comment.) That's right I have lost the callback for chromeos. https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.h (right): https://codereview.chromium.org/1411863002/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.h:379: void MetricsReportingChangeCallback(bool enabled); On 2015/11/03 19:55:25, Alexei Svitkine (slow) wrote: > I find the name of this method not very intuitive. I know it was like that > before, but can you change it to be more meaningful? > > e.g. NotifyPageOfMetricsReportingChange()? Done.
https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:89: void onDeviceSettingChange() { Nit: Capitalize. Also, add a comment. https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:97: } // namespace Nit: Add an empty line above. https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:131: chromeos::kStatsReportingPref, base::Bind(&onDeviceSettingChange)); Is it possible to add a unit test for this? i.e. make the test change the CrOS pref and verify whether the other pref was updated and metrics service was started or stopped? https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2179: InitiateMetricsReportingChange( 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.)
https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2173: InitiateMetricsReportingChange( On 2015/11/03 19:46:17, gayane wrote: > On 2015/11/03 13:24:06, Mattias Nissler wrote: > > You shouldn't call this for OS_CHROMEOS as the CrosSettings::SetBoolean() call > > will update the value, and once they system updates the value, the observer > you > > added will pick up the new state. > > Done. Not done?
asvitkine@ address all the comments mnissler@ I have reverted back the change you asked for in BrowserOptionsHandler because even though observer of device settings will successfully update metrics service and kMetricsReportingEnabled pref, we would still need to update the UI in case the update will fail. However I have added a TODO, which I will address in my follow up refactoring, to get rid of additional invocation of InitiateMetricsReportingChange function. https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:89: void onDeviceSettingChange() { On 2015/11/03 23:18:23, Alexei Svitkine (slow) wrote: > Nit: Capitalize. Also, add a comment. Done. https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:97: } // namespace On 2015/11/03 23:18:23, Alexei Svitkine (slow) wrote: > Nit: Add an empty line above. Done. https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:131: chromeos::kStatsReportingPref, base::Bind(&onDeviceSettingChange)); On 2015/11/03 23:18:23, Alexei Svitkine (slow) wrote: > Is it possible to add a unit test for this? > > i.e. make the test change the CrOS pref and verify whether the other pref was > updated and metrics service was started or stopped? As to discussion, I added a TODO to add unittests after refactoring of metrics_reporting_state https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2179: InitiateMetricsReportingChange( On 2015/11/03 23:18:23, Alexei Svitkine (slow) wrote: > 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.) You are right InitiateMetricsReportingChange will be called twice for Chrome OS but it should be fine. I added a TODO to clean this up.
lgtm https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:89: // Callback function for Chrome OS device settings change. Nit: Expand comment. e.g. (% wrapping) // Callback for when Chrome OS device settings change, so that the update is applied to metrics reporting state.
LGTM after adjusting the comment. https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2180: // OS. Sorry, but this is not just a cosmetic issue about a function getting called twice. You should only ever call InitiateMetricsReportingChange() once device settings say metrics reporting is on. Please update the comment to state that metrics reporting state can only change after the device settings has flipped to true. Only then can you have full confidence that the change was initiated by the device owner. The TODO should also reference a bug. I'm fine with addressing these points in subsequent CLs, given that you have the check above to stop any action in case we're not the device owner.
Patchset #14 (id:360001) has been deleted
Thanks for the comments. https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/metrics... File chrome/browser/metrics/metrics_reporting_state.cc (right): https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/metrics... chrome/browser/metrics/metrics_reporting_state.cc:89: // Callback function for Chrome OS device settings change. On 2015/11/05 20:20:00, Alexei Svitkine (slow) wrote: > Nit: Expand comment. > > e.g. (% wrapping) > > // Callback for when Chrome OS device settings change, so that the update is > applied to metrics reporting state. Done. https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1411863002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2180: // OS. On 2015/11/06 13:05:09, Mattias Nissler wrote: > Sorry, but this is not just a cosmetic issue about a function getting called > twice. You should only ever call InitiateMetricsReportingChange() once device > settings say metrics reporting is on. Please update the comment to state that > metrics reporting state can only change after the device settings has flipped to > true. Only then can you have full confidence that the change was initiated by > the device owner. > > The TODO should also reference a bug. > > I'm fine with addressing these points in subsequent CLs, given that you have the > check above to stop any action in case we're not the device owner. Done.
gayane@chromium.org changed reviewers: + bartfab@chromium.org, thakis@chromium.org
bartfab@chromium.org: Please review changes in chrome/browser/policy/configuration_policy_handler_list_factory.cc thakis@chromium.org: Please review changes in chrome/browser/browser_process_impl.cc chrome/browser/profiles/* chrome/browser/resources/* chrome/browser/ui/webui/*
rs-lgtm based on mnissler's review
gayane@chromium.org changed reviewers: - bartfab@chromium.org
gayane@chromium.org changed reviewers: + cschuet@chromium.org
+cschuet for configuration_policy_handler_list_factory.cc OWNERS review
On 2015/11/13 19:25:59, gayane wrote: > +cschuet for configuration_policy_handler_list_factory.cc OWNERS review LGTM
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1411863002/#ps380001 (title: "extend comments and todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411863002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411863002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
I have added a default value for kStatsReporingPref device setting to fix failing browser_tests. This is necessary in cases when chrome is launched for the first time without first-run experience.
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, asvitkine@chromium.org, cschuet@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1411863002/#ps400001 (title: "Add default value for device setting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411863002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411863002/400001
Message was sent while issue was closed.
Committed patchset #15 (id:400001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/e7e32dc0a88082d9fa8a85cca1eb36d74cc1966d Cr-Commit-Position: refs/heads/master@{#360657}
Message was sent while issue was closed.
Just a tip for future CLs: when rebasing and also making changes, upload a patchset just after the rebase before you make any new changes. Then, upload a patchset with the extra changes. Otherwise, it makes it hard for reviewers to see in the tool which changes between patchset are yours and which came from the rebase. (e.g. if I diff between patchset 15 and patchset 14 here). No worries this time, but a suggestion for next time.
Message was sent while issue was closed.
On 2015/11/19 21:19:44, Alexei Svitkine (slow) wrote: > Just a tip for future CLs: when rebasing and also making changes, upload a > patchset just after the rebase before you make any new changes. Then, upload a > patchset with the extra changes. > > Otherwise, it makes it hard for reviewers to see in the tool which changes > between patchset are yours and which came from the rebase. (e.g. if I diff > between patchset 15 and patchset 14 here). > > No worries this time, but a suggestion for next time. Ah, sorry. Forgot that I synced. |
