Implement device management backend.

Implement device management backend.

This adds a device management backend implementation running over HTTP.

BUG=None
TEST=unit tests in device_management_backend_impl_unittest.cc

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

[Mattias Nissler (ping if slow), 2010-10-26 15:24:12]

Note: This depends on http://codereview.chromium.org/4121003/show

danno: can you start reviewing this? It still has some stuff you'll probably fix
in beforementioned CL, but I'd like to get some feedback.

eroman: I put you on the list since this CL adds a custom URLRequestContext
implementation and I see your name in that code :) I'd like to make sure what I
do is the right approach. The main reasons for adding the custom request context
are:

- The device management backend isn't associated with a profile, but
ChromeURLRequestContext is, if I understand correctly.
- The code may run early in the initialization process at which point the
profile is not yet available
- The requests generated should be separate from regular Chrome requests in
terms of cookies, caching etc.

Feedback and advice is appreciated! Thanks.

[danno, 2010-10-29 13:09:43]

Here's a start, but I'm not done yet. I'll give you something to do :-)

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

http://codereview.chromium.org/4098004/diff/1/3#newcode51
chrome/browser/policy/device_management_backend_impl.cc:51: host_resolver_ =
I assume this code is pretty much cut and pasted from somewhere else except for
the exceptions? Any way to share code?

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

http://codereview.chromium.org/4098004/diff/1/4#newcode54
chrome/browser/policy/device_management_backend_impl.h:54: const std::string&
data) OVERRIDE;
Do you really need override here? It seems like you would get an compile error
here if the signature didn't match anyway, Evan's guidelines was to use it in
places that make the code clearer, I'm not sure this is one of them. If it is,
then I don't see why the Process* calls don't have them.

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

http://codereview.chromium.org/4098004/diff/1/5#newcode17
chrome/browser/policy/device_management_backend_impl_unittest.cc:17: const char
kServiceURL[] = "https://example.com/management_service";
put these constants in an anonymous namespace

http://codereview.chromium.org/4098004/diff/1/5#newcode26
chrome/browser/policy/device_management_backend_impl_unittest.cc:26: #define
PROTO_STRING(name) (std::string(name, arraysize(name) - 1))
Why the macro? Just make a function that uses strlen appropriately. Or just
handle the whole business in const char* type space.

http://codereview.chromium.org/4098004/diff/1/5#newcode280
chrome/browser/policy/device_management_backend_impl_unittest.cc:280:
URLRequestStatus status(URLRequestStatus::SUCCESS, 0);
const? also several similar instances throughout this file.

[Mattias Nissler (ping if slow), 2010-10-29 13:46:06]

New version is up.

eroman: PING (or suggest someone to review custom URLRequestContext subclass)
danno: more comments
bauerb: so you know what we're up to

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

http://codereview.chromium.org/4098004/diff/1/3#newcode51
chrome/browser/policy/device_management_backend_impl.cc:51: host_resolver_ =
On 2010/10/29 13:09:43, danno wrote:
> I assume this code is pretty much cut and pasted from somewhere else except
for
> the exceptions? Any way to share code?

Actually, it's missing a good number of bits from other similar places and
differs subtly in other aspects. Moreover, I expect I'll require some special
magic to set proxy_service_ sooner or later, so I figured trying hard to reuse
stuff would only complicate things.

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

http://codereview.chromium.org/4098004/diff/1/4#newcode54
chrome/browser/policy/device_management_backend_impl.h:54: const std::string&
data) OVERRIDE;
On 2010/10/29 13:09:43, danno wrote:
> Do you really need override here? It seems like you would get an compile error
> here if the signature didn't match anyway, Evan's guidelines was to use it in
> places that make the code clearer, I'm not sure this is one of them. If it is,
> then I don't see why the Process* calls don't have them.

I can't recall when I put it anyway, so removed.

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

http://codereview.chromium.org/4098004/diff/1/5#newcode17
chrome/browser/policy/device_management_backend_impl_unittest.cc:17: const char
kServiceURL[] = "https://example.com/management_service";
On 2010/10/29 13:09:43, danno wrote:
> put these constants in an anonymous namespace

Done.

http://codereview.chromium.org/4098004/diff/1/5#newcode26
chrome/browser/policy/device_management_backend_impl_unittest.cc:26: #define
PROTO_STRING(name) (std::string(name, arraysize(name) - 1))
On 2010/10/29 13:09:43, danno wrote:
> Why the macro? Just make a function that uses strlen appropriately. Or just
> handle the whole business in const char* type space.

That's not possible. The binary protobuf encoding can have semantically
significant trailing zeros.

