Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4098004:
Implement device management backend. (Closed)
|
Created:
10 years, 1 month ago by Mattias Nissler (ping if slow) Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., ben+cc_chromium.org, Bernhard Bauer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement 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
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments. #Patch Set 3 : Pass DMToken in Auth header. #
Total comments: 2
Patch Set 4 : misc updates. #
Total comments: 10
Patch Set 5 : Address willchan's feedback and add end-to-end test. #
Total comments: 12
Patch Set 6 : Address feedback. #
Total comments: 6
Patch Set 7 : Fix nits. #
Total comments: 1
Messages
Total messages: 13 (0 generated)
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.
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.
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.
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?
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.
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.
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.
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.
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
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
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.
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.
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? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
