Add consumer_device_management_service() in BrowserPolicyConnectorChromeOS.

Add consumer_device_management_service() in BrowserPolicyConnectorChromeOS.
    
    Expose kServiceInitializationStartupDelay in ChromeBrowserPolicyConnector.
    
    Extract DeviceManagementServiceConfiguration to its own files.
    
    Add a command line flag to override the consumer DM server address for
    testing.
    
    Fix a comment in policy_switches as it no longer applies.
    
    BUG=353050
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266449

[davidyu, 2014-04-23 04:13:20]

On 2014/04/17 09:40:04, davidyu wrote:

ping

[bartfab (slow), 2014-04-23 12:17:17]

https://codereview.chromium.org/240903002/diff/1/chrome/browser/policy/chrome...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/1/chrome/browser/policy/chrome...
chrome/browser/policy/chrome_browser_policy_connector.cc:82:
DeviceManagementServiceConfiguration(
Nit: explicit.

https://codereview.chromium.org/240903002/diff/1/chrome/browser/policy/chrome...
chrome/browser/policy/chrome_browser_policy_connector.cc:156:
ScopedVector<DeviceManagementService> services;
Nit: #include "base/memory/scoped_vector.h"

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
File components/policy/core/browser/browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:160: // Skip device
initialization if the BrowserPolicyConnector was never
Nit: That comment is no longer necessary. If |device_management_services_| is
empty, the method will obviously do nothing.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:162: for
(ScopedVector<DeviceManagementService>::const_iterator i =
Nit: The most common variable name for iterators in Chrome code is "it."

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:177: return
device_management_services_[management_mode];
Nit: Add DCHECKs to verify that we were not given an invalid |management_mode|.

Alternatively, and I would prefer this solution, replace the ScopedVector with
two members, enterprise_device_management_service_ and
consumer_device_management_service_. We are not planning to expand beyond two
DMSs anytime soon anyway, so a vector is a bit of an overkill.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:244: return
std::string();
Nit: Add a NOTREACHED().

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
File components/policy/core/browser/browser_policy_connector.h (right):

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.h:84: // DEPRECATED: use
GetDeviceManagementService() instead.
Nit: Add a TODO and a bug number to have all current callers switch to
GetDeviceManagementService() instead.

[davidyu, 2014-04-23 12:56:58]

Also added a NOT_MANAGED value in the enum so that it is the same as the one in
the proto. This may be useful later.

https://codereview.chromium.org/240903002/diff/1/chrome/browser/policy/chrome...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/1/chrome/browser/policy/chrome...
chrome/browser/policy/chrome_browser_policy_connector.cc:82:
DeviceManagementServiceConfiguration(
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: explicit.

Done.

https://codereview.chromium.org/240903002/diff/1/chrome/browser/policy/chrome...
chrome/browser/policy/chrome_browser_policy_connector.cc:156:
ScopedVector<DeviceManagementService> services;
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: #include "base/memory/scoped_vector.h"

ScopedVector<> removed.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
File components/policy/core/browser/browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:160: // Skip device
initialization if the BrowserPolicyConnector was never
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: That comment is no longer necessary. If |device_management_services_| is
> empty, the method will obviously do nothing.

Code reverted.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:162: for
(ScopedVector<DeviceManagementService>::const_iterator i =
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: The most common variable name for iterators in Chrome code is "it."

Code removed.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:177: return
device_management_services_[management_mode];
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: Add DCHECKs to verify that we were not given an invalid
|management_mode|.
> 
> Alternatively, and I would prefer this solution, replace the ScopedVector with
> two members, enterprise_device_management_service_ and
> consumer_device_management_service_. We are not planning to expand beyond two
> DMSs anytime soon anyway, so a vector is a bit of an overkill.

Done.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.cc:244: return
std::string();
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: Add a NOTREACHED().

Done.

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
File components/policy/core/browser/browser_policy_connector.h (right):

https://codereview.chromium.org/240903002/diff/1/components/policy/core/brows...
components/policy/core/browser/browser_policy_connector.h:84: // DEPRECATED: use
GetDeviceManagementService() instead.
On 2014/04/23 12:17:18, bartfab wrote:
> Nit: Add a TODO and a bug number to have all current callers switch to
> GetDeviceManagementService() instead.

Done.

[davidyu, 2014-04-23 13:02:17]

Hi Nikita,

Can you help reviewing chrome_restart_request.cc again? Thanks!

[Mattias Nissler (ping if slow), 2014-04-23 14:32:39]

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
chrome/browser/policy/chrome_browser_policy_connector.cc:82: explicit
DeviceManagementServiceConfiguration(
Note that this file is the cross-platform BrowserPolicyConnector, it should not
contain any bits specific to Chrome OS. Is consumer management expected to be
available on other Chrome platforms as well in the near future? If not, this
should move to browser_policy_connector_chromeos.cc

It may make sense to extract DeviceManagementServiceConfiguration into a
publicly-available class, in that case I'd prefer to just pass the URL as the
parameter, not the enum (that'll be more flexible for tests etc.).

[davidyu, 2014-04-23 15:22:49]

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
chrome/browser/policy/chrome_browser_policy_connector.cc:82: explicit
DeviceManagementServiceConfiguration(
On 2014/04/23 14:32:39, Mattias Nissler wrote:
> Note that this file is the cross-platform BrowserPolicyConnector, it should
not
> contain any bits specific to Chrome OS. Is consumer management expected to be
> available on other Chrome platforms as well in the near future? If not, this
> should move to browser_policy_connector_chromeos.cc
> 
> It may make sense to extract DeviceManagementServiceConfiguration into a
> publicly-available class, in that case I'd prefer to just pass the URL as the
> parameter, not the enum (that'll be more flexible for tests etc.).

I don't think we will make this feature available on other platforms anytime
soon. Do you think I should also move the related in BrowserPolicyConnector to
BrowserPolicyConnectorChromeOS as well?

[Mattias Nissler (ping if slow), 2014-04-23 15:25:17]

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
chrome/browser/policy/chrome_browser_policy_connector.cc:82: explicit
DeviceManagementServiceConfiguration(
On 2014/04/23 15:22:50, davidyu wrote:
> On 2014/04/23 14:32:39, Mattias Nissler wrote:
> > Note that this file is the cross-platform BrowserPolicyConnector, it should
> not
> > contain any bits specific to Chrome OS. Is consumer management expected to
be
> > available on other Chrome platforms as well in the near future? If not, this
> > should move to browser_policy_connector_chromeos.cc
> > 
> > It may make sense to extract DeviceManagementServiceConfiguration into a
> > publicly-available class, in that case I'd prefer to just pass the URL as
the
> > parameter, not the enum (that'll be more flexible for tests etc.).
> 
> I don't think we will make this feature available on other platforms anytime
> soon. Do you think I should also move the related in BrowserPolicyConnector to
> BrowserPolicyConnectorChromeOS as well?

Yes, everything related to consumer management should be in the
BrowserPolicyConnectorChromeOS then.

[davidyu, 2014-04-24 04:52:41]

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
File chrome/browser/policy/chrome_browser_policy_connector.cc (right):

https://codereview.chromium.org/240903002/diff/20001/chrome/browser/policy/ch...
chrome/browser/policy/chrome_browser_policy_connector.cc:82: explicit
DeviceManagementServiceConfiguration(
On 2014/04/23 15:25:18, Mattias Nissler wrote:
> On 2014/04/23 15:22:50, davidyu wrote:
> > On 2014/04/23 14:32:39, Mattias Nissler wrote:
> > > Note that this file is the cross-platform BrowserPolicyConnector, it
should
> > not
> > > contain any bits specific to Chrome OS. Is consumer management expected to
> be
> > > available on other Chrome platforms as well in the near future? If not,
this
> > > should move to browser_policy_connector_chromeos.cc
> > > 
> > > It may make sense to extract DeviceManagementServiceConfiguration into a
> > > publicly-available class, in that case I'd prefer to just pass the URL as
> the
> > > parameter, not the enum (that'll be more flexible for tests etc.).
> > 
> > I don't think we will make this feature available on other platforms anytime
> > soon. Do you think I should also move the related in BrowserPolicyConnector
to
> > BrowserPolicyConnectorChromeOS as well?
> 
> Yes, everything related to consumer management should be in the
> BrowserPolicyConnectorChromeOS then.

Done.

[Mattias Nissler (ping if slow), 2014-04-24 11:38:11]

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:78:
chromeos::switches::kConsumerDeviceManagementUrl))
nit: curlies for readability please

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:81: else
no need for the else here.

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.cc (right):

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:21: return
BrowserPolicyConnector::GetDeviceManagementUrl();
Can we just pass this in as a ctor parameter? That way, you also avoid the
subclass.

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.h (right):

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:21: virtual
std::string GetServerUrl() OVERRIDE;
#include <base/compiler_specific.h> for OVERRIDE

[Nikita (slow), 2014-04-24 12:23:55]

On 2014/04/23 13:02:17, davidyu wrote:
> Hi Nikita,
> 
> Can you help reviewing chrome_restart_request.cc again? Thanks!

chrome/browser/chromeos/login/chrome_restart_request.cc lgtm

[davidyu, 2014-04-25 01:45:39]

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:78:
chromeos::switches::kConsumerDeviceManagementUrl))
On 2014/04/24 11:38:12, Mattias Nissler wrote:
> nit: curlies for readability please

Done.

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:81: else
On 2014/04/24 11:38:12, Mattias Nissler wrote:
> no need for the else here.

Done.

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.cc (right):

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:21: return
BrowserPolicyConnector::GetDeviceManagementUrl();
On 2014/04/24 11:38:12, Mattias Nissler wrote:
> Can we just pass this in as a ctor parameter? That way, you also avoid the
> subclass.

Done.

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.h (right):

https://codereview.chromium.org/240903002/diff/40001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:21: virtual
std::string GetServerUrl() OVERRIDE;
On 2014/04/24 11:38:12, Mattias Nissler wrote:
> #include <base/compiler_specific.h> for OVERRIDE

Done.

[Mattias Nissler (ping if slow), 2014-04-25 09:11:53]

LGTM

[bartfab (slow), 2014-04-25 09:17:34]

lgtm

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:70:
std::string GetConsumerDeviceManagementServerUrl() {
Nit: #include <string>

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:71:
CommandLine* command_line = CommandLine::ForCurrentProcess();
Nit: const.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.cc (right):

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:19: const
std::string& server_url) : server_url_(server_url) {}
Nit: This violates the style guide: If entire method declaration and
initialization cannot fit on one line, the initialitation should go on its own
line:

DeviceManagementServiceConfiguration::DeviceManagementServiceConfiguration(
    const std::string& server_url)
    : server_url_(server_url) {}

Nit: This also violates what was discussed on chromium-dev recently: It is OK to
use {} for inline methods but when defining a method outside a class, the
closing brace should go on its own line.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:21:
DeviceManagementServiceConfiguration::~DeviceManagementServiceConfiguration() {}
Nit: The closing brace should go on its own line.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:46: LOG(ERROR)
<< "Failed to get machine information";
Nit: #include "base/logging.h"

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:54: int32
os_major_version = 0;
Nit: #include "base/basictypes.h"

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.h (right):

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:16: // is used
to create the device management service instance.
Nit: s/the device management service instance/device management service
instances/

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:28: std::string
server_url_;
Nit: const.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:29: };
Nit: Add DISALLOW_COPY_AND_ASSIGN(DeviceManagementServiceConfiguration) and
#include "base/macros.h" for it.

[davidyu, 2014-04-28 02:58:21]

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:70:
std::string GetConsumerDeviceManagementServerUrl() {
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:71:
CommandLine* command_line = CommandLine::ForCurrentProcess();
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.cc (right):

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:19: const
std::string& server_url) : server_url_(server_url) {}
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: This violates the style guide: If entire method declaration and
> initialization cannot fit on one line, the initialitation should go on its own
> line:
> 
> DeviceManagementServiceConfiguration::DeviceManagementServiceConfiguration(
>     const std::string& server_url)
>     : server_url_(server_url) {}
> 
> Nit: This also violates what was discussed on chromium-dev recently: It is OK
to
> use {} for inline methods but when defining a method outside a class, the
> closing brace should go on its own line.

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:21:
DeviceManagementServiceConfiguration::~DeviceManagementServiceConfiguration() {}
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: The closing brace should go on its own line.

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:46: LOG(ERROR)
<< "Failed to get machine information";
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.cc:54: int32
os_major_version = 0;
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: #include "base/basictypes.h"

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
File chrome/browser/policy/device_management_service_configuration.h (right):

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:16: // is used
to create the device management service instance.
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: s/the device management service instance/device management service
> instances/

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:28: std::string
server_url_;
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/240903002/diff/60001/chrome/browser/policy/de...
chrome/browser/policy/device_management_service_configuration.h:29: };
On 2014/04/25 09:17:35, bartfab wrote:
> Nit: Add DISALLOW_COPY_AND_ASSIGN(DeviceManagementServiceConfiguration) and
> #include "base/macros.h" for it.

Done.

[davidyu, 2014-04-28 02:58:41]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-04-28 02:59:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidyu@chromium.org/240903002/80001

[commit-bot: I haz the power, 2014-04-28 03:41:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 03:41:10]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[davidyu, 2014-04-28 03:58:37]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-04-28 04:00:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidyu@chromium.org/240903002/80001

[commit-bot: I haz the power, 2014-04-28 04:45:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 04:45:27]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[davidyu, 2014-04-28 06:04:12]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-04-28 06:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidyu@chromium.org/240903002/80001

[commit-bot: I haz the power, 2014-04-28 06:48:31]

Change committed as 266449