[Bernhard Bauer, 2010-10-29 16:19:31]

http://codereview.chromium.org/4098004/diff/11001/12001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/11001/12001#newcode222
chrome/browser/policy/device_management_backend_impl.cc:222: virtual void
OnError(DeviceManagementBackend::ErrorCode error);
Why don't you define these methods inline?

[Mattias Nissler (ping if slow), 2010-11-02 13:11:19]

Here's an updated version.

willchan, can you take over for eroman who doesn't respond? I'm looking for
someone to give me feedback on implementing a custom URLRequestContext subclass
is the way to go here, see the initial comment for more information.

http://codereview.chromium.org/4098004/diff/11001/12001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/11001/12001#newcode222
chrome/browser/policy/device_management_backend_impl.cc:222: virtual void
OnError(DeviceManagementBackend::ErrorCode error);
On 2010/10/29 16:19:31, Bernhard Bauer wrote:
> Why don't you define these methods inline?

Good idea, thanks, that makes the code more compact. Done.

[willchan no longer on Chromium, 2010-11-02 15:45:31]

I think in the short-term this is probably fine.  How are you making sure you
aren't leaking the URLRequestContext?  When is this backend object getting
deleted?

Long-term, we probably want to make this request context "global", perhaps by
sticking it in the browser process or something.  Or perhaps we should have an
easy way to generate a URLFetcher that doesn't need users to create new
URLRequestContext objects.  We could have the URLFetcher itself or some nice
helper object generate new, temporary URLRequestContexts.

Eric, can you chime in with your thoughts here?

http://codereview.chromium.org/4098004/diff/15001/16001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/15001/16001#newcode53
chrome/browser/policy/device_management_backend_impl.cc:53: host_resolver_ =
You're leaking this.  And even if you don't want the same CookieMonster and
HttpCache, you probably want to reuse the same HostResolver stored in IOThread. 
You should probably construct the getter with g_browser_process->io_thread(),
and then access IOThread::Globals on the IO thread to get the HostResolver and
set it into the new request context.

The reason to ask you to use the same HostResolver is so we enforce the same
limits for number of concurrent requests, same micro dns cache, etc.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode57
chrome/browser/policy/device_management_backend_impl.cc:57: proxy_service_ =
net::ProxyService::CreateDirect();
CreateDirect()?  What if you need a proxy to access the internet?  This doesn't
look correct to me.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode58
chrome/browser/policy/device_management_backend_impl.cc:58: ssl_config_service_
= new net::SSLConfigServiceDefaults;
Is this only ever used in Chrome OS?  It's probably safest for you to call
SSLConfigService::CreateSystemSSLConfigService() to make this cross-platform.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode59
chrome/browser/policy/device_management_backend_impl.cc:59:
http_auth_handler_factory_ =
You're leaking this.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode61
chrome/browser/policy/device_management_backend_impl.cc:61:
http_transaction_factory_ =
You're leaking this.

[Mattias Nissler (ping if slow), 2010-11-02 19:38:34]

A new version is up that addresses willchan's comments and add a browser test
that exercises the request context code paths.

http://codereview.chromium.org/4098004/diff/15001/16001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/15001/16001#newcode53
chrome/browser/policy/device_management_backend_impl.cc:53: host_resolver_ =
On 2010/11/02 15:45:31, willchan wrote:
> You're leaking this.  And even if you don't want the same CookieMonster and
> HttpCache, you probably want to reuse the same HostResolver stored in
IOThread. 
> You should probably construct the getter with g_browser_process->io_thread(),
> and then access IOThread::Globals on the IO thread to get the HostResolver and
> set it into the new request context.
> 
> The reason to ask you to use the same HostResolver is so we enforce the same
> limits for number of concurrent requests, same micro dns cache, etc.

Done.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode57
chrome/browser/policy/device_management_backend_impl.cc:57: proxy_service_ =
net::ProxyService::CreateDirect();
On 2010/11/02 15:45:31, willchan wrote:
> CreateDirect()?  What if you need a proxy to access the internet?  This
doesn't
> look correct to me.

Actually, this is intentional for the time being. We need to ask the management
service to fetch the initial proxy configuration, so this is why we'll create a
direct connection for now. I expect this to change once we have the code in
place that caches the device management policy information.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode58
chrome/browser/policy/device_management_backend_impl.cc:58: ssl_config_service_
= new net::SSLConfigServiceDefaults;
On 2010/11/02 15:45:31, willchan wrote:
> Is this only ever used in Chrome OS?  It's probably safest for you to call
> SSLConfigService::CreateSystemSSLConfigService() to make this cross-platform.

