In process browser test for login screen.

In process browser test for login screen.

BUG=chromiumos:2036
TEST=Run out/Debug/browser_tests --gtest_filter=LoginManagerViewTest.TestBasic


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

[Nikita (slow), 2010-03-25 14:34:29]

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode4
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:4: 
header guard

http://codereview.chromium.org/1256004/diff/1/5#newcode112
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:112: test_api->
SetLoginLibrary(NULL);
empty space after ->

http://codereview.chromium.org/1256004/diff/1/3
File chrome/browser/chromeos/login/utils.h (right):

http://codereview.chromium.org/1256004/diff/1/3#newcode23
chrome/browser/chromeos/login/utils.h:23: class LoginUtils {
May be it makes sense to completely extract interface so that one could create
default gMock class for that
like mock_login_utils.cc.

[Nikita (slow), 2010-03-25 14:45:59]

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode4
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:4: 
On 2010/03/25 14:34:29, Nikita Kostylev wrote:
> header guard

Sorry, my fault it's not a header :)

[Paweł Hajdan Jr., 2010-03-25 15:11:30]

Drive-by with a test comment.

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode132
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:132:
MessageLoop::current()->Quit();
That's weird. It shouldn't be needed. Maybe what you really want is a unit test
with a test window?

[Dmitry Polukhin, 2010-03-25 15:26:11]

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode112
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:112: test_api->
SetLoginLibrary(NULL);
On 2010/03/25 14:34:29, Nikita Kostylev wrote:
> empty space after ->

Done.

http://codereview.chromium.org/1256004/diff/1/5#newcode132
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:132:
MessageLoop::current()->Quit();
On 2010/03/25 15:11:30, Paweł Hajdan Jr. wrote:
> That's weird. It shouldn't be needed. Maybe what you really want is a unit
test
> with a test window?

Unfortunately it looks like we can't avoid this. OOBE tests are special case in
many sense. We would like to test these screens in conditions close as much as
possible to real run.

http://codereview.chromium.org/1256004/diff/1/3
File chrome/browser/chromeos/login/utils.h (right):

http://codereview.chromium.org/1256004/diff/1/3#newcode23
chrome/browser/chromeos/login/utils.h:23: class LoginUtils {
On 2010/03/25 14:34:29, Nikita Kostylev wrote:
> May be it makes sense to completely extract interface so that one could create
> default gMock class for that
> like mock_login_utils.cc.

Done.

[sky, 2010-03-25 15:28:54]

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode18
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:18: using
::testing::AnyNumber;
Add a newline between 17 and 18.

http://codereview.chromium.org/1256004/diff/1/5#newcode71
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:71: };
DISALLOW_COPY_...

http://codereview.chromium.org/1256004/diff/1/5#newcode81
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:81:
command_line->AppendSwitchWithValue(switches::kLoginScreen, "login");
These constants should be a common place, otherwise it's easy to miss a name
change.

http://codereview.chromium.org/1256004/diff/1/6
File chrome/browser/chromeos/login/utils.cc (right):

http://codereview.chromium.org/1256004/diff/1/6#newcode80
chrome/browser/chromeos/login/utils.cc:80: return
Singleton<scoped_ptr<LoginUtils> >::get()->get();
You shouldn't need a scoped_ptr with singleton.

http://codereview.chromium.org/1256004/diff/1/3
File chrome/browser/chromeos/login/utils.h (right):

http://codereview.chromium.org/1256004/diff/1/3#newcode23
chrome/browser/chromeos/login/utils.h:23: class LoginUtils {
If you're going to make this a class you need to change the file name and header
guards.

http://codereview.chromium.org/1256004/diff/1/3#newcode29
chrome/browser/chromeos/login/utils.h:29: static void Set(LoginUtils*);
Include the variable name in your arguments.

[Nikita (slow), 2010-03-25 15:33:50]

LGTM

[Nikita (slow), 2010-03-25 15:35:59]

Please fix 3 Lint errors.

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode81
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:81:
command_line->AppendSwitchWithValue(switches::kLoginScreen, "login");
On 2010/03/25 15:28:54, sky wrote:
> These constants should be a common place, otherwise it's easy to miss a name
> change.

Agree, we need to make them public in a WizardController.

[Dmitry Polukhin, 2010-03-25 16:01:26]

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode18
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:18: using
::testing::AnyNumber;
On 2010/03/25 15:28:54, sky wrote:
> Add a newline between 17 and 18.

Done.

http://codereview.chromium.org/1256004/diff/1/5#newcode71
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:71: };
On 2010/03/25 15:28:54, sky wrote:
> DISALLOW_COPY_...

Done.

http://codereview.chromium.org/1256004/diff/1/5#newcode81
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:81:
command_line->AppendSwitchWithValue(switches::kLoginScreen, "login");
On 2010/03/25 15:36:00, Nikita Kostylev wrote:
> On 2010/03/25 15:28:54, sky wrote:
> > These constants should be a common place, otherwise it's easy to miss a name
> > change.
> 
> Agree, we need to make them public in a WizardController.

Done.

http://codereview.chromium.org/1256004/diff/1/6
File chrome/browser/chromeos/login/utils.cc (right):

http://codereview.chromium.org/1256004/diff/1/6#newcode80
chrome/browser/chromeos/login/utils.cc:80: return
Singleton<scoped_ptr<LoginUtils> >::get()->get();
On 2010/03/25 15:28:54, sky wrote:
> You shouldn't need a scoped_ptr with singleton.

Unfortunately I do need scoped_ptr to be able to replace implementation.
Otherwise only default implementation will be used. But I changed it to separate
the interface from implementation.

http://codereview.chromium.org/1256004/diff/1/3
File chrome/browser/chromeos/login/utils.h (right):

http://codereview.chromium.org/1256004/diff/1/3#newcode23
chrome/browser/chromeos/login/utils.h:23: class LoginUtils {
On 2010/03/25 15:28:54, sky wrote:
> If you're going to make this a class you need to change the file name and
header
> guards.

Done.

http://codereview.chromium.org/1256004/diff/1/3#newcode29
chrome/browser/chromeos/login/utils.h:29: static void Set(LoginUtils*);
On 2010/03/25 15:28:54, sky wrote:
> Include the variable name in your arguments.

Done.

[Paweł Hajdan Jr., 2010-03-25 16:11:44]

http://codereview.chromium.org/1256004/diff/1/5
File chrome/browser/chromeos/login/login_manager_view_browsertest.cc (right):

http://codereview.chromium.org/1256004/diff/1/5#newcode132
chrome/browser/chromeos/login/login_manager_view_browsertest.cc:132:
MessageLoop::current()->Quit();
On 2010/03/25 15:26:12, Dmitry Polukhin wrote:
> On 2010/03/25 15:11:30, Paweł Hajdan Jr. wrote:
> > That's weird. It shouldn't be needed. Maybe what you really want is a unit
> test
> > with a test window?
> 
> Unfortunately it looks like we can't avoid this. OOBE tests are special case
in
> many sense. We would like to test these screens in conditions close as much as
> possible to real run.

I see. But I still don't understand why quitting the message loop is required
here. Could you explain more?

[Dmitry Polukhin, 2010-03-26 11:33:06]

On 2010/03/25 16:11:44, Paweł Hajdan Jr. wrote:
> I see. But I still don't understand why quitting the message loop is required
> here. Could you explain more?

We don't create normal browser window so it is not closed and doesn't send quite
on window close. We should actually move this line to some common place for all
OOBE screen tests.

[Paweł Hajdan Jr., 2010-03-26 16:32:37]

I see it committed (which is sort of weird because there were pending comments
of mine, and I didn't LGTM the patch), so could you do a follow-up to move the
MessageLoop::current()->Quit() call to a common place?

[Nikita (slow), 2010-03-26 16:36:08]

On 2010/03/26 16:32:37, Paweł Hajdan Jr. wrote:
> I see it committed (which is sort of weird because there were pending comments
> of mine, and I didn't LGTM the patch), so could you do a follow-up to move the
> MessageLoop::current()->Quit() call to a common place?

Paweł, there are have several OOBE test cases and we're working on introducing
base class that will address such issues in a separate refactoring patch.

[Paweł Hajdan Jr., 2010-03-26 16:42:21]

On 2010/03/26 16:36:08, Nikita Kostylev wrote:
> On 2010/03/26 16:32:37, Paweł Hajdan Jr. wrote:
> > I see it committed (which is sort of weird because there were pending
comments
> > of mine, and I didn't LGTM the patch), so could you do a follow-up to move
the
> > MessageLoop::current()->Quit() call to a common place?
> 
> Paweł, there are have several OOBE test cases and we're working on introducing
> base class that will address such issues in a separate refactoring patch.

Sounds good to me.

[Dmitry Polukhin, 2010-03-26 18:30:08]

Paweł, I'm sorry for misunderstanding. This CL already had one LGTM and it
is third test with the same issue (it is common for all OOBE tests) that we
are going to address in separate CL as Nikita already said (I'm sorry if it
wasn't clear from my comment). All other comments were addressed so I
committed it.

On Fri, Mar 26, 2010 at 7:32 PM, <phajdan.jr@chromium.org> wrote:

> I see it committed (which is sort of weird because there were pending
> comments
> of mine, and I didn't LGTM the patch), so could you do a follow-up to move
> the
> MessageLoop::current()->Quit() call to a common place?
>
>
> http://codereview.chromium.org/1256004
>

[Paweł Hajdan Jr., 2010-03-26 18:48:26]

On Fri, Mar 26, 2010 at 19:29, Dmitry Polukhin <dpolukhin@chromium.org>wrote:

> Paweł, I'm sorry for misunderstanding. This CL already had one LGTM and it
> is third test with the same issue (it is common for all OOBE tests) that we
> are going to address in separate CL as Nikita already said (I'm sorry if it
> wasn't clear from my comment). All other comments were addressed so I
> committed it.


I see. If possible, in the future please make sure to explain it before
committing and wait for an LGTM (the explanation like the above is most
often fine).