Added policy to disable/enable a system log upload.

Added policy to disable/enable a system log upload.

BUG=471888
Committed: https://crrev.com/b1d23bcdd177a18e44593e02b37a7c08fef41261
Cr-Commit-Position: refs/heads/master@{#343861}

[Polina Bondarenko, 2015-08-10 10:12:10]

Patchset #1 (id:1) has been deleted

[Polina Bondarenko, 2015-08-10 10:14:48]

pbond@chromium.org changed reviewers:
+ atwilson@chromium.org

[Polina Bondarenko, 2015-08-10 10:14:49]

Hi Andrew,

could you please take a look at these changes. I added new policy to
enable/disable system log uploads, by default - disabled.

[Andrew T Wilson (Slow), 2015-08-13 11:26:19]

mostly LG, a few questions/points below around the test stuff.

https://codereview.chromium.org/1280003004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right):

https://codereview.chromium.org/1280003004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:169: // Replace
settings provider must be after observer creation in the system
Why is the ordering important? I'm not sure I understand?

https://codereview.chromium.org/1280003004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:191:
InitTestCase(syslog_delegate.Pass());
It's a bit weird to have this Init/DestroyTestCase() stuff. Is it not possible
to have this in the SetUp()/TearDown() code?

[Polina Bondarenko, 2015-08-13 14:51:16]

pbond@chromium.org changed reviewers:
+ derat@chromium.org, mpearson@chromium.org

[Polina Bondarenko, 2015-08-13 14:51:17]

Hi,

I added a policy to disable/enable the system log upload, by default it is
disabled, the "enabled flag" is stored in crossettings. Please take a look at my
changes.

mpearson@chromium.org: Please review changes in 

tools/metrics/histograms/histograms.xml

derat@chromium.org: Please review changes in 

chrome/browser/chromeos/settings/device_settings_provider.cc
chrome/browser/chromeos/settings/device_settings_provider_unittest.cc
chromeos/settings/cros_settings_names.h
chromeos/settings/cros_settings_names.cc

https://codereview.chromium.org/1280003004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right):

https://codereview.chromium.org/1280003004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:169: // Replace
settings provider must be after observer creation in the system
On 2015/08/13 11:26:18, Andrew T Wilson wrote:
> Why is the ordering important? I'm not sure I understand?

Yes, you're right, it's not important, removed it.

https://codereview.chromium.org/1280003004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:191:
InitTestCase(syslog_delegate.Pass());
On 2015/08/13 11:26:18, Andrew T Wilson wrote:
> It's a bit weird to have this Init/DestroyTestCase() stuff. Is it not possible
> to have this in the SetUp()/TearDown() code?

Yes, it's possible, done.

[Andrew T Wilson (Slow), 2015-08-13 15:03:21]

lgtm

https://codereview.chromium.org/1280003004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/system_log_uploader_unittest.cc (right):

https://codereview.chromium.org/1280003004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/system_log_uploader_unittest.cc:192:
settings_helper_.SetBoolean(chromeos::kLogUploadEnabled, false);
Remove this line to make sure we default to false.

[Polina Bondarenko, 2015-08-13 15:09:05]

Fixed disabled by default unit test.

[Daniel Erat, 2015-08-13 15:13:39]

lgtm for settings

https://codereview.chromium.org/1280003004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc
(right):

https://codereview.chromium.org/1280003004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:502: //
Turn on log upload and verify that the log upload settings have been
nit: i'd probably delete these comments; the function calls here seem
self-explanatory

[Polina Bondarenko, 2015-08-14 07:37:58]

pbond@chromium.org changed reviewers:
- mpearson@chromium.org

[Polina Bondarenko, 2015-08-14 07:40:19]

pbond@chromium.org changed reviewers:
+ isherman@chromium.org

[Polina Bondarenko, 2015-08-14 07:40:19]

Hi,

isherman@chromium.org: Please review changes in 

tools/metrics/histograms/histograms.xml

Added new device policy to enable/disable system log upload.

[Ilya Sherman, 2015-08-14 21:19:47]

histograms.xml lgtm

[Ilya Sherman, 2015-08-14 21:20:07]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-08-14 21:20:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1280003004/#ps80001
(title: "Removed comments from unit tests.")

[commit-bot: I haz the power, 2015-08-14 21:20:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280003004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280003004/80001

[commit-bot: I haz the power, 2015-08-14 22:34:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-14 22:34:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Polina Bondarenko, 2015-08-17 08:18:39]

The CQ bit was checked by pbond@chromium.org

[commit-bot: I haz the power, 2015-08-17 08:18:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280003004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280003004/80001

[commit-bot: I haz the power, 2015-08-17 09:25:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-17 09:25:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Polina Bondarenko, 2015-08-17 17:23:43]

Patchset #5 (id:100001) has been deleted

[Polina Bondarenko, 2015-08-17 17:46:12]

The CQ bit was checked by pbond@chromium.org

[Polina Bondarenko, 2015-08-17 17:46:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, derat@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1280003004/#ps120001
(title: "Fixed build to pass browser and unit tests.")

[commit-bot: I haz the power, 2015-08-17 17:46:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280003004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280003004/120001

[commit-bot: I haz the power, 2015-08-17 20:16:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-17 20:16:48]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Polina Bondarenko, 2015-08-18 08:22:21]

The CQ bit was checked by pbond@chromium.org

[commit-bot: I haz the power, 2015-08-18 08:22:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280003004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280003004/120001

[commit-bot: I haz the power, 2015-08-18 08:57:49]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-08-18 08:58:32]

Patchset 5 (id:??) landed as
https://crrev.com/b1d23bcdd177a18e44593e02b37a7c08fef41261
Cr-Commit-Position: refs/heads/master@{#343861}