Refactored DeviceStatusCollector to enable truely asynchronous status queries

Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function. They are always called together and combining them makes StatusGetter in status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status is requested. Before, the data was sampled regularly and only the last sample was used.
Removed unused geo location reporting code.
Deprecated device_location field in protobuf.
Changed thread-checking to use ThreadChecker class.

BUG=639372
Committed: https://crrev.com/6e4082c0fc2425bb6f040e29baae4362246ef93d
Cr-Commit-Position: refs/heads/master@{#421475}

[ljusten (tachyonic), 2016-09-06 14:12:47]

The CQ bit was checked by ljusten@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 14:12:56]

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

[ljusten (tachyonic), 2016-09-06 14:20:14]

ljusten@chromium.org changed reviewers:
+ atwilson@chromium.org, tnagel@chromium.org

[ljusten (tachyonic), 2016-09-06 14:20:14]

Please review "DeviceStatusCollector - Asynchronicity II".

[ljusten (tachyonic), 2016-09-06 14:20:49]

tnagel@chromium.org, atwilson@chromium.org, please review "DeviceStatusCollector
- Asynchronicity II".

[commit-bot: I haz the power, 2016-09-06 15:42:28]

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

[commit-bot: I haz the power, 2016-09-06 15:42:30]

Dry run: This issue passed the CQ dry run.

[ljusten (tachyonic), 2016-09-09 09:27:05]

Description was changed from

==========
Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function.
They are always called together and combining them makes StatusGetter in
status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status
is requested. Before, the data was sampled regularly and only the last sample
was used.
Removed unused geo location reporting code.
Changed thread-checking to use ThreadChecker class.

BUG=639372
==========

to

==========
Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function.
They are always called together and combining them makes StatusGetter in
status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status
is requested. Before, the data was sampled regularly and only the last sample
was used.
Removed unused geo location reporting code.
Deprecated device_location field in protobuf.
Changed thread-checking to use ThreadChecker class.

BUG=639372
==========

[ljusten (tachyonic), 2016-09-22 19:48:13]

The CQ bit was checked by ljusten@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 19:48:41]

Dry run: 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-09-22 20:54:49]

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

[commit-bot: I haz the power, 2016-09-22 20:54:50]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-09-26 16:33:42]

Code LGTM, but some comments on comments/documentation.

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

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:265: // Helper class
for state tracking of async status queries.
This does clever stuff in the destructor - you should have class-level
documentation of that behavior and how this class is intended to be used.

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:273:
device_status_(base::MakeUnique<em::DeviceStatusReportRequest>()),
I thought unique_ptr initialized itself to null - do you have to have an
explicit initialization (and nullptr isn't sufficient, you actually need a
MakeUnique)?

Not saying this is wrong - I haven't written enough code with unique_ptr to have
a good grasp of the semantics, just checking.

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:313: // Queues an
async callback to query CPU temperator information.
nit: temperator->temperature

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:744:
status->set_boot_mode("Dev");
This is all fine, although note that your patch is significantly larger because
you decided to rename this variable. I might've split these into two patches
(one for a mechanical renaming, one for logic changes).

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

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.h:72: // collected
asynchronously in GetStatusAsync.
Is GetStatusAsync() even a function any more?

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.h:90: // callback. Null
pointers passed into the response indicate errors or that
I think you should document the meaning of null pointers up in the definition of
StatusCallback.

[Thiemo Nagel, 2016-09-27 16:39:30]

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

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:273:
device_status_(base::MakeUnique<em::DeviceStatusReportRequest>()),
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> I thought unique_ptr initialized itself to null - do you have to have an
> explicit initialization (and nullptr isn't sufficient, you actually need a
> MakeUnique)?

+1 to this.  Also, for new code we prefer in-class initialization over
in-constructor initialization.  But a unique_ptr doesn't require any
initialization at all.

[ljusten (tachyonic), 2016-09-27 16:53:55]

Incorporated comments

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

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:265: // Helper class
for state tracking of async status queries.
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> This does clever stuff in the destructor - you should have class-level
> documentation of that behavior and how this class is intended to be used.

