Updated DeviceStatusReportRequest to contain new monitoring data.

Updated DeviceStatusReportRequest to contain new monitoring data.

Added code to DeviceStatusCollector to populate new NetworkState fields.

BUG=430908
Committed: https://crrev.com/1bae4cbc36222bc1f85d3b5e2392df5c55a9eec1
Cr-Commit-Position: refs/heads/master@{#308393}

[Andrew T Wilson (Slow), 2014-12-05 15:11:10]

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

[Andrew T Wilson (Slow), 2014-12-05 15:11:10]

A nice CL for you to review while you're in MTV :)

[Andrew T Wilson (Slow), 2014-12-05 15:27:25]

atwilson@chromium.org changed reviewers:
+ derat@chromium.org, ygorshenin@chromium.org

[Andrew T Wilson (Slow), 2014-12-05 15:27:25]

Yuri, PTAL at owner_settings_service_chromeos.cc and
device_settings_provider.cc.

Daniel: PTAL at cros_settings_names.*

[ygorshenin1, 2014-12-05 15:55:41]

LGTM to owner_settings_service_chromeos.cc and device_settings_provider.cc

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

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:466: for (state =
state_list.begin(); state != state_list.end(); ++state) {
nit: you can just write:
for (const auto& state : state_list) { ... }

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

https://codereview.chromium.org/776803003/diff/40001/components/policy/proto/...
components/policy/proto/device_management_backend.proto:495: IDLE=0;
nit: could you please surround '=' characters by spaces in the enum?

[Daniel Erat, 2014-12-05 16:17:35]

lgtm for cros_settings_names

[pneubeck (no reviews), 2014-12-09 03:00:10]

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

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:213: if
(!cros_settings_->GetBoolean(
IMO it would help a bit to drop a comment in front of these repeated 'if
(!cros_settings_->Get...)' that we report everything by default (if not
explicitly disabled).

DeviceLocation falls out of this pattern because it's not released and could be
moved to a separate block/be removed.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:406: // Maps flimflam
device connection status to proto enum constants.
we shouldn't use the name flimflam of the previous incarnation of the network
manager anymore.
flimflam -> shill

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:408: const char*
type_string;
type_string -> state_string

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:460: false,  //
configured_only
just verify with your intent here:
this will list _all_ networks. Which means any network that is configured or
that is visible, i.e. also visible but not configured networks.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:470: std::string
connection_state_string = (*state)->connection_state();
nit: const, use constructor instead of assignment

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:511:
em::DeviceStatusReportRequest* status) {
why is this empty function necessary?
if it will be filled eventually better just punt this part of the CL.

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

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:694: const
int signal_strength;
nit: const not required

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:701: // by
convention shill will not report a signal strength of "0" for a visible
nit: "0" -> 0
for consistency.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:766: for
(size_t i = 0; i < arraysize(kFakeNetworks); ++i) {
should be possible to replace by

for (const FakeNetworkState& fake_network : kFakeNetworks) {
 ...

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:773: true
/* visible */);
would you want to test a not visible network as well, since you include them in
the report?

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:855: for
(size_t i = 0; i < arraysize(kFakeNetworks); ++i) {
same here

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:860: for
(proto_state = status_.network_state().begin();
for (const em::NetworkState& proto_state : status_.network_state()) {
 ...

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

https://codereview.chromium.org/776803003/diff/40001/components/policy/proto/...
components/policy/proto/device_management_backend.proto:537: // Information
about a single disk volume.
are the changes down from here required for / used in this CL?

[Andrew T Wilson (Slow), 2014-12-11 00:09:55]

Philipp: PTAL - I have a few forward-looking items in this CL (proto definitions
that I'll fill in in future CLs, but I want to get them in all at once).

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

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:213: if
(!cros_settings_->GetBoolean(
On 2014/12/09 03:00:09, pneubeck wrote:
> IMO it would help a bit to drop a comment in front of these repeated 'if
> (!cros_settings_->Get...)' that we report everything by default (if not
> explicitly disabled).
> 
> DeviceLocation falls out of this pattern because it's not released and could
be
> moved to a separate block/be removed.

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:406: // Maps flimflam
device connection status to proto enum constants.
On 2014/12/09 03:00:10, pneubeck wrote:
> we shouldn't use the name flimflam of the previous incarnation of the network
> manager anymore.
> flimflam -> shill

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:408: const char*
type_string;
On 2014/12/09 03:00:09, pneubeck wrote:
> type_string -> state_string

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:460: false,  //
configured_only
On 2014/12/09 03:00:09, pneubeck wrote:
> just verify with your intent here:
> this will list _all_ networks. Which means any network that is configured or
> that is visible, i.e. also visible but not configured networks.

Hmmm. It's probably better to be a little tighter and only report configured
networks. Changed this, good catch.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:466: for (state =
state_list.begin(); state != state_list.end(); ++state) {
On 2014/12/05 15:55:41, ygorshenin1 wrote:
> nit: you can just write:
> for (const auto& state : state_list) { ... }

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:470: std::string
connection_state_string = (*state)->connection_state();
On 2014/12/09 03:00:09, pneubeck wrote:
> nit: const, use constructor instead of assignment

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:511:
em::DeviceStatusReportRequest* status) {
On 2014/12/09 03:00:10, pneubeck wrote:
> why is this empty function necessary?
> if it will be filled eventually better just punt this part of the CL.

I'd like to keep this in place because I'd like to keep the surrounding logic
(checking the report_hardware_status_ setting, etc in this CL). I've added a
TODO here.

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

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:694: const
int signal_strength;
On 2014/12/09 03:00:10, pneubeck wrote:
> nit: const not required

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:701: // by
convention shill will not report a signal strength of "0" for a visible
On 2014/12/09 03:00:10, pneubeck wrote:
> nit: "0" -> 0
> for consistency.

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:766: for
(size_t i = 0; i < arraysize(kFakeNetworks); ++i) {
On 2014/12/09 03:00:10, pneubeck wrote:
> should be possible to replace by
> 
> for (const FakeNetworkState& fake_network : kFakeNetworks) {
>  ...

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:773: true
/* visible */);
On 2014/12/09 03:00:10, pneubeck wrote:
> would you want to test a not visible network as well, since you include them
in
> the report?

Added a test for this (we have a disconnected network and I've made it
non-visible).

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:855: for
(size_t i = 0; i < arraysize(kFakeNetworks); ++i) {
On 2014/12/09 03:00:10, pneubeck wrote:
> same here

Done.

https://codereview.chromium.org/776803003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:860: for
(proto_state = status_.network_state().begin();
On 2014/12/09 03:00:10, pneubeck wrote:
> for (const em::NetworkState& proto_state : status_.network_state()) {
>  ...

Done.

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

https://codereview.chromium.org/776803003/diff/40001/components/policy/proto/...
components/policy/proto/device_management_backend.proto:495: IDLE=0;
On 2014/12/05 15:55:41, ygorshenin1 wrote:
> nit: could you please surround '=' characters by spaces in the enum?

Done.

https://codereview.chromium.org/776803003/diff/40001/components/policy/proto/...
components/policy/proto/device_management_backend.proto:537: // Information
about a single disk volume.
On 2014/12/09 03:00:10, pneubeck wrote:
> are the changes down from here required for / used in this CL?

Not strictly required, but I'm trying to put all of the reporting proto changes
in a single CL, even though I'm not filling in every field yet.

[pneubeck (no reviews), 2014-12-12 02:02:47]

LGTM!! with a few minor comments

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

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:474: std::string
connection_state_string(state->connection_state());
optional nit: you could still change the type to 'const std::string'

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

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:772: //
Non-visible networks report a disconnected state, so test
maybe I'm just misreading the second part of this sentence or it's not matching
the code below.
you could just drop the second part.

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:796: //
list of networks because it doesn't have a profile specified.
nit: 'list of networks' -> 'reported list of networks'

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:886:
google::protobuf::RepeatedPtrField<em::NetworkState>::const_iterator
this can be removed

[Andrew T Wilson (Slow), 2014-12-15 17:00:49]

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

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.cc:474: std::string
connection_state_string(state->connection_state());
On 2014/12/12 02:02:47, pneubeck wrote:
> optional nit: you could still change the type to 'const std::string'

Done.

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

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:772: //
Non-visible networks report a disconnected state, so test
On 2014/12/12 02:02:47, pneubeck wrote:
> maybe I'm just misreading the second part of this sentence or it's not
matching
> the code below.
> you could just drop the second part.

Done.

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:796: //
list of networks because it doesn't have a profile specified.
On 2014/12/12 02:02:47, pneubeck wrote:
> nit: 'list of networks' -> 'reported list of networks'

Done.

https://codereview.chromium.org/776803003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:886:
google::protobuf::RepeatedPtrField<em::NetworkState>::const_iterator
On 2014/12/12 02:02:47, pneubeck wrote:
> this can be removed

Done.

[Andrew T Wilson (Slow), 2014-12-15 17:00:57]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:01:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/776803003/100001

[commit-bot: I haz the power, 2014-12-15 19:39:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-15 19:41:29]

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