Fixing ProfileManagerTest to use the TestingProfile instead of a real profile. This will allow ea...

Fixing ProfileManagerTest to use the TestingProfile instead of a real profile. This will allow easier testing of ProfileManager-specific functionality as well as not creating problems in the future when changes to Profile are made that should not affect the testing of ProfileManager.

BUG=101434
TEST=Passes all existing ProfileManager tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112124
reverted (r112155)

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

[Paweł Hajdan Jr., 2011-11-21 12:19:37]

Drive-by, and see chrome/test/OWNERS.

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:135:
enable_lazy_service_initialization_(false),
This new variable is not checked consistently in TestingProfile. Please make
either everything check it, or change the name, or find some other solution.

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:188:
profile_dependency_manager_->CreateProfileServices(this, true);
Isn't this essentially identical to Init except the PostTask bit? Could you
factor the code in a way that doesn't duplicate most of it?

[Miranda Callahan, 2011-11-21 15:31:31]

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:207: virtual Profile*
CreateAsyncProfile(const FilePath& path,
I think making an asynchronous creation method analogous to CreateProfile, as
you've done here, is the right way to go, but I'm a bit worried about the
naming. With this method, we now have "CreateProfileAsync" and
"CreateAsyncProfile", and neither the names nor the comments are sufficient to
distinguish what's happening here -- they both basically say "create profile
asynchronously." Can you refactor the naming and comments to be more explicit,
so that it's clear what the different roles of these two methods are?

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:71: return new
TestingProfile(path, this);
Could there be any race conditions with immediately returning the TestingProfile
here? I know in regular asynchronous Profile creation, we don't create the
Browser attached to the Profile or register it in the ProfileInfoCache until
it's fully created, which prevents it from being accessed too early; I'm afraid
that for unit tests, we don't have the same guards.

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.h:61: // profile profiles, use that factory
method instead of this directly.
The comment here says that for multi-profile testing we should use the factory
method -- is this also applicable for the constructor below? (If so, can you add
the comment here?)

[rpetterson, 2011-11-21 22:59:30]

Pawel, thanks for adding on as a test owner.

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager.h (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager.h:207: virtual Profile*
CreateAsyncProfile(const FilePath& path,
On 2011/11/21 15:31:31, Miranda Callahan wrote:
> I think making an asynchronous creation method analogous to CreateProfile, as
> you've done here, is the right way to go, but I'm a bit worried about the
> naming. With this method, we now have "CreateProfileAsync" and
> "CreateAsyncProfile", and neither the names nor the comments are sufficient to
> distinguish what's happening here -- they both basically say "create profile
> asynchronously." Can you refactor the naming and comments to be more explicit,
> so that it's clear what the different roles of these two methods are? 

I see your point. I renamed the helper functions to CreateProfileHelper and
CreateProfileAsyncHelper which should hopefully make their designation as simple
helper functions clearer. I also added more to the comments.

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:71: return new
TestingProfile(path, this);
On 2011/11/21 15:31:31, Miranda Callahan wrote:
> Could there be any race conditions with immediately returning the
TestingProfile
> here? I know in regular asynchronous Profile creation, we don't create the
> Browser attached to the Profile or register it in the ProfileInfoCache until
> it's fully created, which prevents it from being accessed too early; I'm
afraid
> that for unit tests, we don't have the same guards. 

Currently in the ProfileManager we call
ProfileInfo* info =
    RegisterProfile(Profile::CreateProfileAsync(user_data_dir, this), false);

RegisterProfile registers it in the info cache so a profile is returned
immediately (since Profile::CreateProfileAsync shouldn't block) even though it
hasn't been fully initialized at this point. So this behavior should be the
same. With regards to the Browser window, since we aren't creating those for any
of the ProfileManager unittests, it shouldn't be an issue.

In either case, it should not be an issue since the primary asynchronous
activity that happens (from what I can tell) is prefs creation and that occurs
lazily in the normal TestingProfile as is.

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:135:
enable_lazy_service_initialization_(false),
On 2011/11/21 12:19:37, Paweł Hajdan Jr. wrote:
> This new variable is not checked consistently in TestingProfile. Please make
> either everything check it, or change the name, or find some other solution.

I added it to all Get*Service methods which:
- are explicitly named for a service 
- do not currently return NULL
- do not require additional parameters to initialize

There were 4 such methods. The only exception was GetPrefs() which always
initializes lazily.

If this is insufficient, please let me know and I will find a different way
around it.

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.cc:188:
profile_dependency_manager_->CreateProfileServices(this, true);
On 2011/11/21 12:19:37, Paweł Hajdan Jr. wrote:
> Isn't this essentially identical to Init except the PostTask bit? Could you
> factor the code in a way that doesn't duplicate most of it?

Done.

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.h:61: // profile profiles, use that factory
method instead of this directly.
On 2011/11/21 15:31:31, Miranda Callahan wrote:
> The comment here says that for multi-profile testing we should use the factory
> method -- is this also applicable for the constructor below? (If so, can you
add
> the comment here?)

This comment isn't actually applicable here either because it requires a
TestingProfileManager and we're trying to test the ProfileManager (so using a
TestingProfileManager would be contrary to that purpose). I've added a comment
that the new function should be used when testing the ProfileManager.

[Miranda Callahan, 2011-11-21 23:15:00]

On 2011/11/21 22:59:30, rpetterson wrote:
> Pawel, thanks for adding on as a test owner.
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profile_manager.h (right):
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_manager.h:207: virtual Profile*
> CreateAsyncProfile(const FilePath& path,
> On 2011/11/21 15:31:31, Miranda Callahan wrote:
> > I think making an asynchronous creation method analogous to CreateProfile,
as
> > you've done here, is the right way to go, but I'm a bit worried about the
> > naming. With this method, we now have "CreateProfileAsync" and
> > "CreateAsyncProfile", and neither the names nor the comments are sufficient
to
> > distinguish what's happening here -- they both basically say "create profile
> > asynchronously." Can you refactor the naming and comments to be more
explicit,
> > so that it's clear what the different roles of these two methods are? 
> 
> I see your point. I renamed the helper functions to CreateProfileHelper and
> CreateProfileAsyncHelper which should hopefully make their designation as
simple
> helper functions clearer. I also added more to the comments.
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profile_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/prof...
> chrome/browser/profiles/profile_manager_unittest.cc:71: return new
> TestingProfile(path, this);
> On 2011/11/21 15:31:31, Miranda Callahan wrote:
> > Could there be any race conditions with immediately returning the
> TestingProfile
> > here? I know in regular asynchronous Profile creation, we don't create the
> > Browser attached to the Profile or register it in the ProfileInfoCache until
> > it's fully created, which prevents it from being accessed too early; I'm
> afraid
> > that for unit tests, we don't have the same guards. 
> 
> Currently in the ProfileManager we call
> ProfileInfo* info =
>     RegisterProfile(Profile::CreateProfileAsync(user_data_dir, this), false);
> 
> RegisterProfile registers it in the info cache so a profile is returned
> immediately (since Profile::CreateProfileAsync shouldn't block) even though it
> hasn't been fully initialized at this point. So this behavior should be the
> same. With regards to the Browser window, since we aren't creating those for
any
> of the ProfileManager unittests, it shouldn't be an issue.
> 
> In either case, it should not be an issue since the primary asynchronous
> activity that happens (from what I can tell) is prefs creation and that occurs
> lazily in the normal TestingProfile as is.
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
> File chrome/test/base/testing_profile.cc (right):
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
> chrome/test/base/testing_profile.cc:135:
> enable_lazy_service_initialization_(false),
> On 2011/11/21 12:19:37, Paweł Hajdan Jr. wrote:
> > This new variable is not checked consistently in TestingProfile. Please make
> > either everything check it, or change the name, or find some other solution.
> 
> I added it to all Get*Service methods which:
> - are explicitly named for a service 
> - do not currently return NULL
> - do not require additional parameters to initialize
> 
> There were 4 such methods. The only exception was GetPrefs() which always
> initializes lazily.
> 
> If this is insufficient, please let me know and I will find a different way
> around it.
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
> chrome/test/base/testing_profile.cc:188:
> profile_dependency_manager_->CreateProfileServices(this, true);
> On 2011/11/21 12:19:37, Paweł Hajdan Jr. wrote:
> > Isn't this essentially identical to Init except the PostTask bit? Could you
> > factor the code in a way that doesn't duplicate most of it?
> 
> Done.
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
> File chrome/test/base/testing_profile.h (right):
> 
>
http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_pro...
> chrome/test/base/testing_profile.h:61: // profile profiles, use that factory
> method instead of this directly.
> On 2011/11/21 15:31:31, Miranda Callahan wrote:
> > The comment here says that for multi-profile testing we should use the
factory
> > method -- is this also applicable for the constructor below? (If so, can you
> add
> > the comment here?)
> 
> This comment isn't actually applicable here either because it requires a
> TestingProfileManager and we're trying to test the ProfileManager (so using a
> TestingProfileManager would be contrary to that purpose). I've added a comment
> that the new function should be used when testing the ProfileManager.

Thanks for the fixes; LGTM!

[willchan no longer on Chromium, 2011-11-21 23:21:12]

I'm fine with Miranda's approval. I don't think you need to wait on me.

On Mon, Nov 21, 2011 at 3:15 PM, <mirandac@chromium.org> wrote:

> On 2011/11/21 22:59:30, rpetterson wrote:
>
>> Pawel, thanks for adding on as a test owner.
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
browser/profiles/profile_**manager.h<http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/profile_manager.h>
>
>> File chrome/browser/profiles/**profile_manager.h (right):
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
browser/profiles/profile_**manager.h#newcode207<http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/profile_manager.h#newcode207>
>
>> chrome/browser/profiles/**profile_manager.h:207: virtual Profile*
>> CreateAsyncProfile(const FilePath& path,
>> On 2011/11/21 15:31:31, Miranda Callahan wrote:
>> > I think making an asynchronous creation method analogous to
>> CreateProfile,
>>
> as
>
>> > you've done here, is the right way to go, but I'm a bit worried about
>> the
>> > naming. With this method, we now have "CreateProfileAsync" and
>> > "CreateAsyncProfile", and neither the names nor the comments are
>> sufficient
>>
> to
>
>> > distinguish what's happening here -- they both basically say "create
>> profile
>> > asynchronously." Can you refactor the naming and comments to be more
>>
> explicit,
>
>> > so that it's clear what the different roles of these two methods are?
>>
>
>  I see your point. I renamed the helper functions to CreateProfileHelper
>> and
>> CreateProfileAsyncHelper which should hopefully make their designation as
>>
> simple
>
>> helper functions clearer. I also added more to the comments.
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
browser/profiles/profile_**manager_unittest.cc<http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/profile_manager_unittest.cc>
>
>> File chrome/browser/profiles/**profile_manager_unittest.cc (right):
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
browser/profiles/profile_**manager_unittest.cc#newcode71<http://codereview.chromium.org/8565032/diff/5002/chrome/browser/profiles/profile_manager_unittest.cc#newcode71>
>
>> chrome/browser/profiles/**profile_manager_unittest.cc:**71: return new
>> TestingProfile(path, this);
>> On 2011/11/21 15:31:31, Miranda Callahan wrote:
>> > Could there be any race conditions with immediately returning the
>> TestingProfile
>> > here? I know in regular asynchronous Profile creation, we don't create
>> the
>> > Browser attached to the Profile or register it in the ProfileInfoCache
>> until
>> > it's fully created, which prevents it from being accessed too early; I'm
>> afraid
>> > that for unit tests, we don't have the same guards.
>>
>
>  Currently in the ProfileManager we call
>> ProfileInfo* info =
>>     RegisterProfile(Profile::**CreateProfileAsync(user_data_**dir,
>> this), false);
>>
>
>  RegisterProfile registers it in the info cache so a profile is returned
>> immediately (since Profile::CreateProfileAsync shouldn't block) even
>> though it
>> hasn't been fully initialized at this point. So this behavior should be
>> the
>> same. With regards to the Browser window, since we aren't creating those
>> for
>>
> any
>
>> of the ProfileManager unittests, it shouldn't be an issue.
>>
>
>  In either case, it should not be an issue since the primary asynchronous
>> activity that happens (from what I can tell) is prefs creation and that
>> occurs
>> lazily in the normal TestingProfile as is.
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
test/base/testing_profile.cc<http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_profile.cc>
>
>> File chrome/test/base/testing_**profile.cc (right):
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
test/base/testing_profile.cc#**newcode135<http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_profile.cc#newcode135>
>
>> chrome/test/base/testing_**profile.cc:135:
>> enable_lazy_service_**initialization_(false),
>> On 2011/11/21 12:19:37, Paweł Hajdan Jr. wrote:
>> > This new variable is not checked consistently in TestingProfile. Please
>> make
>> > either everything check it, or change the name, or find some other
>> solution.
>>
>
>  I added it to all Get*Service methods which:
>> - are explicitly named for a service
>> - do not currently return NULL
>> - do not require additional parameters to initialize
>>
>
>  There were 4 such methods. The only exception was GetPrefs() which always
>> initializes lazily.
>>
>
>  If this is insufficient, please let me know and I will find a different
>> way
>> around it.
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
test/base/testing_profile.cc#**newcode188<http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_profile.cc#newcode188>
>
>> chrome/test/base/testing_**profile.cc:188:
>> profile_dependency_manager_->**CreateProfileServices(this, true);
>> On 2011/11/21 12:19:37, Paweł Hajdan Jr. wrote:
>> > Isn't this essentially identical to Init except the PostTask bit? Could
>> you
>> > factor the code in a way that doesn't duplicate most of it?
>>
>
>  Done.
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
test/base/testing_profile.h<http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_profile.h>
>
>> File chrome/test/base/testing_**profile.h (right):
>>
>
>
> http://codereview.chromium.**org/8565032/diff/5002/chrome/**
>
test/base/testing_profile.h#**newcode61<http://codereview.chromium.org/8565032/diff/5002/chrome/test/base/testing_profile.h#newcode61>
>
>> chrome/test/base/testing_**profile.h:61: // profile profiles, use that
>> factory
>> method instead of this directly.
>> On 2011/11/21 15:31:31, Miranda Callahan wrote:
>> > The comment here says that for multi-profile testing we should use the
>>
> factory
>
>> > method -- is this also applicable for the constructor below? (If so,
>> can you
>> add
>> > the comment here?)
>>
>
>  This comment isn't actually applicable here either because it requires a
>> TestingProfileManager and we're trying to test the ProfileManager (so
>> using a
>> TestingProfileManager would be contrary to that purpose). I've added a
>> comment
>> that the new function should be used when testing the ProfileManager.
>>
>
> Thanks for the fixes; LGTM!
>
>
http://codereview.chromium.**org/8565032/<http://codereview.chromium.org/8565...
>

[Paweł Hajdan Jr., 2011-11-22 09:18:41]

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:417: // Always lazily initialize so no check
for
I don't understand the difference here. It seems that in other cases
enable_lazy_service_initialization_ = false means "disable", and the service is
not created. Why is this case different? Why do we need this boolean member
variable at all?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:574: if (!prefs_.get()) {
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:597: if (!file_system_context_) {
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:649: if (!extensions_request_context_)
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:674: if (!find_bar_state_.get())
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:680: if (!host_content_settings_map_.get())
{
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:689: if
(!geolocation_permission_context_.get()) {
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:697: if (!speech_input_preferences_.get())
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:749: if (webkit_context_ == NULL) {
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:771: if (!pref_proxy_config_tracker_.get())
{
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:788: if (!token_service_.get()) {
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:800: if (!profile_sync_service_.get()) {
Should this check enable_lazy_service_initialization_ ?

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:823: if (!chrome_url_data_manager_.get())
Should this check enable_lazy_service_initialization_ ?

[rpetterson, 2011-11-22 21:43:25]

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8565032/diff/27001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:417: // Always lazily initialize so no check
for
On 2011/11/22 09:18:42, Paweł Hajdan Jr. wrote:
> I don't understand the difference here. It seems that in other cases
> enable_lazy_service_initialization_ = false means "disable", and the service
is
> not created. Why is this case different? Why do we need this boolean member
> variable at all?

The intention of enable_lazy_service_initialization_ was that if it is true,
then we allow lazy initialization. That is, if we are getting the object and it
did not exist before, we create it then. If enable_lazy_service_initialization_
is false and the object does not exist, we do not create it and return null so
it would be up to the user of the code to create it.

I understand the point you were trying make now and rather than introducing a
new variable, I have changed the profile manager unittest. This seems to be a
cleaner solution.

[Paweł Hajdan Jr., 2011-11-23 09:45:33]

LGTM with a comment, thank you. This is much better.

http://codereview.chromium.org/8565032/diff/37001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile_manager.cc (right):

http://codereview.chromium.org/8565032/diff/37001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile_manager.cc:24: virtual Profile*
CreateProfileHelper(const FilePath& file_path) {
nit: Could you use OVERRIDE to catch more errors at compile time?

[rpetterson, 2011-11-29 00:54:30]

http://codereview.chromium.org/8565032/diff/37001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile_manager.cc (right):

http://codereview.chromium.org/8565032/diff/37001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile_manager.cc:24: virtual Profile*
CreateProfileHelper(const FilePath& file_path) {
On 2011/11/23 09:45:33, Paweł Hajdan Jr. wrote:
> nit: Could you use OVERRIDE to catch more errors at compile time?

Done. Also added OVERRIDE to the profile_manager_unittest where I override the
same functions.

[commit-bot: I haz the power, 2011-11-29 00:54:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8565032/43001

[commit-bot: I haz the power, 2011-11-29 00:54:50]

Presubmit check for 8565032-43001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
The old callback system is deprecated. If possible, use base::Bind and
base::Callback instead.
    chrome/browser/profiles/profile_manager_unittest.cc:70

Presubmit checks took 1.7s to calculate.

[commit-bot: I haz the power, 2011-11-30 00:08:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8565032/48001

[commit-bot: I haz the power, 2011-11-30 04:39:11]

Change committed as 112124

[rpetterson, 2011-12-01 21:17:07]

@mirandac, I had to tweak some bits in profile_manager_unittest to handle some
cros stuff. Do you mind taking another quick look to make sure I didn't move
anything that shouldn't have been moved? Thanks.

[Miranda Callahan, 2011-12-01 21:23:59]

On 2011/12/01 21:17:07, rpetterson wrote:
> @mirandac, I had to tweak some bits in profile_manager_unittest to handle some
> cros stuff. Do you mind taking another quick look to make sure I didn't move
> anything that shouldn't have been moved? Thanks.

Hey, Rachel -- LGTM, none of your tweaks should have changed the underlying
logic of the PM unittests. Thanks!

[commit-bot: I haz the power, 2011-12-02 00:10:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8565032/61003

[commit-bot: I haz the power, 2011-12-02 02:37:19]

Change committed as 112617