[cros] Use mocked URLFetcher and other mocks in ExistingUserController tests.

[cros] Use mocked URLFetcher and other mocks in ExistingUserController tests.

* Add MockLoginUtils + DoLaunchBrowser is not a static method any more.
* Add MockUserManager
* Added few missing basic expectation in MockDBusThreadManager

Renaming of virtual getters/setters is done in http://codereview.chromium.org/9355059/

BUG=chromium-os:25926
TEST=ExistingUserController.ExistingUserLogin

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

[Nikita (slow), 2012-02-20 14:53:54]

ivankr > please review whole CL as it is mostly around UserManager / LoginUtils
mocks.

satorux > few changes in
chrome/browser/chromeos/dbus/mock_dbus_thread_manager.cc to cleanup test output
from gmock warnings.

[satorux1, 2012-02-20 17:54:47]

This patch is fairly big. I think it'd be nice to split, if possible.

Anyway, chrome/browser/chromeos/dbus LGTM.

[Nikita (slow), 2012-02-20 18:21:45]

On 2012/02/20 17:54:47, satorux1 wrote:
> This patch is fairly big. I think it'd be nice to split, if possible.

I know but most of it is just renaming of 6 getters/setters that go side by side
with UserManager interface addition.

Most changes are in user_manager.* and login_utils.* +
existing_user_controller_browsertest.cc

Rename could be in a separate CL though. Do you think it makes sense to create a
separate one for it?

[satorux1, 2012-02-20 20:31:30]

On 2012/02/20 18:21:45, Nikita Kostylev wrote:
> On 2012/02/20 17:54:47, satorux1 wrote:
> > This patch is fairly big. I think it'd be nice to split, if possible.
> 
> I know but most of it is just renaming of 6 getters/setters that go side by
side
> with UserManager interface addition.
> 
> Most changes are in user_manager.* and login_utils.* +
> existing_user_controller_browsertest.cc
> 
> Rename could be in a separate CL though. Do you think it makes sense to create
a
> separate one for it?

It's up to you but I generally like to create a separate patch that only renames
things without changing logic. Then create a new patch to introduce logic
changes. my 2c.

[Nikita (slow), 2012-02-21 10:21:02]

On 2012/02/20 20:31:30, satorux1 wrote:
> On 2012/02/20 18:21:45, Nikita Kostylev wrote:
> > On 2012/02/20 17:54:47, satorux1 wrote:
> > > This patch is fairly big. I think it'd be nice to split, if possible.
> > 
> > I know but most of it is just renaming of 6 getters/setters that go side by
> side
> > with UserManager interface addition.
> > 
> > Most changes are in user_manager.* and login_utils.* +
> > existing_user_controller_browsertest.cc
> > 
> > Rename could be in a separate CL though. Do you think it makes sense to
create
> a
> > separate one for it?
> 
> It's up to you but I generally like to create a separate patch that only
renames
> things without changing logic. Then create a new patch to introduce logic
> changes. my 2c.

I agree. Split renaming into separate CL -
http://codereview.chromium.org/9355059/ (will commit after this one).

[Nikita (slow), 2012-02-21 17:21:05]

trybots
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos_clang/...
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

[Ivan Korotkov, 2012-02-22 17:16:35]

LGTM with nits:

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:112:
testing_profile_.reset(NULL);
Why do you need this? Is it too late to run from destructor?

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:66: const char klogged_in_users[]
= "logged_in_users";
I guess this was sed :)

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:515: const std::string&
display_email) {
Align please (and below).

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:162: // Implementation of the
UserManager.
Split this to a separate file, please.

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:361: class UserManagerImplWrapper {
Do we really need this wrapper? Is LAZY_INITIALIZED deprecated?
Anyway, let's move it to .cc, looks like the declaration is not needed anywhere.

[satorux1, 2012-02-22 19:30:05]

chrome/browser/chromeos/dbus/ still LGTM

[Nikita (slow), 2012-03-05 14:42:36]

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:112:
testing_profile_.reset(NULL);
On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> Why do you need this? Is it too late to run from destructor?

It has to be deleted on main thread otherwise some of the services will be
already gone.

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:66: const char klogged_in_users[]
= "logged_in_users";
On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> I guess this was sed :)

Done.

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:515: const std::string&
display_email) {
On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> Align please (and below).

Done.

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:162: // Implementation of the
UserManager.
On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> Split this to a separate file, please.

Done.

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:361: class UserManagerImplWrapper {
On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> Do we really need this wrapper? Is LAZY_INITIALIZED deprecated?
> Anyway, let's move it to .cc, looks like the declaration is not needed
anywhere.

Wrapper actually is needed to have ability to set mock as UserManager instance.

It also has to stay in header file to be marked as friend of UserManagerImpl.

[Ivan Korotkov, 2012-03-05 14:52:24]

LGTM

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:361: class UserManagerImplWrapper {
On 2012/03/05 14:42:36, Nikita Kostylev wrote:
> On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> > Do we really need this wrapper? Is LAZY_INITIALIZED deprecated?
> > Anyway, let's move it to .cc, looks like the declaration is not needed
> anywhere.
> 
> Wrapper actually is needed to have ability to set mock as UserManager
instance.
> 
> It also has to stay in header file to be marked as friend of UserManagerImpl.

You don't actually need the declaration of a class to mark it as friend (friend
class X acts as forward declaration of X);

[Nikita (slow), 2012-03-06 15:41:57]

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9348022/diff/3068/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:361: class UserManagerImplWrapper {
On 2012/03/05 14:52:24, Ivan Korotkov wrote:
> On 2012/03/05 14:42:36, Nikita Kostylev wrote:
> > On 2012/02/22 17:16:35, Ivan Korotkov wrote:
> > > Do we really need this wrapper? Is LAZY_INITIALIZED deprecated?
> > > Anyway, let's move it to .cc, looks like the declaration is not needed
> > anywhere.
> > 
> > Wrapper actually is needed to have ability to set mock as UserManager
> instance.
> > 
> > It also has to stay in header file to be marked as friend of
UserManagerImpl.
> 
> You don't actually need the declaration of a class to mark it as friend
(friend
> class X acts as forward declaration of X);

Done. I placed it in user_manager.cc

[commit-bot: I haz the power, 2012-03-06 15:42:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/9348022/27002

[commit-bot: I haz the power, 2012-03-06 15:42:24]

Can't process patch for file chrome/browser/chromeos/login/user_manager.h.
File's status is None, patchset upload is incomplete.

[Nikita (slow), 2012-03-06 15:43:39]

Pushing manually. All patchets are failing to upload for this CL fully.