Done.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode59
chrome/browser/policy/device_management_backend_impl.cc:59:
http_auth_handler_factory_ =
On 2010/11/02 15:45:31, willchan wrote:
> You're leaking this.

Done.

http://codereview.chromium.org/4098004/diff/15001/16001#newcode61
chrome/browser/policy/device_management_backend_impl.cc:61:
http_transaction_factory_ =
On 2010/11/02 15:45:31, willchan wrote:
> You're leaking this.

Done.

[willchan no longer on Chromium, 2010-11-03 11:04:11]

http://codereview.chromium.org/4098004/diff/20001/21001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/20001/21001#newcode48
chrome/browser/policy/device_management_backend_impl.cc:48: // Custom request
context implementation, allowing to override user agent etc.
Please fully explain the need for this special URLRequestContext so that it's
obvious why you're not just using the default one.

http://codereview.chromium.org/4098004/diff/20001/21001#newcode99
chrome/browser/policy/device_management_backend_impl.cc:99: :
io_thread_(g_browser_process->io_thread()) {}
Just to check, this is run on the UI thread, right?  I believe this accessor is
invalid on other threads.

http://codereview.chromium.org/4098004/diff/20001/21001#newcode177
chrome/browser/policy/device_management_backend_impl.cc:177: protected:
This should be private, right?  Or do subclasses need to call this?

http://codereview.chromium.org/4098004/diff/20001/21001#newcode289
chrome/browser/policy/device_management_backend_impl.cc:289:
request_context_getter_(
What's the lifetime of DeviceManagementBackendImpl?  I'm trying to figure out
what the lifetime of your URLRequestContext is.  The URLRequestContext owns
various data structures which are NonThreadSafe.  This means that they always
expect to be constructed/destructed on the same thread.  The
URLRequestContextGetter enforces this by posting the DeleteTask to the IO
thread.  But if the IOThread is gone, since the URLRequestContext will get
leaked.  Therefore, in this case, we need to make sure that the
DeviceManagementBackendImpl is deleted before the IO thread has stopped.

http://codereview.chromium.org/4098004/diff/20001/21002
File chrome/browser/policy/device_management_backend_impl.h (right):

http://codereview.chromium.org/4098004/diff/20001/21002#newcode72
chrome/browser/policy/device_management_backend_impl.h:72: typedef
std::map<const URLFetcher*, ResponseHandler*> ResponseHandlerMap;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
typedefs and enums go first in the section.  I know you're just putting it next
to its use, but google convention is to put this at the beginning of the
section.

http://codereview.chromium.org/4098004/diff/20001/21003
File chrome/browser/policy/device_management_backend_impl_browsertest.cc
(right):

http://codereview.chromium.org/4098004/diff/20001/21003#newcode7
chrome/browser/policy/device_management_backend_impl_browsertest.cc:7: #include
"chrome/browser/policy/device_management_backend_impl.h"
This include should go first.  See
http://www.corp.google.com/eng/doc/cppguide.xml#Names_and_Order_of_Includes
which says "dir/foo.cc and dir2/foo2.h are often in the same directory (e.g.
base/basictypes_test.cc and base/basictypes.h)" which shows that the foo.h and
foo.cc relationship all applies for foo.h and foo_unittest.cc.

[Mattias Nissler (ping if slow), 2010-11-03 12:02:44]

Addressed all of willchan's comments, new version is up.

http://codereview.chromium.org/4098004/diff/20001/21001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/20001/21001#newcode48
chrome/browser/policy/device_management_backend_impl.cc:48: // Custom request
context implementation, allowing to override user agent etc.
On 2010/11/03 11:04:12, willchan wrote:
> Please fully explain the need for this special URLRequestContext so that it's
> obvious why you're not just using the default one.

Done.

http://codereview.chromium.org/4098004/diff/20001/21001#newcode99
chrome/browser/policy/device_management_backend_impl.cc:99: :
io_thread_(g_browser_process->io_thread()) {}
On 2010/11/03 11:04:12, willchan wrote:
> Just to check, this is run on the UI thread, right?  I believe this accessor
is
> invalid on other threads.

That's true and this will run on the UI thread during browser_process
initialization. Note that corresponding code is not yet there, but that's our
plan :)

http://codereview.chromium.org/4098004/diff/20001/21001#newcode177
chrome/browser/policy/device_management_backend_impl.cc:177: protected:
On 2010/11/03 11:04:12, willchan wrote:
> This should be private, right?  Or do subclasses need to call this?

Done.

