Componentize LoginManagerTest.

Componentize LoginManagerTest.

This lets browser tests use the functionality of LoginManagerTestHelper
without having to derive from LoginManagerTest.

If the CL is correct, there should be no logic changes,
except for a fix to
login_state_notification_blocker_chromeos_browsertest.cc.

BUG=359578
TEST=these are the tests
R=nkostylev@chromium.org, mukai@chromium.org, battre@chromium.org, dzhioev@chromium.org

mukai: c/b/notifications/
battre: c/b/net/

[michaelpg, 2014-05-07 00:40:35]

PTAL. This will help me in adding browser tests for settings in multi-profiles
mode.
Thanks!

[michaelpg, 2014-05-07 00:55:05]

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wallpaper_manager_policy_browsertest.cc
(right):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wallpaper_manager_policy_browsertest.cc:184: //
sanestart-up of user profiles.
"sane start-up"

https://codereview.chromium.org/270563002/diff/1/chrome/browser/notifications...
File
chrome/browser/notifications/login_state_notification_blocker_chromeos_browsertest.cc
(left):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/login_state_notification_blocker_chromeos_browsertest.cc:40:
virtual void SetUpOnMainThread() OVERRIDE {
this doesn't call LoginManagerTestSetUpOnMainThread().. maybe it should?

https://codereview.chromium.org/270563002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc
(right):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc:68:
protected:
add back space

[Jun Mukai, 2014-05-07 01:01:41]

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_manager_test_helper.h (right):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_manager_test_helper.h:23: explicit
LoginManagerTestHelper(bool should_launch_browser);
should_launch_browser_ is only referred in SetUpInProcessBrowserTestFixture(). 
Can we move this as a parameter of SetUpLoginUtils()?

https://codereview.chromium.org/270563002/diff/1/chrome/browser/notifications...
File
chrome/browser/notifications/login_state_notification_blocker_chromeos_browsertest.cc
(left):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/login_state_notification_blocker_chromeos_browsertest.cc:40:
virtual void SetUpOnMainThread() OVERRIDE {
On 2014/05/07 00:55:05, Michael Giuffrida wrote:
> this doesn't call LoginManagerTestSetUpOnMainThread().. maybe it should?

Probably. I didn't noticed this missing. Thanks for finding.

[michaelpg, 2014-05-07 02:48:24]

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_manager_test_helper.h (right):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_manager_test_helper.h:23: explicit
LoginManagerTestHelper(bool should_launch_browser);
On 2014/05/07 01:01:41, Jun Mukai wrote:
> should_launch_browser_ is only referred in SetUpInProcessBrowserTestFixture().

> Can we move this as a parameter of SetUpLoginUtils()?

Done.

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/wallpaper_manager_policy_browsertest.cc
(right):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/wallpaper_manager_policy_browsertest.cc:184: //
sanestart-up of user profiles.
On 2014/05/07 00:55:05, Michael Giuffrida wrote:
> "sane start-up"

Done.

https://codereview.chromium.org/270563002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc
(right):

https://codereview.chromium.org/270563002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc:68:
protected:
On 2014/05/07 00:55:05, Michael Giuffrida wrote:
> add back space

Done.

[battre, 2014-05-07 08:16:19]

chrome/browser/net/nss_context_chromeos_browsertest.cc 
LGTM

[Nikita (slow), 2014-05-12 15:23:52]

lgtm

https://codereview.chromium.org/270563002/diff/10001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_manager_test_helper.h (right):

https://codereview.chromium.org/270563002/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_manager_test_helper.h:27: void SetUp();
nit: Should this be named SetUpOnMainThread() to be clear when it is intended to
be called in tests?

https://codereview.chromium.org/270563002/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_manager_test_helper.h:30: void CleanUp();
nit: CleanUpOnMainThread()?

[dzhioev (left Google), 2014-05-12 20:35:52]

On 2014/05/07 00:40:35, Michael Giuffrida wrote:
> PTAL. This will help me in adding browser tests for settings in multi-profiles
> mode.
> Thanks!

Hi Michael,

I think these changes are not good. LoginManagerTest became very hard to use.
To use it correctly you should add portions of code in 5-6 different places of
your test.

I suggest another way: let's make base of LoginManagerTest customizable. It will
make
possible to use WebUIBrowserTest as a base for LoginManagerTest.
I've created draft CL illustrating my proposal
https://codereview.chromium.org/284623002.
How do you like it?

[michaelpg, 2014-05-13 22:13:35]

On 2014/05/12 20:35:52, dzhioev wrote:
> On 2014/05/07 00:40:35, Michael Giuffrida wrote:
> > PTAL. This will help me in adding browser tests for settings in
multi-profiles
> > mode.
> > Thanks!
> 
> Hi Michael,
> 
> I think these changes are not good. LoginManagerTest became very hard to use.
> To use it correctly you should add portions of code in 5-6 different places of
> your test.
> 
> I suggest another way: let's make base of LoginManagerTest customizable. It
will
> make
> possible to use WebUIBrowserTest as a base for LoginManagerTest.
> I've created draft CL illustrating my proposal
> https://codereview.chromium.org/284623002.
> How do you like it?

template <typename Base>
class LoginManagerTestProxy : public Base

This feels weird to me. I haven't seen it before outside of base.

What about creating a separate helper class, and extracting the functionality of
LoginManagerTest into that helper class? Existing tests wouldn't have to be
changed, and my test could derive from WebUIBrowserTest but still be able to use
the helper class.

[dzhioev (left Google), 2014-05-13 23:38:57]

On 2014/05/13 22:13:35, Michael Giuffrida wrote:
> On 2014/05/12 20:35:52, dzhioev wrote:
> > On 2014/05/07 00:40:35, Michael Giuffrida wrote:
> > > PTAL. This will help me in adding browser tests for settings in
> multi-profiles
> > > mode.
> > > Thanks!
> > 
> > Hi Michael,
> > 
> > I think these changes are not good. LoginManagerTest became very hard to
use.
> > To use it correctly you should add portions of code in 5-6 different places
of
> > your test.
> > 
> > I suggest another way: let's make base of LoginManagerTest customizable. It
> will
> > make
> > possible to use WebUIBrowserTest as a base for LoginManagerTest.
> > I've created draft CL illustrating my proposal
> > https://codereview.chromium.org/284623002.
> > How do you like it?
> 
> template <typename Base>
> class LoginManagerTestProxy : public Base
> 
> This feels weird to me. I haven't seen it before outside of base.
> 
> What about creating a separate helper class, and extracting the functionality
of
> LoginManagerTest into that helper class? Existing tests wouldn't have to be
> changed, and my test could derive from WebUIBrowserTest but still be able to
use
> the helper class.
OK, that sounds good.

[michaelpg, 2014-05-16 02:11:00]

OK, this CL is quite different so just compare it with ToT.

I've moved most of the logic in LoginManagerTest to
LoginManagerTestHelper and given LoginManagerTest an instance
of the helper class.

This way the changes are opaque to the existing tests that
subclass LoginManagerTest. And new tests can use
LoginManagerTestHelper without making all of the assumptions
that LoginManagerTest makes.

For an example, see this CL (not for review):
https://codereview.chromium.org/290453006

That test derives from WebUIBrowserTest and doesn't need the
Login UI/OOBE, but it can log in users using the test
helper.

[dzhioev (left Google), 2014-05-16 10:16:47]

On 2014/05/16 02:11:00, Michael Giuffrida wrote:
> OK, this CL is quite different so just compare it with ToT.
> 
> I've moved most of the logic in LoginManagerTest to
> LoginManagerTestHelper and given LoginManagerTest an instance
> of the helper class.
> 
> This way the changes are opaque to the existing tests that
> subclass LoginManagerTest. And new tests can use
> LoginManagerTestHelper without making all of the assumptions
> that LoginManagerTest makes.
> 
> For an example, see this CL (not for review):
> https://codereview.chromium.org/290453006
> 
> That test derives from WebUIBrowserTest and doesn't need the
> Login UI/OOBE, but it can log in users using the test
> helper.

LGTM

[michaelpg, 2014-05-19 20:13:26]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-05-19 20:13:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/270563002/50001

[commit-bot: I haz the power, 2014-05-19 22:04:33]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[michaelpg, 2014-05-19 22:08:36]

The CQ bit was unchecked by michaelpg@chromium.org