Pass focus to browser after login.

Pass focus to browser after login.

BUG=265424
TEST=BrowserLoginTest.BrowserActive

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

[dzhioev (left Google), 2013-08-16 12:17:26]

Hi, please review.

[Nikita (slow), 2013-08-16 14:00:29]

https://codereview.chromium.org/22887021/diff/1/chrome/browser/ui/ash/chrome_...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/22887021/diff/1/chrome/browser/ui/ash/chrome_...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:54:
ash::Shell::GetInstance()->mru_window_tracker()->BuildMruWindowList();
What happens when session has no browser windows?

[oshima, 2013-08-16 14:17:48]

Do we know why/how this is broken now?
If a browser believe it has focus but not really focused,
then there may be some bug in a framework. If that's the
case, we should fix it rather than manually refocusing.

[dzhioev (left Google), 2013-08-16 14:29:59]

On 2013/08/16 14:17:48, oshima wrote:
> Do we know why/how this is broken now?
> If a browser believe it has focus but not really focused,
> then there may be some bug in a framework. If that's the
> case, we should fix it rather than manually refocusing.

At the moment when last window on login screen destructed we still have
(IsUserSessionStarted() == false). It implies (IsUserSessionBlocked() == true)
so browser can't be activated (in fact there are no activatable windows at this
moment). After receiving NOTIFICATION_SESSION_STARTED we can be sure that
(IsUserSessionStarted() == true).

[dzhioev (left Google), 2013-08-16 14:34:37]

Maybe the better way is to notify focus controller that focus restrictions
was changed, but I don't know how to do it.


On Fri, Aug 16, 2013 at 6:29 PM, <dzhioev@chromium.org> wrote:

> On 2013/08/16 14:17:48, oshima wrote:
>
>> Do we know why/how this is broken now?
>> If a browser believe it has focus but not really focused,
>> then there may be some bug in a framework. If that's the
>> case, we should fix it rather than manually refocusing.
>>
>
> At the moment when last window on login screen destructed we still have
> (IsUserSessionStarted() == false). It implies (IsUserSessionBlocked() ==
> true)
> so browser can't be activated (in fact there are no activatable windows at
> this
> moment). After receiving NOTIFICATION_SESSION_STARTED we can be sure that
> (IsUserSessionStarted() == true).
>
>
https://codereview.chromium.**org/22887021/<https://codereview.chromium.org/2...
>



-- 
Pavel Sergeev

[Nikita (slow), 2013-08-19 13:26:06]

