Use a service to create device management backends.

Use a service to create device management backends.

Move responsibility for creating backend objects to a device management service
object that lives in the profile. Doing allows us to make use of the profile's
request context, which has the advantage over the independent request context
implementation that stuff is more efficient (i.e. we need to only resolve the
proxy once).

BUG=63608
TEST=device_management_service_(unit|browser)test.cc

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

[Mattias Nissler (ping if slow), 2010-11-18 15:26:59]

This is a follow-up to http://codereview.chromium.org/5178005/ and replaces
http://codereview.chromium.org/5026001/, which is no obsolete.

Markus, Nico: Please review.
Others: FYI, shouldn't make any visible changes to the stuff you're working on.

[Nico, 2010-11-18 15:36:33]

Maybe pin this to the bug the other cl was pinned to.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/profile.h
File chrome/browser/profile.h (right):

http://codereview.chromium.org/5153002/diff/1/chrome/browser/profile.h#newcod...
chrome/browser/profile.h:481: virtual policy::DeviceManagementService*
GetDeviceManagementService() = 0;
Do you think it might be worth it to have a PolicyProvider class which contains
"all" this policy stuff (currently just DeviceManagementService and
DeviceManagementPolicyProvider), so that the profile has only one accessor for
all the policy stuff?

[markusheintz_, 2010-11-18 16:12:49]

Some quick stuff.

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

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:23: const char
kChromePolicyScope[] = "cros/device";
please replace this with "chromeos/device"

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

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:42: const char
kServiceParamDeviceID[] = "deviceid";
s/deviceid/device/

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:43: const char
kServiceParamAgent[] = "agent";
Please add:

const char kServiceParamAppType[] = "apptype";

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:46: const char
kServiceValueDeviceType[] = "Chrome";
Pls add:

const char kServiceValueAppType[] = "Chrome";

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:176: // A wrapper that
implements the actual backend interface. This is separate from
This class could live in its own .h .cc file.

Wdyt? Whould help make everything more readable.We could of course address this
in a separate CL.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:217: // Represents a job run
by the service. This contains the common code,
I somehow think the DeviceManagementJob classes should live in their own file,
this would make this file easier to read.

wdyt? We could of course address this in a separate CL.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:244:
query_params_.Put(kServiceParamDeviceType, kServiceValueDeviceType);
Pls add:

query_params_.Put(kServiceParamAppType, kServiceValueAppType);

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:374: virtual void
ProcessError(DeviceManagementBackend::ErrorCode error) {
Why naming ProcessError if the delegate's method is called OnError?

[Mattias Nissler (ping if slow), 2010-11-19 16:03:56]

Addressed all your comments for continued reviewing pleasure! Please take
another look :)

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

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider.cc:23: const char
kChromePolicyScope[] = "cros/device";
On 2010/11/18 16:12:49, markusheintz_ wrote:
> please replace this with "chromeos/device"

Done.

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

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:42: const char
kServiceParamDeviceID[] = "deviceid";
On 2010/11/18 16:12:49, markusheintz_ wrote:
> s/deviceid/device/

Na, the parameter is really called deviceid.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:43: const char
kServiceParamAgent[] = "agent";
On 2010/11/18 16:12:49, markusheintz_ wrote:
> Please add:
> 
> const char kServiceParamAppType[] = "apptype";

Done.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:46: const char
kServiceValueDeviceType[] = "Chrome";
On 2010/11/18 16:12:49, markusheintz_ wrote:
> Pls add:
> 
> const char kServiceValueAppType[] = "Chrome";

Done.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:176: // A wrapper that
implements the actual backend interface. This is separate from
Done. Also renamed to DeviceManagementBackendImpl.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:217: // Represents a job run
by the service. This contains the common code,
On 2010/11/18 16:12:49, markusheintz_ wrote:
> I somehow think the DeviceManagementJob classes should live in their own file,
> this would make this file easier to read.
> 
> wdyt? We could of course address this in a separate CL.

These are really implementation details for the BackendImpl formerly known as
BackendProxy, so I now put them into that file.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:244:
query_params_.Put(kServiceParamDeviceType, kServiceValueDeviceType);
On 2010/11/18 16:12:49, markusheintz_ wrote:
> Pls add:
> 
> query_params_.Put(kServiceParamAppType, kServiceValueAppType);

Done.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:374: virtual void
ProcessError(DeviceManagementBackend::ErrorCode error) {
On 2010/11/18 16:12:49, markusheintz_ wrote:
> Why naming ProcessError if the delegate's method is called OnError?

Done.

http://codereview.chromium.org/5153002/diff/1/chrome/browser/profile.h
File chrome/browser/profile.h (right):

http://codereview.chromium.org/5153002/diff/1/chrome/browser/profile.h#newcod...
chrome/browser/profile.h:481: virtual policy::DeviceManagementService*
GetDeviceManagementService() = 0;
On 2010/11/18 15:36:33, Nico wrote:
> Do you think it might be worth it to have a PolicyProvider class which
contains
> "all" this policy stuff (currently just DeviceManagementService and
> DeviceManagementPolicyProvider), so that the profile has only one accessor for
> all the policy stuff?

That is definitely worthwile. I created a new PolicyContext class that bundles
all the bits and has nice accessors.

[markusheintz_, 2010-11-19 17:13:12]

LGTM

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

http://codereview.chromium.org/5153002/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_service.cc:42: const char
kServiceParamDeviceID[] = "deviceid";
On 2010/11/19 16:03:56, Mattias Nissler wrote:
> On 2010/11/18 16:12:49, markusheintz_ wrote:
> > s/deviceid/device/
> 
> Na, the parameter is really called deviceid.

Well the doc says something different, but I trust you more than the doc :-).

