base::Callback-ify policy::DeviceManagementService.

base::Callback-ify policy::DeviceManagementService.

Convert DeviceManagementService to base::Callback-style response
handlers. Support for DeviceManagementBackend will be dropped in a
subsequent CL but is kept intact in the interest of keeping this CL
focused.

BUG=chromium:108928
TEST=Compiles and passes tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116111

[Mattias Nissler (ping if slow), 2012-01-02 14:24:19]

This is the first refactoring change. I suggest you review starting from
device_management_service.h and device_management_service.cc.

[Joao da Silva, 2012-01-02 16:14:43]

Mostly ok! I just couldn't figure the lifetime of the URLFetchers, please see
the comments inline.

Notice that linux_chromeos is also failing because the parameters in the
dmserver requests aren't in a predictable order anymore. Either the test or
query_param_ has to be fixed.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_constants.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_constants.h (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:38: // These codes are sent in
the |error_code| field of the the
the the -> the

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:42: POLICY_FETCH_ERROR_NOT_FOUND
= 902
The other 2 enums have commas after the last entry. For consistency do it here
too

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:48: enum UserAffiliation {
I assume this is removed from the CloudPolicyDataStore in another CL?

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:8: #include <vector>
Nit: not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:13: #include
"base/stringprintf.h"
Nit: not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:14: #include
"base/sys_info.h"
Nit: not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:20: #include
"net/url_request/url_request_status.h"
Nit: these 4 (up to chrome_version_info.h) not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:23: #include
"chrome/browser/chromeos/system/statistics_provider.h"
Nit: not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.cc (left):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:178: delete job->first;
Why? It seems that the outstanding URLFetcher will then keep living, and might
invoke this->OnURLFetchComplete after deletion of the service.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:21: #include
"content/public/common/url_fetcher.h"
Nit: don't include twice

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:34: #include
"net/url_request/url_request_status.h"
Nit: don't include twice

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:363: break;
Some cases return and some break; please use the same statement in all cases.
Also, the { block scopes } aren't necessary, since there are no locals defined
(except in kSuccess). I don't mind leaving them for consistency, but I'd leave
them out :-)

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:587:
pending_jobs_.erase(entry);
If the job is removed, shouldn't its URLFetcher be deleted too? (delete
entry->first). Otherwise it might call back and then |pending_jobs_| doesn't
have it, and in any case it will leak.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.h (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:12: #include <vector>
Nit: not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:22: #include
"googleurl/src/gurl.h"
Nit: move to .cc

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:26: class URLRequestStatus;
Nit: not used

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:74: ParameterMap
query_params_;
This broke some tests on linux_chromeos. It used to be a vector, which preserved
the order of the params and always had "request" first.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service_unittest.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service_unittest.cc:26: using
testing::InvokeWithoutArgs;
Nit: not used

[Mattias Nissler (ping if slow), 2012-01-02 17:53:23]

Thanks for the thorough review. PTAL!

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_constants.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/02 16:14:43, Joao da Silva wrote:
> 2012

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_constants.h (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/02 16:14:43, Joao da Silva wrote:
> 2012

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:38: // These codes are sent in
the |error_code| field of the the
On 2012/01/02 16:14:43, Joao da Silva wrote:
> the the -> the

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:42: POLICY_FETCH_ERROR_NOT_FOUND
= 902
On 2012/01/02 16:14:43, Joao da Silva wrote:
> The other 2 enums have commas after the last entry. For consistency do it here
> too

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_constants.h:48: enum UserAffiliation {
On 2012/01/02 16:14:43, Joao da Silva wrote:
> I assume this is removed from the CloudPolicyDataStore in another CL?

Yes, see cleanup CL: http://codereview.chromium.org/9066005/

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:8: #include <vector>
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:13: #include
"base/stringprintf.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:14: #include
"base/sys_info.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:20: #include
"net/url_request/url_request_status.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: these 4 (up to chrome_version_info.h) not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:23: #include
"chrome/browser/chromeos/system/statistics_provider.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.cc (left):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:178: delete job->first;
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Why? It seems that the outstanding URLFetcher will then keep living, and might
> invoke this->OnURLFetchComplete after deletion of the service.

Must have deleted this by accident when making the changes to adjust job
ownership. Restored.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:21: #include
"content/public/common/url_fetcher.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: don't include twice

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:34: #include
"net/url_request/url_request_status.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: don't include twice

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:363: break;
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Some cases return and some break; please use the same statement in all cases.
> Also, the { block scopes } aren't necessary, since there are no locals defined
> (except in kSuccess). I don't mind leaving them for consistency, but I'd leave
> them out :-)

Switched break to return. I don't mind either way for the braces, so I removed
them.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:587:
pending_jobs_.erase(entry);
On 2012/01/02 16:14:43, Joao da Silva wrote:
> If the job is removed, shouldn't its URLFetcher be deleted too? (delete
> entry->first). Otherwise it might call back and then |pending_jobs_| doesn't
> have it, and in any case it will leak.

You are absolutely right. Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service.h (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/02 16:14:43, Joao da Silva wrote:
> 2012

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:12: #include <vector>
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:22: #include
"googleurl/src/gurl.h"
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: move to .cc

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:26: class URLRequestStatus;
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.h:74: ParameterMap
query_params_;
On 2012/01/02 16:14:43, Joao da Silva wrote:
> This broke some tests on linux_chromeos. It used to be a vector, which
preserved
> the order of the params and always had "request" first.

Sigh. Reverted.

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_service_unittest.cc (right):

http://codereview.chromium.org/8952027/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service_unittest.cc:26: using
testing::InvokeWithoutArgs;
On 2012/01/02 16:14:43, Joao da Silva wrote:
> Nit: not used

Done.

[Joao da Silva, 2012-01-02 18:14:26]

LGTM!

[commit-bot: I haz the power, 2012-01-02 19:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/8952027/6003

[commit-bot: I haz the power, 2012-01-02 20:44:15]

Change committed as 116111