Make CrosSettings and DeviceSettingsService non Lazy instances

Make CrosSettings and DeviceSettingsService non Lazy instances


BUG=222681
For BrowserProcessImpl change:
TBR=sky@chromium.org

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

[stevenjb, 2013-04-15 21:41:35]

This CL isn't quite as bad as it seems. The essential changes are:

1. Move DeviceSettingsService::Initialize/Shutdown to
ChromeBrowserMainPartsChromeos (it needs to be initialized before/after
BrowserProcessImpl is created/destroyed.
2. Move CrosSettings::Initialize/Shutdown to BrowserProcessImpl (it needs to be
initialized after g_browser_process, but before any call to
g_browser_process->local_state()).
3. Add ScopedTestCrosSettings for creating/destroying these classes in tests.

There are a few other changes to tests that address destruction order so that
observers of these classes are removed correctly, now that they are being
properly destroyed with each test.

[xiyuan, 2013-04-16 16:07:30]

LGTM

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.cc:49: // TODO(xiyaun): Use real
stuff when underlying libcros is ready.
This TODO is obsolete. It is the real stuff. :)

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.cc:334:
chromeos::CrosSettings::Initialize();
nit: no need to have chromeos:: since the code is in namespace chromeos.

[Mattias Nissler (ping if slow), 2013-04-16 16:09:50]

First round of comments from a scan earlier today. The bug tracker currently
barks at me, I'd like to look at the underlying issue you're trying to solve a
bit more.