http://codereview.chromium.org/4098004/diff/20001/21001#newcode289
chrome/browser/policy/device_management_backend_impl.cc:289:
request_context_getter_(
On 2010/11/03 11:04:12, willchan wrote:
> What's the lifetime of DeviceManagementBackendImpl?  I'm trying to figure out
> what the lifetime of your URLRequestContext is.  The URLRequestContext owns
> various data structures which are NonThreadSafe.  This means that they always
> expect to be constructed/destructed on the same thread.  The
> URLRequestContextGetter enforces this by posting the DeleteTask to the IO
> thread.  But if the IOThread is gone, since the URLRequestContext will get
> leaked.  Therefore, in this case, we need to make sure that the
> DeviceManagementBackendImpl is deleted before the IO thread has stopped.

I am aware of these issues and the plan is to shut down the backend from the
BrowserProcessImpl dtor before all the threads go down. I'll work out the exact
details once we have a policy provider we hook the backend up with.

http://codereview.chromium.org/4098004/diff/20001/21002
File chrome/browser/policy/device_management_backend_impl.h (right):

http://codereview.chromium.org/4098004/diff/20001/21002#newcode72
chrome/browser/policy/device_management_backend_impl.h:72: typedef
std::map<const URLFetcher*, ResponseHandler*> ResponseHandlerMap;
On 2010/11/03 11:04:12, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> typedefs and enums go first in the section.  I know you're just putting it
next
> to its use, but google convention is to put this at the beginning of the
> section.

Sorry, incompatible habit ;)
Done.

http://codereview.chromium.org/4098004/diff/20001/21003
File chrome/browser/policy/device_management_backend_impl_browsertest.cc
(right):

http://codereview.chromium.org/4098004/diff/20001/21003#newcode7
chrome/browser/policy/device_management_backend_impl_browsertest.cc:7: #include
"chrome/browser/policy/device_management_backend_impl.h"
On 2010/11/03 11:04:12, willchan wrote:
> This include should go first.  See
> http://www.corp.google.com/eng/doc/cppguide.xml#Names_and_Order_of_Includes
> which says "dir/foo.cc and dir2/foo2.h are often in the same directory (e.g.
> base/basictypes_test.cc and base/basictypes.h)" which shows that the foo.h and
> foo.cc relationship all applies for foo.h and foo_unittest.cc.

Done, also for _unittest.cc

[willchan no longer on Chromium, 2010-11-03 12:07:36]

LGTM as is then.  As I mentioned earlier, it's possible we will want to revisit
this use of the URLRequestContext, but I'm fine with it as is.  Eric, please
take a look at this when you get a chance, and we can talk about how we plan to
address this use case in the future.

On 2010/11/03 12:02:44, Mattias Nissler wrote:
> Addressed all of willchan's comments, new version is up.
> 
> http://codereview.chromium.org/4098004/diff/20001/21001
> File chrome/browser/policy/device_management_backend_impl.cc (right):
> 
> http://codereview.chromium.org/4098004/diff/20001/21001#newcode48
> chrome/browser/policy/device_management_backend_impl.cc:48: // Custom request
> context implementation, allowing to override user agent etc.
> On 2010/11/03 11:04:12, willchan wrote:
> > Please fully explain the need for this special URLRequestContext so that
it's
> > obvious why you're not just using the default one.
> 
> Done.
> 
> http://codereview.chromium.org/4098004/diff/20001/21001#newcode99
> chrome/browser/policy/device_management_backend_impl.cc:99: :
> io_thread_(g_browser_process->io_thread()) {}
> On 2010/11/03 11:04:12, willchan wrote:
> > Just to check, this is run on the UI thread, right?  I believe this accessor
> is
> > invalid on other threads.
> 
> That's true and this will run on the UI thread during browser_process
> initialization. Note that corresponding code is not yet there, but that's our
> plan :)
> 
> http://codereview.chromium.org/4098004/diff/20001/21001#newcode177
> chrome/browser/policy/device_management_backend_impl.cc:177: protected:
> On 2010/11/03 11:04:12, willchan wrote:
> > This should be private, right?  Or do subclasses need to call this?
> 
> Done.
> 
> http://codereview.chromium.org/4098004/diff/20001/21001#newcode289
> chrome/browser/policy/device_management_backend_impl.cc:289:
> request_context_getter_(
> On 2010/11/03 11:04:12, willchan wrote:
> > What's the lifetime of DeviceManagementBackendImpl?  I'm trying to figure
out
> > what the lifetime of your URLRequestContext is.  The URLRequestContext owns
> > various data structures which are NonThreadSafe.  This means that they
always
> > expect to be constructed/destructed on the same thread.  The
> > URLRequestContextGetter enforces this by posting the DeleteTask to the IO
> > thread.  But if the IOThread is gone, since the URLRequestContext will get
> > leaked.  Therefore, in this case, we need to make sure that the
> > DeviceManagementBackendImpl is deleted before the IO thread has stopped.
> 
> I am aware of these issues and the plan is to shut down the backend from the
> BrowserProcessImpl dtor before all the threads go down. I'll work out the
exact
> details once we have a policy provider we hook the backend up with.
> 
> http://codereview.chromium.org/4098004/diff/20001/21002
> File chrome/browser/policy/device_management_backend_impl.h (right):
> 
> http://codereview.chromium.org/4098004/diff/20001/21002#newcode72
> chrome/browser/policy/device_management_backend_impl.h:72: typedef
> std::map<const URLFetcher*, ResponseHandler*> ResponseHandlerMap;
> On 2010/11/03 11:04:12, willchan wrote:
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> > typedefs and enums go first in the section.  I know you're just putting it
> next
> > to its use, but google convention is to put this at the beginning of the
> > section.
> 
> Sorry, incompatible habit ;)
> Done.
> 
> http://codereview.chromium.org/4098004/diff/20001/21003
> File chrome/browser/policy/device_management_backend_impl_browsertest.cc
> (right):
> 
> http://codereview.chromium.org/4098004/diff/20001/21003#newcode7
> chrome/browser/policy/device_management_backend_impl_browsertest.cc:7:
#include
> "chrome/browser/policy/device_management_backend_impl.h"
> On 2010/11/03 11:04:12, willchan wrote:
> > This include should go first.  See
> > http://www.corp.google.com/eng/doc/cppguide.xml#Names_and_Order_of_Includes
> > which says "dir/foo.cc and dir2/foo2.h are often in the same directory (e.g.
> > base/basictypes_test.cc and base/basictypes.h)" which shows that the foo.h
and
> > foo.cc relationship all applies for foo.h and foo_unittest.cc.
> 
> Done, also for _unittest.cc

