Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396103002:
Create policy ReportArcStatus (Closed)
DescriptionCreate policy ReportArcStatus
It controls that a status blob generated by CloudDPC is uploaded
with the Chrome OS reporting system as part of the session status.
TEST=browsertest
BUG=654431
BUG=b/31084348
Committed: https://crrev.com/9e31a6f21150d74e3b677e7b8d01af1d6b3dcad6
Cr-Commit-Position: refs/heads/master@{#425029}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase, Id increased #Patch Set 3 : Fix pref-mapping test #
Total comments: 16
Patch Set 4 : per-profile=false #Patch Set 5 : Address nits #
Total comments: 2
Patch Set 6 : const string-> const char[] #Patch Set 7 : Rebase #Messages
Total messages: 52 (38 generated)
phweiss@chromium.org changed reviewers: + bartfab@chromium.org
ptal
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
phweiss@chromium.org changed reviewers: + jwd@chromium.org
Hi Jesse, ptal at histograms.xml.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
histograms lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2396103002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2396103002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:557: report_android_status_ = local_state_->GetBoolean(prefs::kReportArcStatus); If this is a per-profile pref, you should not be accessing it via local state but via a profile.
https://codereview.chromium.org/2396103002/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/device_status_collector.cc (right): https://codereview.chromium.org/2396103002/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/device_status_collector.cc:557: report_android_status_ = local_state_->GetBoolean(prefs::kReportArcStatus); On 2016/10/10 14:40:14, bartfab (slow) wrote: > If this is a per-profile pref, you should not be accessing it via local state > but via a profile. Changing the documentation to "not per-profile".
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Create policy ReportArcStatus It controls that a status blob generated by CloudDPC is uploaded with the Chrome OS reporting system as part of the session status. TEST=browsertest BUG=b/31084348 ========== to ========== Create policy ReportArcStatus It controls that a status blob generated by CloudDPC is uploaded with the Chrome OS reporting system as part of the session status. TEST=browsertest BUG=654431 BUG=b/31084348 ==========
https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:235: FROM_HERE, Bind(&CallAndroidStatusReceiver, receiver, Nit: s/Bind/base::Bind/ (why does this even compile? Who is defining Bind in the global namespace? :( ) https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:998: mojo::String status = "{\"applications\":[ { " Here and below: Nit 1: const. Nit 2: You can use an std::string here. It will be automatically converted to a mojo::String as needed. https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1016: mojo::String status = "{\"applications\":[ { " Nit: Can you move these lengthy constants to an anonymous namespace at the top of the file? https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1031: base::MakeUnique<policy::DeviceLocalAccount>(fake_device_local_account_)); Nit: s/policy::// https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1050: prefs_.SetBoolean(prefs::kReportArcStatus, false); Nit: Even if this was true, reproting should return nothing because it is off overall. Is it worth having one more test case for this? https://codereview.chromium.org/2396103002/diff/30010/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2396103002/diff/30010/chrome/common/pref_name... chrome/common/pref_names.h:26: extern const char kReportArcStatus[]; Per the comment above, this is for profile prefs, but you are adding a local state pref. Also, why is this conditional on ENABLE_APP_LIST? https://codereview.chromium.org/2396103002/diff/30010/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2396103002/diff/30010/components/policy/resou... components/policy/resources/policy_templates.json:124: # into the browser or only one profile. This only affects the template Nit: Please remove "this only affects the template generation." This may be true for the policy code (we just pass the value through to anyone who asks), but we expect the actual subsystems that use the policy to honor this. https://codereview.chromium.org/2396103002/diff/30010/components/policy/resou... components/policy/resources/policy_templates.json:4995: }, Nit 1: Where is that status reported to? Nit 2: Document that if reporting is off in the first place, this is ignored.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:235: FROM_HERE, Bind(&CallAndroidStatusReceiver, receiver, On 2016/10/10 15:49:16, bartfab (slow) wrote: > Nit: s/Bind/base::Bind/ (why does this even compile? Who is defining Bind in the > global namespace? :( ) Done. https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:998: mojo::String status = "{\"applications\":[ { " On 2016/10/10 15:49:16, bartfab (slow) wrote: > Here and below: > > Nit 1: const. > Nit 2: You can use an std::string here. It will be automatically converted to a > mojo::String as needed. Done. https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1016: mojo::String status = "{\"applications\":[ { " On 2016/10/10 15:49:16, bartfab (slow) wrote: > Nit: Can you move these lengthy constants to an anonymous namespace at the top > of the file? Done. https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1031: base::MakeUnique<policy::DeviceLocalAccount>(fake_device_local_account_)); On 2016/10/10 15:49:16, bartfab (slow) wrote: > Nit: s/policy::// Done. https://codereview.chromium.org/2396103002/diff/30010/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:1050: prefs_.SetBoolean(prefs::kReportArcStatus, false); On 2016/10/10 15:49:16, bartfab (slow) wrote: > Nit: Even if this was true, reproting should return nothing because it is off > overall. Is it worth having one more test case for this? In the current implementation, kReportDeviceSessionStatus only controls the kiosk session reporting (as it says in its documentation), and is independent of ARC reporting. You need to turn both off to receive nothing. I added a comment. https://codereview.chromium.org/2396103002/diff/30010/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2396103002/diff/30010/chrome/common/pref_name... chrome/common/pref_names.h:26: extern const char kReportArcStatus[]; On 2016/10/10 15:49:16, bartfab (slow) wrote: > Per the comment above, this is for profile prefs, but you are adding a local > state pref. Also, why is this conditional on ENABLE_APP_LIST? Moving it down to local state prefs, and out of the ENABLE_APP_LIST #if. https://codereview.chromium.org/2396103002/diff/30010/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2396103002/diff/30010/components/policy/resou... components/policy/resources/policy_templates.json:124: # into the browser or only one profile. This only affects the template On 2016/10/10 15:49:17, bartfab (slow) wrote: > Nit: Please remove "this only affects the template generation." This may be true > for the policy code (we just pass the value through to anyone who asks), but we > expect the actual subsystems that use the policy to honor this. Done. https://codereview.chromium.org/2396103002/diff/30010/components/policy/resou... components/policy/resources/policy_templates.json:4995: }, On 2016/10/10 15:49:17, bartfab (slow) wrote: > Nit 1: Where is that status reported to? > Nit 2: Document that if reporting is off in the first place, this is ignored. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2396103002/diff/70001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2396103002/diff/70001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:84: const std::string kArcStatus = "{\"applications\":[ { " Nit: You are adding static initializers, which are forbidden in Chrome code. String constants should be char[] instead.
https://codereview.chromium.org/2396103002/diff/70001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc (right): https://codereview.chromium.org/2396103002/diff/70001/chrome/browser/chromeos... chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:84: const std::string kArcStatus = "{\"applications\":[ { " On 2016/10/13 13:18:04, bartfab (slow) wrote: > Nit: You are adding static initializers, which are forbidden in Chrome code. > String constants should be char[] instead. Done.
The CQ bit was checked by phweiss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org, bartfab@chromium.org Link to the patchset: https://codereview.chromium.org/2396103002/#ps90001 (title: "const string-> const char[]")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by phweiss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2396103002/#ps110001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create policy ReportArcStatus It controls that a status blob generated by CloudDPC is uploaded with the Chrome OS reporting system as part of the session status. TEST=browsertest BUG=654431 BUG=b/31084348 ========== to ========== Create policy ReportArcStatus It controls that a status blob generated by CloudDPC is uploaded with the Chrome OS reporting system as part of the session status. TEST=browsertest BUG=654431 BUG=b/31084348 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Create policy ReportArcStatus It controls that a status blob generated by CloudDPC is uploaded with the Chrome OS reporting system as part of the session status. TEST=browsertest BUG=654431 BUG=b/31084348 ========== to ========== Create policy ReportArcStatus It controls that a status blob generated by CloudDPC is uploaded with the Chrome OS reporting system as part of the session status. TEST=browsertest BUG=654431 BUG=b/31084348 Committed: https://crrev.com/9e31a6f21150d74e3b677e7b8d01af1d6b3dcad6 Cr-Commit-Position: refs/heads/master@{#425029} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9e31a6f21150d74e3b677e7b8d01af1d6b3dcad6 Cr-Commit-Position: refs/heads/master@{#425029} |
