Rework the device management backend implementation.

Rework the device management backend implementation.

Decouple the backend interface implementation from the actual network
request code we have. This allows for more flexible use of the backend
and resubmitting of requests.

BUG=none
TEST=device_management_impl_(unit|browser)test.cc

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

Can you please review? This change will also allow us to easily re-issue
requests that failed because of the proxy being unavailable.

[danno, 2010-11-16 22:02:56]

a bit late, but here it is...

http://codereview.chromium.org/5026001/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/5026001/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:61: #include
"chrome/browser/policy/device_management_backend_impl.h"
alphabetize

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:54: class
URLQueryParameters {
I am surprised that there is no service to do this already. Consider moving to a
common place?

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:188: static void
SwitchBackend(DeviceManagementBackendImpl* backend);
In practice, there either only the backend NULL or the once cannonical
BackendImpl. However, much of the API around the backend getting swapped into
place seems a little too general, like it's saying, "have as many backends as
you want!". Maybe two public methods here "ActivateBackend(backend)" and
"DeactivateBackend()" or something similar that call SwitchBackend as a private
method might help.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:201: //
GoogleAppsPolicyService overrides.
Superclass name is the old one.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:317:
ProcessError(DeviceManagementBackend::kErrorRequestFailed);
Any particular reason for the name change?

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.h:30:
URLRequestContextGetter* request_context_getter);
I don't know if it would be cleaner/better, but you could consider making the
test classes/base classes friends, and having instantiator methods that are only
visible to the tests. That way you don't have to have the test friend nonsense
put the constructor can be private. Just a thought.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.h:34: static
DeviceManagementBackend* Get();
I am unsure about the name.  I get the whole multiple proxy thing, but since the
caller is responsible for the memory mgmt of the return value, Get seems a bit
misleading, and the comment probably should be stronger about the object
lifecycle management.

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_browsertest.cc:10: #include
"chrome/browser/profile.h"
alphabetize

[markusheintz_, 2010-11-18 12:40:36]

Looks good in general only some nits so far. Bu I got to do a second round to
get the bigger picture.

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:184: // Called by the
DeviceManagementJob dtor so we can clean up.
typo? "dtor"?

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:317:
ProcessError(DeviceManagementBackend::kErrorRequestFailed);
On 2010/11/16 22:02:56, danno wrote:
> Any particular reason for the name change?

+1 for this questions. Especially since the delegate calls OnError

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.h:28: // this, it's only
public so tests can access it.
If the constructor should be private, wouldn't it be better to make the test a
friend instead of making the constructor public?

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_browsertest.cc:29: const
char kServiceURL[] = "http://example.com/device_management";
s/URL/Url/

please do that in the reset of the CL as well.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_unittest.cc:385: 
Sorry if this is a dumb question but what exactly is tested by these two tests?
Does resetting the Service cancel the job?

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_unittest.cc:414:
request.set_policy_scope("chromium");
Not sure if this is important for you test, but the value for the policy scope
is "chromeos/device" according to your doc.

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

Closed in favor of http://codereview.chromium.org/5153002/ I'll incorporate your
feedback given here into that CL if applicable.

[Mattias Nissler (ping if slow), 2010-11-19 17:21:43]

for completeness sake

http://codereview.chromium.org/5026001/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/5026001/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:61: #include
"chrome/browser/policy/device_management_backend_impl.h"
On 2010/11/16 22:02:56, danno wrote:
> alphabetize

Done.

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:54: class
URLQueryParameters {
On 2010/11/16 22:02:56, danno wrote:
> I am surprised that there is no service to do this already. Consider moving to
a
> common place?

I've looked pretty hard and couldn't find anything. There is code that does
pretty awkward non-escaped string stitching. I considered moving to base, but
since this implementation is not ready for general consumption (e.g. how to
handle multiple parameters of the same name) I figured to go with a local
declaration for now.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:184: // Called by the
DeviceManagementJob dtor so we can clean up.
On 2010/11/18 12:40:36, markusheintz_ wrote:
> typo? "dtor"?

It means destructor and is a well-known abbreviation. There are 26 instances in
chrome/:

grep -r '\Wdtor\W' chrome | wc -l

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:188: static void
SwitchBackend(DeviceManagementBackendImpl* backend);
On 2010/11/16 22:02:56, danno wrote:
> In practice, there either only the backend NULL or the once cannonical
> BackendImpl. However, much of the API around the backend getting swapped into
> place seems a little too general, like it's saying, "have as many backends as
> you want!". Maybe two public methods here "ActivateBackend(backend)" and
> "DeactivateBackend()" or something similar that call SwitchBackend as a
private
> method might help.

Doesn't apply any longer.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:201: //
GoogleAppsPolicyService overrides.
On 2010/11/16 22:02:56, danno wrote:
> Superclass name is the old one.

Done.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.cc:317:
ProcessError(DeviceManagementBackend::kErrorRequestFailed);
On 2010/11/16 22:02:56, danno wrote:
> Any particular reason for the name change?

Not any longer, it is a leftover. Changed it back.

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.h:28: // this, it's only
public so tests can access it.
On 2010/11/18 12:40:36, markusheintz_ wrote:
> If the constructor should be private, wouldn't it be better to make the test a
> friend instead of making the constructor public?

doesn't apply any longer.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.h:30:
URLRequestContextGetter* request_context_getter);
On 2010/11/16 22:02:56, danno wrote:
> I don't know if it would be cleaner/better, but you could consider making the
> test classes/base classes friends, and having instantiator methods that are
only
> visible to the tests. That way you don't have to have the test friend nonsense
> put the constructor can be private. Just a thought.

doesn't apply any longer.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl.h:34: static
DeviceManagementBackend* Get();
On 2010/11/16 22:02:56, danno wrote:
> I am unsure about the name.  I get the whole multiple proxy thing, but since
the
> caller is responsible for the memory mgmt of the return value, Get seems a bit
> misleading, and the comment probably should be stronger about the object
> lifecycle management.

Done.

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_browsertest.cc:10: #include
"chrome/browser/profile.h"
On 2010/11/16 22:02:56, danno wrote:
> alphabetize

Done.

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_browsertest.cc:29: const
char kServiceURL[] = "http://example.com/device_management";
On 2010/11/18 12:40:36, markusheintz_ wrote:
> s/URL/Url/
> 
> please do that in the reset of the CL as well.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

I changed the constant names, but the class names I kept in caps (i.e.
URLQueryParameters) for consistence with the existing code (see URLRequest,
URLFetcher, URLRequestContext etc.)

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

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_unittest.cc:385: 
On 2010/11/18 12:40:36, markusheintz_ wrote:
> Sorry if this is a dumb question but what exactly is tested by these two
tests?
> Does resetting the Service cancel the job?

Yes, that's a reset of a scoped_ptr, so the backend goes out of scope, hence the
callback shouldn't happen (which is what we state in the EXPECT_CALL).

http://codereview.chromium.org/5026001/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_backend_impl_unittest.cc:414:
request.set_policy_scope("chromium");
On 2010/11/18 12:40:36, markusheintz_ wrote:
> Not sure if this is important for you test, but the value for the policy scope
> is "chromeos/device" according to your doc. 

Done.