Refactored device status uploading out of policy fetch infrastructure.

Refactored device status uploading out of policy fetch infrastructure.

Created StatusUploader class to manage the uploading of device status.

Refactored status uploading functionality in cloud policy infrastructure - the
timing and contents of the device upload are now controlled by StatusUploader,
while CloudPolicyClient exposes an API to create the upload job.

BUG=430908
Committed: https://crrev.com/7dc108c5f3e04ea68ad1444ab3910dbfca9da92a
Cr-Commit-Position: refs/heads/master@{#313535}

[Andrew T Wilson (Slow), 2015-01-22 15:56:18]

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

[Andrew T Wilson (Slow), 2015-01-22 15:56:19]

PTAL

[Mattias Nissler (ping if slow), 2015-01-23 13:46:35]

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.h:78:
enterprise_management::SessionStatusReportRequest* status);
I don't think we're using this at all, remove?

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:43: // status upload if it
changes.
I don't understand this TODO. Does it refer to adding the currently active user
to device status uploads? Or does it refer to user status uploads? The latter
don't belong here IMHO as they'd use a different CloudPolicyClient and
StatusUploader instance.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:58: base::Unretained(this)));
#include "base/bind_helpers.h"

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:71: bool have_session_status =
collector_->GetSessionStatus(&session_status);
Hm, session status doesn't make sense for device status reporting, just remove
this?

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:78: request_job_ =
client_->CreateUploadStatusJob(
You're assuming that |client_| is registered with the server here. That should
either be documented if we can safely assume the CloudPolicyClient will always
have a client ID / DM token available (I haven't checked), or alternatively
StatusUploader could register as an observer with CloudPolicyClient to be
notified of (un-)registration and take the proper action. The latter removes any
assumptions about the state of the CloudPolicyClient dependency, so is probably
preferable from a code hygiene perspective.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/status_uploader.h (right):

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.h:35: PrefService* local_state,
forward-declare?

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.h:38: const
scoped_refptr<base::SequencedTaskRunner>& task_runner);
#include "base/memory/ref_counted.h"

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:88:
EXPECT_TRUE(upload_request);
This needs to be ASSERT_TRUE, otherwise you crash below.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:100:
chromeos::ScopedTestCrosSettings test_cros_settings_;
It's kinda sad that we have to bring up this heavy-wait BS just to test the
status uploader...

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:108: new
MockDeviceStatusCollector(&prefs_));
All the tests need a collector - move it to the fixture?

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:111:
CreateStatusUploader(collector.Pass()).Pass());
The additional boilerplate hardly justifies why you want a CreateStatusUploader
helper instead of just doing:

StatusUploader uploader(&prefs_, client_.get(), collector.Pass(), task_runner_);

Same below.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:117:
TEST_F(StatusUploaderTest, ResetTimerAfterStatusCollection) {
Would it make sense to check the delays as well to make sure they're not too
short (assuming TestSimpleTaskRunner gives you access to them)?

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/policy/te...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:327: return (response[0], body)
Thanks for rescuing this snippet.

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.h:136: const
enterprise_management::SessionStatusReportRequest* session_status);
Is there a good reason to return the request job instead of just taking a
callback that will be invoked once the operation completes? The latter would be
consistent with UploadCertificate() above at least and hides the existence of
request job objects (which can be considered an implementation detail).

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.h:137: 
remove extra blank line

[Andrew T Wilson (Slow), 2015-01-23 19:16:19]

PTAL

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.h:78:
enterprise_management::SessionStatusReportRequest* status);
On 2015/01/23 13:46:34, Mattias Nissler wrote:
> I don't think we're using this at all, remove?

We are using this. DeviceStatusCollector currently doesn't fill it in, but I
have a CL in progress to fill it in. Doesn't make sense to remove this existing
API only to re-add it in the next CL.

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:43: // status upload if it
changes.
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> I don't understand this TODO. Does it refer to adding the currently active
user
> to device status uploads? Or does it refer to user status uploads? The latter
> don't belong here IMHO as they'd use a different CloudPolicyClient and
> StatusUploader instance.

I'm planning to upload session status using the device cloud policy client.
There's no reason not to do this, and in fact for kiosk sessions there isn't a
user cloud policy client, right?

Basically this TODO is saying that we want to trigger a status upload after a
session starts/ends because various things (accessible drives/networks, session
status) will change.

That said, I've removed the TODO because it really doesn't add any value as you
pointed out.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:58: base::Unretained(this)));
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> #include "base/bind_helpers.h"

