kiosk: Add status report for os update and running app

kiosk: Add status report for os update and running app

- Update chrome_device_settings.proto to include report setting
  for os_update_status and running_kiosk_app;
- Update device_management_backend.proto to include os_update_status
  and running_kiosk_app in DeviceStatusReportRequest;

BUG=577800
TEST=browser_tests --gtest_filter=DeviceStatusCollectorTest.*OsUpdateStatus*:
    DeviceStatusCollectorTest.*RunningKioskApp*

Committed: https://crrev.com/bdf91bc327487db842a0831fbf2247b671b3d988
Cr-Commit-Position: refs/heads/master@{#405196}

[xiyuan, 2016-07-07 16:42:25]

xiyuan@chromium.org changed reviewers:
+ jennyz@chromium.org, jinzhang@chromium.org, tnagel@chromium.org

[xiyuan, 2016-07-07 16:42:25]

Suggested split:

jinzhang@,
  *.proto
  c/b/policy/device_status_collector.cc
  to check whether the report sent from client matches
  server side expectation
  Note that the comment for running_kiosk_app says it
  is sent when there is a running kiosk app. However, it
  might be confusing because required_platform_version is
  only used for the auto launched kiosk app. So the CL
  only reports that for the auto launched kiosk app. Let
  me know if that is okay.

jennyz@:
  c/b/chromeos/app_mode/*

tnagel@:
  General review for policy code change since you are
  the only owner that are not marked "slow" these days. :)

Thanks.

[jinzhang1, 2016-07-07 17:46:42]

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
components/policy/proto/device_management_backend.proto:755: // If no kiosk app
is currently running, then this field is empty.
Let's change this comment to:
// Set only when there is an auto-launched Kiosk app configured.

WDYT? If you agree, I'll update the comment on the server-side proto as well.

[xiyuan, 2016-07-07 17:53:59]

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
components/policy/proto/device_management_backend.proto:755: // If no kiosk app
is currently running, then this field is empty.
On 2016/07/07 17:46:42, jinzhang1 wrote:
> Let's change this comment to:
> // Set only when there is an auto-launched Kiosk app configured.
> 
> WDYT? If you agree, I'll update the comment on the server-side proto as well.

I updated with the following:
  // Set only when there is an auto launched with zero delay kiosk app
  // and it is currently running. Otherwise, this field is empty.

Think it is more close to what we report.

[jennyz, 2016-07-07 17:58:55]

lgtm for c/b/chromeos/app_mode/*

[jinzhang1, 2016-07-07 17:59:31]

lgtm

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
components/policy/proto/device_management_backend.proto:755: // If no kiosk app
is currently running, then this field is empty.
On 2016/07/07 17:53:59, xiyuan wrote:
> On 2016/07/07 17:46:42, jinzhang1 wrote:
> > Let's change this comment to:
> > // Set only when there is an auto-launched Kiosk app configured.
> > 
> > WDYT? If you agree, I'll update the comment on the server-side proto as
well.
> 
> I updated with the following:
>   // Set only when there is an auto launched with zero delay kiosk app
>   // and it is currently running. Otherwise, this field is empty.
> 
> Think it is more close to what we report.

LGTM. Did you forget to update the CL?

[jinzhang1, 2016-07-07 17:59:32]

lgtm

[jinzhang1, 2016-07-07 18:00:15]

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/20001/components/policy/proto...
components/policy/proto/device_management_backend.proto:755: // If no kiosk app
is currently running, then this field is empty.
On 2016/07/07 17:59:31, jinzhang1 wrote:
> On 2016/07/07 17:53:59, xiyuan wrote:
> > On 2016/07/07 17:46:42, jinzhang1 wrote:
> > > Let's change this comment to:
> > > // Set only when there is an auto-launched Kiosk app configured.
> > > 
> > > WDYT? If you agree, I'll update the comment on the server-side proto as
> well.
> > 
> > I updated with the following:
> >   // Set only when there is an auto launched with zero delay kiosk app
> >   // and it is currently running. Otherwise, this field is empty.
> > 
> > Think it is more close to what we report.
> 
> LGTM. Did you forget to update the CL?

never mind. saw the new patch. lgtm.

[Thiemo Nagel, 2016-07-10 08:47:47]

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

https://codereview.chromium.org/2132663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:960:
os_update_status->set_update_status(
Would it make sense to set_new_platform_version() here as well?

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

https://codereview.chromium.org/2132663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:338:
dbus_setter->SetUpdateEngineClient(
Would it be sufficient to set the update engine client once in the
DeviceStatusCollectorTest constructor?

https://codereview.chromium.org/2132663002/diff/40001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/40001/components/policy/proto...
components/policy/proto/device_management_backend.proto:749: // This field is
set, only when an OS update is needed because of Kiosk
Nit: drop comma

https://codereview.chromium.org/2132663002/diff/40001/components/policy/proto...
components/policy/proto/device_management_backend.proto:768: optional string
new_platform_version = 2;
Please document this entry.

[xiyuan, 2016-07-11 16:29:18]

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

https://codereview.chromium.org/2132663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:960:
os_update_status->set_update_status(
On 2016/07/10 08:47:47, Thiemo Nagel wrote:
> Would it make sense to set_new_platform_version() here as well?

Done.

There could be edge cases that update_engine_status.new_version is a dummy
"0.0.0.0" for UPDATED_NEED_REBOOT status, e.g. when update engine is somehow
restarted, the new version is not restored. Added a comment to document that.

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

https://codereview.chromium.org/2132663002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:338:
dbus_setter->SetUpdateEngineClient(
On 2016/07/10 08:47:47, Thiemo Nagel wrote:
> Would it be sufficient to set the update engine client once in the
> DeviceStatusCollectorTest constructor?

Moved this and fake local state setup above to ctor.

https://codereview.chromium.org/2132663002/diff/40001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/40001/components/policy/proto...
components/policy/proto/device_management_backend.proto:749: // This field is
set, only when an OS update is needed because of Kiosk
On 2016/07/10 08:47:47, Thiemo Nagel wrote:
> Nit: drop comma

Done.

https://codereview.chromium.org/2132663002/diff/40001/components/policy/proto...
components/policy/proto/device_management_backend.proto:768: optional string
new_platform_version = 2;
On 2016/07/10 08:47:47, Thiemo Nagel wrote:
> Please document this entry.

Done.

[Thiemo Nagel, 2016-07-12 14:08:46]

lgtm % nits

https://codereview.chromium.org/2132663002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/2132663002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:315: //
Use FakeUpdateEngienClient.
Nit: typo

https://codereview.chromium.org/2132663002/diff/60001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:749: // This field is
set only when an OS update is needed because of Kiosk
nit: s/Kiosk/kiosk/

https://codereview.chromium.org/2132663002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:750: // app pinned OS
version of an updated Kiosk app is different from the
nit: s/Kiosk/kiosk/

[xiyuan, 2016-07-12 15:17:04]

https://codereview.chromium.org/2132663002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/2132663002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:315: //
Use FakeUpdateEngienClient.
On 2016/07/12 14:08:46, Thiemo Nagel wrote:
> Nit: typo

Done.

https://codereview.chromium.org/2132663002/diff/60001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2132663002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:749: // This field is
set only when an OS update is needed because of Kiosk
On 2016/07/12 14:08:46, Thiemo Nagel wrote:
> nit: s/Kiosk/kiosk/

Revised.

"Kiosk app pinned OS version" -> "the required platform version"

https://codereview.chromium.org/2132663002/diff/60001/components/policy/proto...
components/policy/proto/device_management_backend.proto:750: // app pinned OS
version of an updated Kiosk app is different from the
On 2016/07/12 14:08:46, Thiemo Nagel wrote:
> nit: s/Kiosk/kiosk/

Done.

[Thiemo Nagel, 2016-07-12 15:22:43]

Thanks!  Still lgtm.

[xiyuan, 2016-07-12 15:39:43]

On 2016/07/12 15:22:43, Thiemo Nagel wrote:
> Thanks!  Still lgtm.

Thanks for the quick review. :)

Made one more change to fix DeviceStatusCollectorNetworkInterfacesTest, failed
because fake local state set up is moved into base class ctor and it is not
calling base class's TearDown. And fixed by moving the clean up code from base
class DeviceStatusCollectorTest::TearDown to its dtor.

[xiyuan, 2016-07-12 20:02:00]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-07-12 20:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
jinzhang@chromium.org, jennyz@chromium.org, tnagel@chromium.org
Link to the patchset: https://codereview.chromium.org/2132663002/#ps100001
(title: "move clean up code from TearDown to dtor since set up code is in ctor
now and fix DeviceStatusColle…")

[commit-bot: I haz the power, 2016-07-12 20:03:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 22:38:35]

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

[commit-bot: I haz the power, 2016-07-12 22:38:37]

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

[xiyuan, 2016-07-13 15:24:44]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-07-13 15:25:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 16:56:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-13 16:57:23]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 16:59:55]

Description was changed from

==========
kiosk: Add status report for os update and running app

- Update chrome_device_settings.proto to include report setting
  for os_update_status and running_kiosk_app;
- Update device_management_backend.proto to include os_update_status
  and running_kiosk_app in DeviceStatusReportRequest;

BUG=577800
TEST=browser_tests --gtest_filter=DeviceStatusCollectorTest.*OsUpdateStatus*:
    DeviceStatusCollectorTest.*RunningKioskApp*
==========

to

==========
kiosk: Add status report for os update and running app

- Update chrome_device_settings.proto to include report setting
  for os_update_status and running_kiosk_app;
- Update device_management_backend.proto to include os_update_status
  and running_kiosk_app in DeviceStatusReportRequest;

BUG=577800
TEST=browser_tests --gtest_filter=DeviceStatusCollectorTest.*OsUpdateStatus*:
    DeviceStatusCollectorTest.*RunningKioskApp*

Committed: https://crrev.com/bdf91bc327487db842a0831fbf2247b671b3d988
Cr-Commit-Position: refs/heads/master@{#405196}
==========

[commit-bot: I haz the power, 2016-07-13 16:59:56]

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