My main concerns with the current version are:
* You mention that DeviceSettingsService depends on local_state, which is either
wrong or a bug. Can you elaborate on what you found?
* You're making DeviceSettingsService only usable as a static singleton. That
makes testing harder, not easier, since we now can't create one-off instances
for unit tests any longer.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:862:
chromeos::CrosSettings::Initialize();
Why is this not in chrome_browser_main_chromeos.cc?

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:736: //
g_browser_process.
How would g_browser_process access DeviceSettingsService? Is this comment
accurate?

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:792: // Destroy
DeviceSettingsService after g_browser_process.
Isn't the actual dependency CrosSettings here?

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:536: // Initialized
after |mock_dbus_thread_manager_| and |croptohome_| are set up.
fix typo

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:59: //
CloudPolicyManager
colon.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.cc:49: // TODO(xiyaun): Use real
stuff when underlying libcros is ready.
I think this comment is entirely obsolete by now.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.h:29: // Class factory.
This comment is now out of place.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.h:100: virtual ~CrosSettings();
why? nothing is virtual here.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:48: static
DeviceSettingsService*  g_device_settings_service = NULL;
nit: spacing

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:115: void
InitializeSessionManager(SessionManagerClient* session_manager_client,
This function name is unfortunate, the function doesn't actually initialize
session_manager.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:120: void
ShutdownSessionManager();
ditto

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:186: virtual
~DeviceSettingsService();
Why? Nothing is virtual here.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:129:
scoped_ptr<ScopedTestCrosSettings> test_cros_settings_;
This file shouldn't have to depend on CrosSettings really.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:174:
DeviceSettingsService* device_settings_service_;
The whole point of this class was to enable testing against a non-singleton
DeviceSettingsService instance. I'm not a fan of adding references to globals
all over the code.

https://codereview.chromium.org/14200028/diff/6001/chromeos/dbus/dbus_thread_...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chromeos/dbus/dbus_thread_...
chromeos/dbus/dbus_thread_manager.cc:538: // There should never be both a global
instance and a local instance.
Why? This makes writing small unit tests harder!

[stevenjb, 2013-04-16 16:49:43]

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:862:
chromeos::CrosSettings::Initialize();
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> Why is this not in chrome_browser_main_chromeos.cc?
new BrowserProcessImpl() and browser_process_->PreCreateThreads() are both
called in ChromeBrowserMainParts::PreCreateThreadsImpl()

CrosSettings has to be created after g_browser_process because constructing it
references g_browser_process as follows:

CrosSettings::CrosSettings() ->
DeviceSettingsProvider::DeviceSettingsProvider() ->
DeviceSettingsProvider::RetrieveCachedData()

I could put this into ChromeBrowserMainParts::PreCreateThreadsImpl() instead if
that seems better? I suppose I *could* subdivide PreCreateThreads into two
phases, but I'd prefer to avoid combining that level of re-factoring with this.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/ch...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:736: //
g_browser_process.
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> How would g_browser_process access DeviceSettingsService? Is this comment
> accurate?
The relationship is through BrowserPolicyConnector. I'll add that to the
comment.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/ch...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:792: // Destroy
DeviceSettingsService after g_browser_process.
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> Isn't the actual dependency CrosSettings here?
Same as above, also modified comment.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:536: // Initialized
after |mock_dbus_thread_manager_| and |croptohome_| are set up.
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> fix typo

Done.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:59: //
CloudPolicyManager
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> colon.

Done.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.cc:49: // TODO(xiyaun): Use real
stuff when underlying libcros is ready.
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> I think this comment is entirely obsolete by now.

Done.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.cc:334:
chromeos::CrosSettings::Initialize();
On 2013/04/16 16:07:31, xiyuan wrote:
> nit: no need to have chromeos:: since the code is in namespace chromeos.

Done.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/cros_settings.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.h:29: // Class factory.
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> This comment is now out of place.

Done.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/cros_settings.h:100: virtual ~CrosSettings();
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> why? nothing is virtual here.
PrepareTrustedValues

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.cc:48: static
DeviceSettingsService*  g_device_settings_service = NULL;
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> nit: spacing

Done.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:115: void
InitializeSessionManager(SessionManagerClient* session_manager_client,
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> This function name is unfortunate, the function doesn't actually initialize
> session_manager.
->Set

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:120: void
ShutdownSessionManager();
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> ditto
->Unset

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:186: virtual
~DeviceSettingsService();
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> Why? Nothing is virtual here.
SessionManagerClient::Observer methods

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:129:
scoped_ptr<ScopedTestCrosSettings> test_cros_settings_;
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> This file shouldn't have to depend on CrosSettings really.
Enough tests that rely on this also seem to rely on CrosSettings, so this was a
convenient place to put this, rather than each individual test.
That said, if you really don't like this here, I can remove it and track down
each dependency.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:174:
DeviceSettingsService* device_settings_service_;
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> The whole point of this class was to enable testing against a non-singleton
> DeviceSettingsService instance. I'm not a fan of adding references to globals
> all over the code.

The problem is, because it *is* a global singleton, we invariably end up
accessing it in test code whether we expect to or not, which means we have two
instances, causing all sorts of potential trouble. I prefer to control the
global singleton for the test.

https://codereview.chromium.org/14200028/diff/6001/chromeos/dbus/dbus_thread_...
File chromeos/dbus/dbus_thread_manager.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chromeos/dbus/dbus_thread_...
chromeos/dbus/dbus_thread_manager.cc:538: // There should never be both a global
instance and a local instance.
On 2013/04/16 16:09:51, Mattias Nissler wrote:
> Why? This makes writing small unit tests harder!
See previouis comment.

[stevenjb, 2013-04-16 17:07:41]

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:129:
scoped_ptr<ScopedTestCrosSettings> test_cros_settings_;
On 2013/04/16 16:49:43, stevenjb (chromium) wrote:
> On 2013/04/16 16:09:51, Mattias Nissler wrote:
> > This file shouldn't have to depend on CrosSettings really.
> Enough tests that rely on this also seem to rely on CrosSettings, so this was
a
> convenient place to put this, rather than each individual test.
> That said, if you really don't like this here, I can remove it and track down
> each dependency.
> 
I'll go ahead and move this, I agree it is awkward here.

[Mattias Nissler (ping if slow), 2013-04-16 18:04:12]

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:862:
chromeos::CrosSettings::Initialize();
On 2013/04/16 16:49:43, stevenjb (chromium) wrote:
> On 2013/04/16 16:09:51, Mattias Nissler wrote:
> > Why is this not in chrome_browser_main_chromeos.cc?
> new BrowserProcessImpl() and browser_process_->PreCreateThreads() are both
> called in ChromeBrowserMainParts::PreCreateThreadsImpl()
> 
> CrosSettings has to be created after g_browser_process because constructing it
> references g_browser_process as follows:
> 
> CrosSettings::CrosSettings() ->
> DeviceSettingsProvider::DeviceSettingsProvider() ->
> DeviceSettingsProvider::RetrieveCachedData()
> 
> I could put this into ChromeBrowserMainParts::PreCreateThreadsImpl() instead
if
> that seems better? I suppose I *could* subdivide PreCreateThreads into two
> phases, but I'd prefer to avoid combining that level of re-factoring with
this.

Right, and IOThread initialization below immediately makes the first call to
CrosSettings, so we can't move it later in the init sequence either. Hrmpf,
don't know a better solution then without going down more rabbit holes.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_service.h:186: virtual
~DeviceSettingsService();
On 2013/04/16 16:49:43, stevenjb (chromium) wrote:
> On 2013/04/16 16:09:51, Mattias Nissler wrote:
> > Why? Nothing is virtual here.
> SessionManagerClient::Observer methods 

Ah, good point.

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
File chrome/browser/chromeos/settings/device_settings_test_helper.h (right):

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:129:
scoped_ptr<ScopedTestCrosSettings> test_cros_settings_;
On 2013/04/16 17:07:41, stevenjb (chromium) wrote:
> On 2013/04/16 16:49:43, stevenjb (chromium) wrote:
> > On 2013/04/16 16:09:51, Mattias Nissler wrote:
> > > This file shouldn't have to depend on CrosSettings really.
> > Enough tests that rely on this also seem to rely on CrosSettings, so this
was
> a
> > convenient place to put this, rather than each individual test.
> > That said, if you really don't like this here, I can remove it and track
down
> > each dependency.
> > 
> I'll go ahead and move this, I agree it is awkward here.

Thanks!

https://codereview.chromium.org/14200028/diff/6001/chrome/browser/chromeos/se...
chrome/browser/chromeos/settings/device_settings_test_helper.h:174:
DeviceSettingsService* device_settings_service_;
On 2013/04/16 16:49:43, stevenjb (chromium) wrote:
> On 2013/04/16 16:09:51, Mattias Nissler wrote:
> > The whole point of this class was to enable testing against a non-singleton
> > DeviceSettingsService instance. I'm not a fan of adding references to
globals
> > all over the code.
> 
> The problem is, because it *is* a global singleton, we invariably end up
> accessing it in test code whether we expect to or not, which means we have two
> instances, causing all sorts of potential trouble. I prefer to control the
> global singleton for the test.

Per offline discussion, the plan is to not initialize the global pointer in
tests so access to hidden globals is flagged to people adding new tests.

[stevenjb, 2013-04-16 19:41:14]

OK, I restored the local instance of DeviceSettingsService to
DeviceSettingsTestBase so that tests that derive from DeviceSettingsTestBase can
continue to use a local instance. Those tests should not be initializing or
accessing the global instance and should be fine.

Doing something similar with CrosSettings proved more complicated, so I added a
check for whether DeviceSettingsService was already initialize in
ScopedTestCrosSettings, so that it only initializes and destroys
DeviceSettingsService if it has not already been initialized.

PTAL

[bartfab (slow), 2013-04-17 11:14:09]

lgtm

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils_browsertest.cc:426:
DeviceSettingsService::Get()->SetSessionManager(
Nit: #include "chrome/browser/chromeos/settings/device_settings_service.h"

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils_browsertest.cc:427:
&device_settings_test_helper, new MockOwnerKeyUtil());
Nit: #include "chrome/browser/chromeos/settings/mock_owner_key_util.h"

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/cros_settings.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/cros_settings.cc:31: CHECK(!g_cros_settings);
Nit: #include "base/logging.h"

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/cros_settings.cc:37: return g_cros_settings !=
NULL;
Nit: No need to check against NULL. A simple |return g_cros_settings| will do.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_service.cc:58: return
g_device_settings_service != NULL;
Nit: No need to check against NULL.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/extensions...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/extensions...
chrome/browser/extensions/test_extension_system.h:11: #include
"chrome/browser/chromeos/settings/cros_settings.h"
Nit: It would be sufficient to forward-declare here and do the include in the
implementation file.

[Mattias Nissler (ping if slow), 2013-04-17 13:10:30]

LGTM w/ nits.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:330: //
g_browser_process initializes local_state() (which instantiates
nit: g_browser_process also instantiates BrowserPolicyConnector.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:82:
scoped_ptr<DeviceCloudPolicyManagerChromeOS> manager_;
Any reason this still needs to be wrapped in a scoped_ptr? If not, please revert
to a regular class member.

[stevenjb, 2013-04-17 16:27:12]

+miket@ for chrome/browser/extensions OWNER

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:330: //
g_browser_process initializes local_state() (which instantiates
On 2013/04/17 13:10:30, Mattias Nissler wrote:
> nit: g_browser_process also instantiates BrowserPolicyConnector.

Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils_browsertest.cc:426:
DeviceSettingsService::Get()->SetSessionManager(
On 2013/04/17 11:14:10, bartfab wrote:
> Nit: #include "chrome/browser/chromeos/settings/device_settings_service.h"

Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils_browsertest.cc:427:
&device_settings_test_helper, new MockOwnerKeyUtil());
On 2013/04/17 11:14:10, bartfab wrote:
> Nit: #include "chrome/browser/chromeos/settings/mock_owner_key_util.h"

Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:82:
scoped_ptr<DeviceCloudPolicyManagerChromeOS> manager_;
On 2013/04/17 13:10:30, Mattias Nissler wrote:
> Any reason this still needs to be wrapped in a scoped_ptr? If not, please
revert
> to a regular class member.

With the changes to DeviceSettingsTestBase I can revert these too. Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/cros_settings.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/cros_settings.cc:31: CHECK(!g_cros_settings);
On 2013/04/17 11:14:10, bartfab wrote:
> Nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/cros_settings.cc:37: return g_cros_settings !=
NULL;
On 2013/04/17 11:14:10, bartfab wrote:
> Nit: No need to check against NULL. A simple |return g_cros_settings| will do.

Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_settings_service.cc:58: return
g_device_settings_service != NULL;
On 2013/04/17 11:14:10, bartfab wrote:
> Nit: No need to check against NULL.

Done.

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/extensions...
File chrome/browser/extensions/test_extension_system.h (right):

https://codereview.chromium.org/14200028/diff/33002/chrome/browser/extensions...
chrome/browser/extensions/test_extension_system.h:11: #include
"chrome/browser/chromeos/settings/cros_settings.h"
On 2013/04/17 11:14:10, bartfab wrote:
> Nit: It would be sufficient to forward-declare here and do the include in the
> implementation file.

Done.

[miket_OOO, 2013-04-17 16:31:16]

On 2013/04/17 16:27:12, stevenjb (chromium) wrote:
> +miket@ for chrome/browser/extensions OWNER

c/b/e LGTM

[stevenjb, 2013-04-17 19:24:29]

Committed patchset #10 manually as r194656 (presubmit successful).