Done.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:71: bool have_session_status =
collector_->GetSessionStatus(&session_status);
On 2015/01/23 13:46:34, Mattias Nissler wrote:
> Hm, session status doesn't make sense for device status reporting, just remove
> this?

They are uploaded together. That's why this class is called StatusUploader and
not DeviceStatusUploader :)

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.cc:78: request_job_ =
client_->CreateUploadStatusJob(
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> You're assuming that |client_| is registered with the server here. That should
> either be documented if we can safely assume the CloudPolicyClient will always
> have a client ID / DM token available (I haven't checked), or alternatively
> StatusUploader could register as an observer with CloudPolicyClient to be
> notified of (un-)registration and take the proper action. The latter removes
any
> assumptions about the state of the CloudPolicyClient dependency, so is
probably
> preferable from a code hygiene perspective.

Yes, I'm assuming it's registered already - I'll add a DCHECK() to enforce this.
On un-registration, the idea is that DCPM will blow away the client and status
uploader and create a new one.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/status_uploader.h (right):

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.h:35: PrefService* local_state,
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> forward-declare?

Done.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader.h:38: const
scoped_refptr<base::SequencedTaskRunner>& task_runner);
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> #include "base/memory/ref_counted.h"

Done.

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:88:
EXPECT_TRUE(upload_request);
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> This needs to be ASSERT_TRUE, otherwise you crash below.

Done.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:100:
chromeos::ScopedTestCrosSettings test_cros_settings_;
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> It's kinda sad that we have to bring up this heavy-wait BS just to test the
> status uploader...

Yeah, this is the cost of having a CrosSettings singleton.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:108: new
MockDeviceStatusCollector(&prefs_));
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> All the tests need a collector - move it to the fixture?

Done.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:111:
CreateStatusUploader(collector.Pass()).Pass());
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> The additional boilerplate hardly justifies why you want a
CreateStatusUploader
> helper instead of just doing:
> 
> StatusUploader uploader(&prefs_, client_.get(), collector.Pass(),
task_runner_);
> 
> Same below.

Yep, it used to do more stuff. Happy to un-refactor this.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:117:
TEST_F(StatusUploaderTest, ResetTimerAfterStatusCollection) {
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> Would it make sense to check the delays as well to make sure they're not too
> short (assuming TestSimpleTaskRunner gives you access to them)?

I originally tried this and it's hard to do well - TestSimpleTaskRunner gives us
a "how long until the next task will fire" but if the test is running slowly
this will be a varying amount less than the expected value, and it felt fragile
to have something like (DCHECK_GT(time_remaining, expected_time -
FUDGE_FACTOR_LETS_HOPE_THE_TEST_DOES_NOT_RUN_SLOWER_THAN_THIS)).

So I decided it's better not to test the delay values - if you have an idea of
how to do this more cleanly let me know and I'll add this.

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/policy/te...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:327: return (response[0], body)
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> Thanks for rescuing this snippet.

Yep :)

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.h:136: const
enterprise_management::SessionStatusReportRequest* session_status);
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> Is there a good reason to return the request job instead of just taking a
> callback that will be invoked once the operation completes? The latter would
be
> consistent with UploadCertificate() above at least and hides the existence of
> request job objects (which can be considered an implementation detail).

Because I want to be able to cancel the request job if StatusUploader goes away.
Fundamentally, the request should have the lifetime of the StatusUploader
object, not the CloudPolicyClient.

Agreed that it would be nice to hide the type of request as an implementation
detail, but building the level of abstraction that would still let
StatusUploader control the lifetime of the request is way more work than this
abstraction is worth. And I don't think it's a particularly leaky abstraction to
expose the fact that DeviceManagementService is handling the network connection.

If you prefer, I could move the StartJob() call into CloudPolicyClient, though -
let me know, or if you have an alternative implementation.

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.h:137: 
On 2015/01/23 13:46:35, Mattias Nissler wrote:
> remove extra blank line

Done.

[Mattias Nissler (ping if slow), 2015-01-26 10:37:02]

I think we need to get the session status upload question sorted - see inline
comment.

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.h:78:
enterprise_management::SessionStatusReportRequest* status);
On 2015/01/23 19:16:18, Andrew T Wilson wrote:
> On 2015/01/23 13:46:34, Mattias Nissler wrote:
> > I don't think we're using this at all, remove?
> 
> We are using this. DeviceStatusCollector currently doesn't fill it in, but I
> have a CL in progress to fill it in. Doesn't make sense to remove this
existing
> API only to re-add it in the next CL.