[danno, 2010-11-03 12:32:57]

LGTM with nits. In addition to the ones that I found, please address the missing
includes identified by lint.

http://codereview.chromium.org/4098004/diff/28001/29001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/28001/29001#newcode293
chrome/browser/policy/device_management_backend_impl.cc:293: new
DeviceManagementBackendRequestContextGetter()){
nit: missing space before {

http://codereview.chromium.org/4098004/diff/28001/29004
File chrome/browser/policy/device_management_backend_impl_unittest.cc (right):

http://codereview.chromium.org/4098004/diff/28001/29004#newcode80
chrome/browser/policy/device_management_backend_impl_unittest.cc:80: public
DeviceManagementBackendImplTestBase<
nit: Should colon be on this or next line? In any case, indent 4.

http://codereview.chromium.org/4098004/diff/28001/29004#newcode176
chrome/browser/policy/device_management_backend_impl_unittest.cc:176: class
DeviceManagementBackendImplTest :
Same here.

[Mattias Nissler (ping if slow), 2010-11-03 13:22:21]

Fixed nits, will commit when the tree reopens.

http://codereview.chromium.org/4098004/diff/28001/29001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/28001/29001#newcode293
chrome/browser/policy/device_management_backend_impl.cc:293: new
DeviceManagementBackendRequestContextGetter()){
On 2010/11/03 12:32:57, danno wrote:
> nit: missing space before {

Done.

http://codereview.chromium.org/4098004/diff/28001/29004
File chrome/browser/policy/device_management_backend_impl_unittest.cc (right):

http://codereview.chromium.org/4098004/diff/28001/29004#newcode80
chrome/browser/policy/device_management_backend_impl_unittest.cc:80: public
DeviceManagementBackendImplTestBase<
On 2010/11/03 12:32:57, danno wrote:
> nit: Should colon be on this or next line? In any case, indent 4.

Done.

http://codereview.chromium.org/4098004/diff/28001/29004#newcode176
chrome/browser/policy/device_management_backend_impl_unittest.cc:176: class
DeviceManagementBackendImplTest :
On 2010/11/03 12:32:57, danno wrote:
> Same here.

Done.

[eroman, 2010-11-08 22:38:56]

lgtm regarding the URLRequestContext.

You are right that ChromeURLRequestContext is tied to a profile so for this use
case seems reasonable to have a separate one.

http://codereview.chromium.org/4098004/diff/34002/39001
File chrome/browser/policy/device_management_backend_impl.cc (right):

http://codereview.chromium.org/4098004/diff/34002/39001#newcode115
chrome/browser/policy/device_management_backend_impl.cc:115:
DeviceManagementBackendRequestContextGetter::GetURLRequestContext() {
Could you add an assertion that this is currently on the IO thread?