Done.

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:273:
device_status_(base::MakeUnique<em::DeviceStatusReportRequest>()),
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> I thought unique_ptr initialized itself to null - do you have to have an
> explicit initialization (and nullptr isn't sufficient, you actually need a
> MakeUnique)?
> 
> Not saying this is wrong - I haven't written enough code with unique_ptr to
have
> a good grasp of the semantics, just checking.

unique_ptr does initialize itself to nullptr, but in this case I want to create
empty status blobs because the logic is easier this way. The logic roughly is:

- Create status blobs
- Call all Get* methods and record whether they actually wrote status
- If none of them wrote any status, delete the blob again

This will create a blob and destroy it again if none of the Get* methods reports
anything, but this is much easier and cleaner to do than the alternative:

- Initialize status blobs with null
- Check whether any of the Get* methods wants to report anything
- If so, create status blob and actually call the Get* methods

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:313: // Queues an
async callback to query CPU temperator information.
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> nit: temperator->temperature

Done.

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:744:
status->set_boot_mode("Dev");
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> This is all fine, although note that your patch is significantly larger
because
> you decided to rename this variable. I might've split these into two patches
> (one for a mechanical renaming, one for logic changes).

Acknowledged.

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

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.h:72: // collected
asynchronously in GetStatusAsync.
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> Is GetStatusAsync() even a function any more?

Thanks, should be GetDeviceAndSessionStatusAsync.

https://codereview.chromium.org/2314813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.h:90: // callback. Null
pointers passed into the response indicate errors or that
On 2016/09/26 16:33:42, Andrew T Wilson (Slow) wrote:
> I think you should document the meaning of null pointers up in the definition
of
> StatusCallback.

Done.

[ljusten (tachyonic), 2016-09-28 07:33:15]

The CQ bit was checked by ljusten@chromium.org

[ljusten (tachyonic), 2016-09-28 07:33:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2314813002/#ps80001
(title: "Initialize *status_ in the C++11'y way")

[commit-bot: I haz the power, 2016-09-28 07:33:23]

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-09-28 08:13:08]

Description was changed from

==========
Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function.
They are always called together and combining them makes StatusGetter in
status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status
is requested. Before, the data was sampled regularly and only the last sample
was used.
Removed unused geo location reporting code.
Deprecated device_location field in protobuf.
Changed thread-checking to use ThreadChecker class.

BUG=639372
==========

to

==========
Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function.
They are always called together and combining them makes StatusGetter in
status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status
is requested. Before, the data was sampled regularly and only the last sample
was used.
Removed unused geo location reporting code.
Deprecated device_location field in protobuf.
Changed thread-checking to use ThreadChecker class.

BUG=639372
==========

[commit-bot: I haz the power, 2016-09-28 08:13:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-28 08:14:48]

Description was changed from

==========
Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function.
They are always called together and combining them makes StatusGetter in
status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status
is requested. Before, the data was sampled regularly and only the last sample
was used.
Removed unused geo location reporting code.
Deprecated device_location field in protobuf.
Changed thread-checking to use ThreadChecker class.

BUG=639372
==========

to

==========
Refactored DeviceStatusCollector to enable truely asynchronous status queries

Combined GetDeviceStatusAsync and GetDeviceSessionStatusAsync into one function.
They are always called together and combining them makes StatusGetter in
status_uploader.h/cc unnecessary.
Added helper class to keep state for asynchronous status queries.
Disk volume info and CPU temp info are now collected asynchronously when status
is requested. Before, the data was sampled regularly and only the last sample
was used.
Removed unused geo location reporting code.
Deprecated device_location field in protobuf.
Changed thread-checking to use ThreadChecker class.

BUG=639372

Committed: https://crrev.com/6e4082c0fc2425bb6f040e29baae4362246ef93d
Cr-Commit-Position: refs/heads/master@{#421475}
==========

[commit-bot: I haz the power, 2016-09-28 08:14:49]

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