So the idea was that SessionStatusReportRequest would only ever be used with
user policy registrations. The lines are a bit blurred for Kiosk Apps and Public
Sessions, but these do have their separate CloudPolicyClients as well - probably
makes more sense to hook up session reporting there, in particular in case we
ever want to reuse some reporting for regular sessions as well. Btw. - what data
is going to be uploaded in session status?

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:117:
TEST_F(StatusUploaderTest, ResetTimerAfterStatusCollection) {
On 2015/01/23 19:16:19, Andrew T Wilson wrote:
> On 2015/01/23 13:46:35, Mattias Nissler wrote:
> > Would it make sense to check the delays as well to make sure they're not too
> > short (assuming TestSimpleTaskRunner gives you access to them)?
> 
> I originally tried this and it's hard to do well - TestSimpleTaskRunner gives
us
> a "how long until the next task will fire" but if the test is running slowly
> this will be a varying amount less than the expected value, and it felt
fragile
> to have something like (DCHECK_GT(time_remaining, expected_time -
> FUDGE_FACTOR_LETS_HOPE_THE_TEST_DOES_NOT_RUN_SLOWER_THAN_THIS)).
> 
> So I decided it's better not to test the delay values - if you have an idea of
> how to do this more cleanly let me know and I'll add this.

cloud_policy_refresh_scheduler_unittest.cc has some docs on how to do this
properly:
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...

The idea is basically to measure time before and after you trigger the code
under test and then allow the entire window for valid next refresh delays.

Anyhow, your call - I don't insist on timing tests in this case.

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.h:136: const
enterprise_management::SessionStatusReportRequest* session_status);
On 2015/01/23 19:16:19, Andrew T Wilson wrote:
> On 2015/01/23 13:46:35, Mattias Nissler wrote:
> > Is there a good reason to return the request job instead of just taking a
> > callback that will be invoked once the operation completes? The latter would
> be
> > consistent with UploadCertificate() above at least and hides the existence
of
> > request job objects (which can be considered an implementation detail).
> 
> Because I want to be able to cancel the request job if StatusUploader goes
away.
> Fundamentally, the request should have the lifetime of the StatusUploader
> object, not the CloudPolicyClient.
> 
> Agreed that it would be nice to hide the type of request as an implementation
> detail, but building the level of abstraction that would still let
> StatusUploader control the lifetime of the request is way more work than this
> abstraction is worth. And I don't think it's a particularly leaky abstraction
to
> expose the fact that DeviceManagementService is handling the network
connection.
> 
> If you prefer, I could move the StartJob() call into CloudPolicyClient, though
-
> let me know, or if you have an alternative implementation.

What's the benefit of cancelling the job, instead of just using a WeakPtr in the
callback? The request probably hits the net quickly, so cancelling while it's
pending on the server doesn't seem to give you any benefit? In that light, a
callback bound to a WeakPtr seems the more commonly used solution.

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:34:
DCHECK(client->is_registered());
If you don't think it's worthwhile to make this code work with
CloudPolicyClients in any state, please at least add a comment here saying what
needs to be done in order drop the is_registered() assumption in the future.

Background: One major motivation for the big re-write of the cloud policy code
was that there were lots of hidden dependencies and assumptions about various
pieces being in certain states across the entire cloud policy implementation,
leading to bugs and the inability to refactor anything. That's why we now have
only a handful of core classes (CloudPolicyClient and CloudPolicyStore for the
most part) with observer interfaces, such that you can add aspects (such as
status uploading, invalidations, etc.) in a way that doesn't affect other stuff
that is going on. Turning this around, new components shouldn't fall over if
they find the system in a state they don't expect.

I'm willing to accept that unregistered clients are a non-reality for now, but
if they turn out to become one then we need to handle the fallout here, and not
by having embedding code implement only-start-status-reporting-if-registered
kludges - hence the request for the comment.

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.h (right):

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.h:35: // valid and registered
hrough the lifetime of this StatusUploader
typo

[Andrew T Wilson (Slow), 2015-01-28 13:19:12]

PTAL - I think I addressed all your requests: added timing tests, commented on
need to have a registered client, changed CloudPolicyClient interface to not
expose DMRequestJob.

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_status_collector.h:78:
enterprise_management::SessionStatusReportRequest* status);
On 2015/01/26 10:37:01, Mattias Nissler wrote:
> On 2015/01/23 19:16:18, Andrew T Wilson wrote:
> > On 2015/01/23 13:46:34, Mattias Nissler wrote:
> > > I don't think we're using this at all, remove?
> > 
> > We are using this. DeviceStatusCollector currently doesn't fill it in, but I
> > have a CL in progress to fill it in. Doesn't make sense to remove this
> existing
> > API only to re-add it in the next CL.
> 
> So the idea was that SessionStatusReportRequest would only ever be used with
> user policy registrations. The lines are a bit blurred for Kiosk Apps and
Public
> Sessions, but these do have their separate CloudPolicyClients as well -
probably
> makes more sense to hook up session reporting there, in particular in case we
> ever want to reuse some reporting for regular sessions as well. Btw. - what
data
> is going to be uploaded in session status?