chrome/browser/chromeos/login/* lgtm

oshima, could you please help answer Pavel's questions? This bug is marked as
Beta-blocker.

https://codereview.chromium.org/22887021/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_manager_test.h (right):

https://codereview.chromium.org/22887021/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_manager_test.h:13: class LoginManagerTest :
public CrosInProcessBrowserTest {
nit: Please add comment that describes what this test does and when it should be
used as a base class.

[oshima, 2013-08-19 17:28:40]

On 2013/08/19 13:26:06, Nikita Kostylev wrote:
> chrome/browser/chromeos/login/* lgtm
> 
> oshima, could you please help answer Pavel's questions? This bug is marked as
> Beta-blocker.
> 
>
https://codereview.chromium.org/22887021/diff/1/chrome/browser/chromeos/login...
> File chrome/browser/chromeos/login/login_manager_test.h (right):
> 
>
https://codereview.chromium.org/22887021/diff/1/chrome/browser/chromeos/login...
> chrome/browser/chromeos/login/login_manager_test.h:13: class LoginManagerTest
:
> public CrosInProcessBrowserTest {
> nit: Please add comment that describes what this test does and when it should
be
> used as a base class.

I'll look into focus code and get back to you EOD

[oshima, 2013-08-19 23:56:24]

The root cause was that we change the login state after re restore the session.

https://codereview.chromium.org/22909019/diff/1/chrome/browser/chromeos/login...

I believe the code predates aura, and was necessary at that time
because closing the login window used to shutdown the chrome.
This is no longer the case, so it must be safe (and probably better
as the restore happens with correct state) to change this now. Nikita, what do
you think?

[Nikita (slow), 2013-08-20 09:41:41]

On 2013/08/19 23:56:24, oshima wrote:
> The root cause was that we change the login state after re restore the
session.
> 
>
https://codereview.chromium.org/22909019/diff/1/chrome/browser/chromeos/login...
> 
> I believe the code predates aura, and was necessary at that time
> because closing the login window used to shutdown the chrome.
> This is no longer the case, so it must be safe (and probably better
> as the restore happens with correct state) to change this now. Nikita, what do
> you think?

I've incorporated your changes into this CL and removed changes in Ash. New test
is failing. Investigating.

[dzhioev (left Google), 2013-08-20 10:34:21]

On 2013/08/19 23:56:24, oshima wrote:
> The root cause was that we change the login state after re restore the
session.
> 
>
https://codereview.chromium.org/22909019/diff/1/chrome/browser/chromeos/login...
> 
> I believe the code predates aura, and was necessary at that time
> because closing the login window used to shutdown the chrome.
> This is no longer the case, so it must be safe (and probably better
> as the restore happens with correct state) to change this now. Nikita, what do
> you think?

It is not safe to start session before browser launch, because all Chrome OS
preferences are initialized in LaunchBrowser ( http://goo.gl/27gGP8 ). I wonder
why it works at all.

[Nikita (slow), 2013-08-20 10:41:57]

On 2013/08/20 10:34:21, dzhioev wrote:
> On 2013/08/19 23:56:24, oshima wrote:
> > The root cause was that we change the login state after re restore the
> session.
> > 
> >
>
https://codereview.chromium.org/22909019/diff/1/chrome/browser/chromeos/login...
> > 
> > I believe the code predates aura, and was necessary at that time
> > because closing the login window used to shutdown the chrome.
> > This is no longer the case, so it must be safe (and probably better
> > as the restore happens with correct state) to change this now. Nikita, what
do
> > you think?
> 
> It is not safe to start session before browser launch, because all Chrome OS
> preferences are initialized in LaunchBrowser ( http://goo.gl/27gGP8 ). I
wonder
> why it works at all.

Yes, I've also noticed other side effects like session launcher is shown below
login UI controls.

We want to merge this fix to M30 branch and I don't want to break now assumption
that SessionStarted() is called after  LaunchBrowser() which has been true for a
while now. I expect there would be side effects which will also require fixes on
branch which is not the best strategy.

This bug has been marked as Beta/Stable blocker so I propose to go with solution
Pavel found to be working (and there's a test that checks that) and continue
investigations on what would be the right fix on trunk in M31.

[Nikita (slow), 2013-08-20 10:53:50]

oshima, I've returned ash changes. Please take a look.

[oshima, 2013-08-20 14:56:01]

Ok, please investigate and work on right fix on m31.

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager_test.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager_test.cc:54:
controller->Login(UserContext(username, password, ""));
/""/std::string()/

assuming the method takes const std::string&.

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/session_login_browsertest.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:13: 
namespace chromeos {

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:16: const char*
kTestUser = "test-user@gmail.com";
char[]

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:22:
BrowserLoginTest() : LoginManagerTest(true) {}
virtual BrowserLOginTest() {}

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:41:
EXPECT_TRUE(browser->window()->IsActive());
can you test if the omnibox has focus as well? (they're similar but different
concept)

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:50: // It's needed
because some windows can be opened in background while login UI
because we currently restore browser windows before Login UI is deleted.

[Nikita (slow), 2013-08-20 16:04:39]

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager_test.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager_test.cc:54:
controller->Login(UserContext(username, password, ""));
On 2013/08/20 14:56:01, oshima wrote:
> /""/std::string()/
> 
> assuming the method takes const std::string&.

Done.

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/session_login_browsertest.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:13: 
On 2013/08/20 14:56:01, oshima wrote:
> namespace chromeos {

Done.

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:16: const char*
kTestUser = "test-user@gmail.com";
On 2013/08/20 14:56:01, oshima wrote:
> char[]

Done.

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:22:
BrowserLoginTest() : LoginManagerTest(true) {}
On 2013/08/20 14:56:01, oshima wrote:
> virtual BrowserLOginTest() {}

Do you mean virtual destructor?

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:41:
EXPECT_TRUE(browser->window()->IsActive());
On 2013/08/20 14:56:01, oshima wrote:
> can you test if the omnibox has focus as well? (they're similar but different
> concept)

Done.

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:50: // It's needed
because some windows can be opened in background while login UI
On 2013/08/20 14:56:01, oshima wrote:
> because we currently restore browser windows before Login UI is deleted.

Done.

[Nikita (slow), 2013-08-20 16:06:58]

On 2013/08/20 16:04:39, Nikita Kostylev wrote:
>
https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
> chrome/browser/chromeos/login/session_login_browsertest.cc:41:
> EXPECT_TRUE(browser->window()->IsActive());
> On 2013/08/20 14:56:01, oshima wrote:
> > can you test if the omnibox has focus as well? (they're similar but
different
> > concept)
> 
> Done.

Not uploading patchset as it violates checkdeps rules:
"-chrome/browser/ui/views" from chrome/browser's include_rules

[Nikita (slow), 2013-08-20 17:38:05]

Added omnibox check.

[oshima, 2013-08-20 17:52:47]

lgtm with nits

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/session_login_browsertest.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:22:
BrowserLoginTest() : LoginManagerTest(true) {}
On 2013/08/20 16:04:39, Nikita Kostylev wrote:
> On 2013/08/20 14:56:01, oshima wrote:
> > virtual BrowserLOginTest() {}
> 
> Do you mean virtual destructor?

yep sorry for typo.

https://codereview.chromium.org/22887021/diff/39001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager_test.cc (right):

https://codereview.chromium.org/22887021/diff/39001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager_test.cc:60: }
nit: nuke {}

[Nikita (slow), 2013-08-20 18:04:21]

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/session_login_browsertest.cc (right):

https://codereview.chromium.org/22887021/diff/28001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/session_login_browsertest.cc:22:
BrowserLoginTest() : LoginManagerTest(true) {}
On 2013/08/20 17:52:47, oshima wrote:
> On 2013/08/20 16:04:39, Nikita Kostylev wrote:
> > On 2013/08/20 14:56:01, oshima wrote:
> > > virtual BrowserLOginTest() {}
> > 
> > Do you mean virtual destructor?
> 
> yep sorry for typo.

Done.

https://codereview.chromium.org/22887021/diff/39001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager_test.cc (right):

https://codereview.chromium.org/22887021/diff/39001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager_test.cc:60: }
On 2013/08/20 17:52:47, oshima wrote:
> nit: nuke {}

Done.

[commit-bot: I haz the power, 2013-08-21 08:27:10]

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

[commit-bot: I haz the power, 2013-08-21 09:02:46]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nikita (slow), 2013-08-21 09:05:22]

+jamescook for OWNERS review of chrome/browser/ui/ash/*

[James Cook, 2013-08-21 16:31:49]

c/b/ui/ash LGTM with nit

https://chromiumcodereview.appspot.com/22887021/diff/50001/chrome/browser/ui/...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://chromiumcodereview.appspot.com/22887021/diff/50001/chrome/browser/ui/...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:49: // This function
used for restoring focus after user session was started.
nit: "This function is used for restoring focus after the user session is
started."

[Nikita (slow), 2013-08-21 17:23:24]

https://codereview.chromium.org/22887021/diff/50001/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/22887021/diff/50001/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:49: // This function
used for restoring focus after user session was started.
On 2013/08/21 16:31:49, James Cook wrote:
> nit: "This function is used for restoring focus after the user session is
> started."

Done.

[commit-bot: I haz the power, 2013-08-21 17:24:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/22887021/55001

[commit-bot: I haz the power, 2013-08-21 20:38:40]

Change committed as 218823