[Mattias Nissler (ping if slow), 2010-11-22 10:20:53]

Nico, do you want to take another look at this?

[Nico, 2010-11-22 11:06:06]

LG, but a few questions.

Ignore the "why is this override needed?" question (I'm too lazy to hunt it down
and to remove it in rietveld… :-P)

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service.cc:122: void
DeviceManagementService::Shutdown() {
This moves all in-flight jobs back to the "queued" state, which makes sense. How
will these queue_jobs_ get processed? The destructor dchecks that queue_jobs_ is
empty, so it needs to happen somehow.

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service.cc:180: NOTREACHED() <<
"Callback from foreign URL fetcher";
Doesn't this happen in normal usage if a few requests are pending and then
Shutdown() is called before one of the jobs completes? If so, replace the
NOTREACHED() with a "// This happens when…"-type comment

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_context.h (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.h:21: // gone.
The comments on |Initialize()| and |Shutdown()| suggest that you mean "…vital
that it gets initialized _after_ the profile's prefs and destroyed _before_ the
prefs are gone." (right?)

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.h:43:
scoped_ptr<DeviceManagementService> device_management_service_;
No getter for this guy?

http://codereview.chromium.org/5153002/diff/13002/chrome/test/test_url_reques...
File chrome/test/test_url_request_context_getter.h (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/test/test_url_reques...
chrome/test/test_url_request_context_getter.h:26: return
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO);
why is this override needed?

[Mattias Nissler (ping if slow), 2010-11-22 12:35:39]

Nico, here are answers to your questions. I didn't make any code changes.

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/devic...
File chrome/browser/policy/device_management_service.cc (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service.cc:122: void
DeviceManagementService::Shutdown() {
On 2010/11/22 11:06:06, Nico wrote:
> This moves all in-flight jobs back to the "queued" state, which makes sense.
How
> will these queue_jobs_ get processed? The destructor dchecks that queue_jobs_
is
> empty, so it needs to happen somehow.

The backends should call RemoveJob() when they get destroyed, and they should
get shot down before the service.

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/devic...
chrome/browser/policy/device_management_service.cc:180: NOTREACHED() <<
"Callback from foreign URL fetcher";
On 2010/11/22 11:06:06, Nico wrote:
> Doesn't this happen in normal usage if a few requests are pending and then
> Shutdown() is called before one of the jobs completes? If so, replace the
> NOTREACHED() with a "// This happens when…"-type comment

No, shutdown deletes the fetcher, thus cancelling the callback. At least that's
what's supposed to happen.

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_context.h (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.h:21: // gone.
On 2010/11/22 11:06:06, Nico wrote:
> The comments on |Initialize()| and |Shutdown()| suggest that you mean "…vital
> that it gets initialized _after_ the profile's prefs and destroyed _before_
the
> prefs are gone." (right?)

No, actually the comment is correct. The ProfilePolicyContext is a dependency of
the PrefService, so it needs to be constructed before the the prefs and
destroyed after the prefs.

Initialize and Shutdown are there to handle the parts of the context that are
dependent on other profile stuff that gets started later / shut down earlier.

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.h:43:
scoped_ptr<DeviceManagementService> device_management_service_;
On 2010/11/22 11:06:06, Nico wrote:
> No getter for this guy?

Not needed at the moment. GetDeviceManagementPolicyProvider needs it to create
the provider.

http://codereview.chromium.org/5153002/diff/13002/chrome/test/test_url_reques...
File chrome/test/test_url_request_context_getter.h (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/test/test_url_reques...
chrome/test/test_url_request_context_getter.h:26: return
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO);
On 2010/11/22 11:06:06, Nico wrote:
> why is this override needed?

Ignored :)

[Nico, 2010-11-22 12:40:16]

SLG

(…but note that I'm not terribly familiar with this policy code)

If the answer below is either "No" or "This is a stupid question", feel free to
commit.

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_context.cc (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.cc:37:
device_management_service_->Initialize(profile_->GetRequestContext());
can you DCHECK that the prefs aren't initialized already?

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_context.h (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.h:21: // gone.
On 2010/11/22 12:35:39, Mattias Nissler wrote:
> On 2010/11/22 11:06:06, Nico wrote:
> > The comments on |Initialize()| and |Shutdown()| suggest that you mean
"…vital
> > that it gets initialized _after_ the profile's prefs and destroyed _before_
> the
> > prefs are gone." (right?)
> 
> No, actually the comment is correct. The ProfilePolicyContext is a dependency
of
> the PrefService, so it needs to be constructed before the the prefs and
> destroyed after the prefs.
> 
> Initialize and Shutdown are there to handle the parts of the context that are
> dependent on other profile stuff that gets started later / shut down earlier.

Ah! I mixed up the profile's pref and the profile itself.

[Mattias Nissler (ping if slow), 2010-11-22 14:08:07]

Answered the last question. Good to go now?

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_context.cc (right):

http://codereview.chromium.org/5153002/diff/13002/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_context.cc:37:
device_management_service_->Initialize(profile_->GetRequestContext());
On 2010/11/22 12:40:16, Nico wrote:
> can you DCHECK that the prefs aren't initialized already?

They are already initialized at this point, and that's intentional. The point of
this approach is to decouple the network I/O bits from the pref service
initialization (because prefs is initialized at a point where we don't have a
request context).