Per our discussion with Jin & Bin, I'm going to continue to upload device-level
sessions (public + kiosk) with the device status. We'll upload user session info
via a separate request if/when we add that functionality.

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

https://codereview.chromium.org/845313008/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:117:
TEST_F(StatusUploaderTest, ResetTimerAfterStatusCollection) {
On 2015/01/26 10:37:02, Mattias Nissler wrote:
> On 2015/01/23 19:16:19, Andrew T Wilson wrote:
> > On 2015/01/23 13:46:35, Mattias Nissler wrote:
> > > Would it make sense to check the delays as well to make sure they're not
too
> > > short (assuming TestSimpleTaskRunner gives you access to them)?
> > 
> > I originally tried this and it's hard to do well - TestSimpleTaskRunner
gives
> us
> > a "how long until the next task will fire" but if the test is running slowly
> > this will be a varying amount less than the expected value, and it felt
> fragile
> > to have something like (DCHECK_GT(time_remaining, expected_time -
> > FUDGE_FACTOR_LETS_HOPE_THE_TEST_DOES_NOT_RUN_SLOWER_THAN_THIS)).
> > 
> > So I decided it's better not to test the delay values - if you have an idea
of
> > how to do this more cleanly let me know and I'll add this.
> 
> cloud_policy_refresh_scheduler_unittest.cc has some docs on how to do this
> properly:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...
> 
> The idea is basically to measure time before and after you trigger the code
> under test and then allow the entire window for valid next refresh delays.
> 
> Anyhow, your call - I don't insist on timing tests in this case.

Done.

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/845313008/diff/80001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_client.h:136: const
enterprise_management::SessionStatusReportRequest* session_status);
On 2015/01/26 10:37:02, Mattias Nissler wrote:
> On 2015/01/23 19:16:19, Andrew T Wilson wrote:
> > On 2015/01/23 13:46:35, Mattias Nissler wrote:
> > > Is there a good reason to return the request job instead of just taking a
> > > callback that will be invoked once the operation completes? The latter
would
> > be
> > > consistent with UploadCertificate() above at least and hides the existence
> of
> > > request job objects (which can be considered an implementation detail).
> > 
> > Because I want to be able to cancel the request job if StatusUploader goes
> away.
> > Fundamentally, the request should have the lifetime of the StatusUploader
> > object, not the CloudPolicyClient.
> > 
> > Agreed that it would be nice to hide the type of request as an
implementation
> > detail, but building the level of abstraction that would still let
> > StatusUploader control the lifetime of the request is way more work than
this
> > abstraction is worth. And I don't think it's a particularly leaky
abstraction
> to
> > expose the fact that DeviceManagementService is handling the network
> connection.
> > 
> > If you prefer, I could move the StartJob() call into CloudPolicyClient,
though
> -
> > let me know, or if you have an alternative implementation.
> 
> What's the benefit of cancelling the job, instead of just using a WeakPtr in
the
> callback? The request probably hits the net quickly, so cancelling while it's
> pending on the server doesn't seem to give you any benefit? In that light, a
> callback bound to a WeakPtr seems the more commonly used solution.

Done.

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:34:
DCHECK(client->is_registered());
On 2015/01/26 10:37:02, Mattias Nissler wrote:
> If you don't think it's worthwhile to make this code work with
> CloudPolicyClients in any state, please at least add a comment here saying
what
> needs to be done in order drop the is_registered() assumption in the future.
> 
> Background: One major motivation for the big re-write of the cloud policy code
> was that there were lots of hidden dependencies and assumptions about various
> pieces being in certain states across the entire cloud policy implementation,
> leading to bugs and the inability to refactor anything. That's why we now have
> only a handful of core classes (CloudPolicyClient and CloudPolicyStore for the
> most part) with observer interfaces, such that you can add aspects (such as
> status uploading, invalidations, etc.) in a way that doesn't affect other
stuff
> that is going on. Turning this around, new components shouldn't fall over if
> they find the system in a state they don't expect.
> 
> I'm willing to accept that unregistered clients are a non-reality for now, but
> if they turn out to become one then we need to handle the fallout here, and
not
> by having embedding code implement only-start-status-reporting-if-registered
> kludges - hence the request for the comment.

Comment added.

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.h (right):

https://codereview.chromium.org/845313008/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.h:35: // valid and registered
hrough the lifetime of this StatusUploader
On 2015/01/26 10:37:02, Mattias Nissler wrote:
> typo

Done.

[Mattias Nissler (ping if slow), 2015-01-28 15:44:06]

This looks mostly good now, just some nits. LGTM for the case of any test code
changes not justifying re-review.

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:36: // should be destroyed by
the owner if the client is ever unregistered. If we
nit: "should be destroyed" is ambiguous and might easily be understood to be "by
design", which is not the case. Can you clarify to say "and it is currently safe
to assume that the client doesn't unregister while StatusUploader is alive".

Similarly, can you change "If we want to support unregistered clients," to "If
future changes result in StatusUploader's lifetime colliding with unregistration
events,"

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:101: // of pending items.
Tangential comment: If we allow multiple status upload jobs to be active, then
OnSubmittedSuccessfully might discard data in the following case:

1. Call UploadStatus()
2. Call UploadStatus() again, resulting in an upload that contains all data
collected by the first call + some more.
3. The first UploadStatus() succeeds and results in OnSubmittedSuccessfully()
being invoked, the collector discards its accumulated state.
4. The second request actually fails and ever reaches the server. This means the
data present in the second request but not the first is lost.

I think you'll want to consider this in the follow-up change to allow more than
one active request in CloudPolicyClient.

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader_unittest.cc (right):

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:83: // Now invoke the
response.
nit: It's probably clearer to just record the callback in the UploadDeviceState
expectation using ::testing::Assign(&callback) and then directly invoke the
recorded callback here. That'd also avoid the StatusCallbackSuccess helper.

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:91: //
MockResponseForPendingUploadTask() above. So the next task should be
nit: stale function name reference

[Andrew T Wilson (Slow), 2015-01-28 16:13:09]

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.cc (right):

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:36: // should be destroyed by
the owner if the client is ever unregistered. If we
On 2015/01/28 15:44:06, Mattias Nissler wrote:
> nit: "should be destroyed" is ambiguous and might easily be understood to be
"by
> design", which is not the case. Can you clarify to say "and it is currently
safe
> to assume that the client doesn't unregister while StatusUploader is alive".
> 
> Similarly, can you change "If we want to support unregistered clients," to "If
> future changes result in StatusUploader's lifetime colliding with
unregistration
> events,"

Done.

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.cc:101: // of pending items.
On 2015/01/28 15:44:06, Mattias Nissler wrote:
> Tangential comment: If we allow multiple status upload jobs to be active, then
> OnSubmittedSuccessfully might discard data in the following case:
> 
> 1. Call UploadStatus()
> 2. Call UploadStatus() again, resulting in an upload that contains all data
> collected by the first call + some more.
> 3. The first UploadStatus() succeeds and results in OnSubmittedSuccessfully()
> being invoked, the collector discards its accumulated state.
> 4. The second request actually fails and ever reaches the server. This means
the
> data present in the second request but not the first is lost.
> 
> I think you'll want to consider this in the follow-up change to allow more
than
> one active request in CloudPolicyClient.

Yeah, I'll probably reset the WeakPtrFactory so only one callback will be
invoked at a time (the latest one). In any case, OnSubmittedSuccessfully() is
known to be leaky, since it clears the current cache, including any entries that
may have been collected between the last call to GetDeviceStatus() and the
success notification, so I'm not that concerned about this in general.

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader_unittest.cc (right):

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:83: // Now invoke the
response.
On 2015/01/28 15:44:06, Mattias Nissler wrote:
> nit: It's probably clearer to just record the callback in the
UploadDeviceState
> expectation using ::testing::Assign(&callback) and then directly invoke the
> recorded callback here. That'd also avoid the StatusCallbackSuccess helper.

OK, done. The only reason I didn't is because I don't know all of the gmock best
practices so didn't know the best way to do that. Thanks for the pointer.

https://codereview.chromium.org/845313008/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader_unittest.cc:91: //
MockResponseForPendingUploadTask() above. So the next task should be
On 2015/01/28 15:44:06, Mattias Nissler wrote:
> nit: stale function name reference

Done.

[Andrew T Wilson (Slow), 2015-01-28 16:13:15]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2015-01-28 16:14:05]

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

[commit-bot: I haz the power, 2015-01-28 17:25:55]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-28 17:27:30]

Patchset 9 (id:??) landed as
https://crrev.com/7dc108c5f3e04ea68ad1444ab3910dbfca9da92a
Cr-Commit-Position: refs/heads/master@{#313535}