Focus kept on login screen.

Focus kept on login screen.

BUG=253811
TEST=manually

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

[dzhioev (left Google), 2013-07-08 15:43:29]

+sky@
Scott, we had a problem that window opened in user session stole focus from
login screen. I've fixed it by observing for focused window and returning focus
if it was stolen from login screen. I can't found better way to do keep focus.
Do you know if better way exists?

[sky, 2013-07-08 17:15:18]

This definitely isn't the right way to go about this. You should
investigate why BaseFocusRules allowed the focus change. That'll help
you understand what is happening during the OOBE. Also, you'll want a
test case for this.

  -Scott

On Mon, Jul 8, 2013 at 8:43 AM,  <dzhioev@chromium.org> wrote:
> Reviewers: Nikita Kostylev,
>
> Message:
> +sky@
> Scott, we had a problem that window opened in user session stole focus from
> login screen. I've fixed it by observing for focused window and returning
> focus
> if it was stolen from login screen. I can't found better way to do keep
> focus.
> Do you know if better way exists?
>
> Description:
> Focus kept on login screen.
>
> BUG=253811
> TEST=manually
>
> Please review this at https://codereview.chromium.org/18850003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   A chrome/browser/chromeos/login/focus_keeper.h
>   A chrome/browser/chromeos/login/focus_keeper.cc
>   M chrome/browser/chromeos/login/webui_login_view.h
>   M chrome/browser/chromeos/login/webui_login_view.cc
>   M chrome/chrome_browser_chromeos.gypi
>
>

[dzhioev (left Google), 2013-07-11 18:51:35]

Hi Scott,

I've found the reason why user session window wad stealing focus from login
screen.
It was happenning because of error in
EventClientImpl::CanProcessEventsWithinSubtree.
Can you suggest a place where to put test case for this? 

On 2013/07/08 17:15:18, sky wrote:
> This definitely isn't the right way to go about this. You should
> investigate why BaseFocusRules allowed the focus change. That'll help
> you understand what is happening during the OOBE. Also, you'll want a
> test case for this.
> 
>   -Scott
> 
> On Mon, Jul 8, 2013 at 8:43 AM,  <mailto:dzhioev@chromium.org> wrote:
> > Reviewers: Nikita Kostylev,
> >
> > Message:
> > +sky@
> > Scott, we had a problem that window opened in user session stole focus from
> > login screen. I've fixed it by observing for focused window and returning
> > focus
> > if it was stolen from login screen. I can't found better way to do keep
> > focus.
> > Do you know if better way exists?
> >
> > Description:
> > Focus kept on login screen.
> >
> > BUG=253811
> > TEST=manually
> >
> > Please review this at https://codereview.chromium.org/18850003/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files:
> >   A chrome/browser/chromeos/login/focus_keeper.h
> >   A chrome/browser/chromeos/login/focus_keeper.cc
> >   M chrome/browser/chromeos/login/webui_login_view.h
> >   M chrome/browser/chromeos/login/webui_login_view.cc
> >   M chrome/chrome_browser_chromeos.gypi
> >
> >

[dzhioev (left Google), 2013-07-15 13:02:29]

Scott, adding you to reviewers. Please suggest a place where to put test case
for fix. It seems like focus_controller_unittest and focus_manager_unittest are
general tests, but my fix is Ash-specific.

[sky, 2013-07-15 19:39:19]

What does the complete stack look like?

On Thu, Jul 11, 2013 at 11:51 AM,  <dzhioev@chromium.org> wrote:
> Hi Scott,
>
> I've found the reason why user session window wad stealing focus from login
> screen.
> It was happenning because of error in
> EventClientImpl::CanProcessEventsWithinSubtree.
> Can you suggest a place where to put test case for this?
>
>
> On 2013/07/08 17:15:18, sky wrote:
>>
>> This definitely isn't the right way to go about this. You should
>> investigate why BaseFocusRules allowed the focus change. That'll help
>> you understand what is happening during the OOBE. Also, you'll want a
>> test case for this.
>
>
>>    -Scott
>
>
>> On Mon, Jul 8, 2013 at 8:43 AM,  <mailto:dzhioev@chromium.org> wrote:
>> > Reviewers: Nikita Kostylev,
>> >
>> > Message:
>> > +sky@
>> > Scott, we had a problem that window opened in user session stole focus
>> > from
>> > login screen. I've fixed it by observing for focused window and
>> > returning
>> > focus
>> > if it was stolen from login screen. I can't found better way to do keep
>> > focus.
>> > Do you know if better way exists?
>> >
>> > Description:
>> > Focus kept on login screen.
>> >
>> > BUG=253811
>> > TEST=manually
>> >
>> > Please review this at https://codereview.chromium.org/18850003/
>> >
>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
>> >
>> > Affected files:
>> >   A chrome/browser/chromeos/login/focus_keeper.h
>> >   A chrome/browser/chromeos/login/focus_keeper.cc
>> >   M chrome/browser/chromeos/login/webui_login_view.h
>> >   M chrome/browser/chromeos/login/webui_login_view.cc
>> >   M chrome/chrome_browser_chromeos.gypi
>> >
>> >
>
>
>
> https://codereview.chromium.org/18850003/

[dzhioev (left Google), 2013-07-15 19:48:41]

Something like:

ash::internal::EventClientImpl::CanProcessEventsWithinSubtree
aura::Window::CanFocus
views::corewm::BaseFocusRules::CanFocusWindow
views::corewm::BaseFocusRules::GetFocusableWindow
views::corewm::FocusController::FocusWindow
aura::Window::Focus

On Monday, July 15, 2013, Scott Violet wrote:

> What does the complete stack look like?
>
> On Thu, Jul 11, 2013 at 11:51 AM,  <dzhioev@chromium.org> wrote:
> > Hi Scott,
> >
> > I've found the reason why user session window wad stealing focus from
> login
> > screen.
> > It was happenning because of error in
> > EventClientImpl::CanProcessEventsWithinSubtree.
> > Can you suggest a place where to put test case for this?
> >
> >
> > On 2013/07/08 17:15:18, sky wrote:
> >>
> >> This definitely isn't the right way to go about this. You should
> >> investigate why BaseFocusRules allowed the focus change. That'll help
> >> you understand what is happening during the OOBE. Also, you'll want a
> >> test case for this.
> >
> >
> >>    -Scott
> >
> >
> >> On Mon, Jul 8, 2013 at 8:43 AM,  <mailto:dzhioev@chromium.org> wrote:
> >> > Reviewers: Nikita Kostylev,
> >> >
> >> > Message:
> >> > +sky@
> >> > Scott, we had a problem that window opened in user session stole focus
> >> > from
> >> > login screen. I've fixed it by observing for focused window and
> >> > returning
> >> > focus
> >> > if it was stolen from login screen. I can't found better way to do
> keep
> >> > focus.
> >> > Do you know if better way exists?
> >> >
> >> > Description:
> >> > Focus kept on login screen.
> >> >
> >> > BUG=253811
> >> > TEST=manually
> >> >
> >> > Please review this at https://codereview.chromium.org/18850003/
> >> >
> >> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >> >
> >> > Affected files:
> >> >   A chrome/browser/chromeos/login/focus_keeper.h
> >> >   A chrome/browser/chromeos/login/focus_keeper.cc
> >> >   M chrome/browser/chromeos/login/webui_login_view.h
> >> >   M chrome/browser/chromeos/login/webui_login_view.cc
> >> >   M chrome/chrome_browser_chromeos.gypi
> >> >
> >> >
> >
> >
> >
> > https://codereview.chromium.org/18850003/
>

[sky, 2013-07-15 21:00:20]

I haven't looked at the OOBE code, but it seems like it should be
similar to how system modal dialogs keep focus from moving. You should
look at the code.

  -Scott

On Mon, Jul 15, 2013 at 12:48 PM, Pavel Sergeev <dzhioev@chromium.org> wrote:
> Something like:
>
> ash::internal::EventClientImpl::CanProcessEventsWithinSubtree
> aura::Window::CanFocus
> views::corewm::BaseFocusRules::CanFocusWindow
> views::corewm::BaseFocusRules::GetFocusableWindow
> views::corewm::FocusController::FocusWindow
> aura::Window::Focus
>
> On Monday, July 15, 2013, Scott Violet wrote:
>>
>> What does the complete stack look like?
>>
>> On Thu, Jul 11, 2013 at 11:51 AM,  <dzhioev@chromium.org> wrote:
>> > Hi Scott,
>> >
>> > I've found the reason why user session window wad stealing focus from
>> > login
>> > screen.
>> > It was happenning because of error in
>> > EventClientImpl::CanProcessEventsWithinSubtree.
>> > Can you suggest a place where to put test case for this?
>> >
>> >
>> > On 2013/07/08 17:15:18, sky wrote:
>> >>
>> >> This definitely isn't the right way to go about this. You should
>> >> investigate why BaseFocusRules allowed the focus change. That'll help
>> >> you understand what is happening during the OOBE. Also, you'll want a
>> >> test case for this.
>> >
>> >
>> >>    -Scott
>> >
>> >
>> >> On Mon, Jul 8, 2013 at 8:43 AM,  <mailto:dzhioev@chromium.org> wrote:
>> >> > Reviewers: Nikita Kostylev,
>> >> >
>> >> > Message:
>> >> > +sky@
>> >> > Scott, we had a problem that window opened in user session stole
>> >> > focus
>> >> > from
>> >> > login screen. I've fixed it by observing for focused window and
>> >> > returning
>> >> > focus
>> >> > if it was stolen from login screen. I can't found better way to do
>> >> > keep
>> >> > focus.
>> >> > Do you know if better way exists?
>> >> >
>> >> > Description:
>> >> > Focus kept on login screen.
>> >> >
>> >> > BUG=253811
>> >> > TEST=manually
>> >> >
>> >> > Please review this at https://codereview.chromium.org/18850003/
>> >> >
>> >> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
>> >> >
>> >> > Affected files:
>> >> >   A chrome/browser/chromeos/login/focus_keeper.h
>> >> >   A chrome/browser/chromeos/login/focus_keeper.cc
>> >> >   M chrome/browser/chromeos/login/webui_login_view.h
>> >> >   M chrome/browser/chromeos/login/webui_login_view.cc
>> >> >   M chrome/chrome_browser_chromeos.gypi
>> >> >
>> >> >
>> >
>> >
>> >
>> > https://codereview.chromium.org/18850003/

[dzhioev_at_google, 2013-07-15 21:21:26]

I can't understand what you talking about. Why OOBE? I've already provided
code that fixes issue in Ash part.

On Tuesday, July 16, 2013, Scott Violet wrote:

> I haven't looked at the OOBE code, but it seems like it should be
> similar to how system modal dialogs keep focus from moving. You should
> look at the code.
>
>   -Scott
>
> On Mon, Jul 15, 2013 at 12:48 PM, Pavel Sergeev
<dzhioev@chromium.org<javascript:;>>
> wrote:
> > Something like:
> >
> > ash::internal::EventClientImpl::CanProcessEventsWithinSubtree
> > aura::Window::CanFocus
> > views::corewm::BaseFocusRules::CanFocusWindow
> > views::corewm::BaseFocusRules::GetFocusableWindow
> > views::corewm::FocusController::FocusWindow
> > aura::Window::Focus
> >
> > On Monday, July 15, 2013, Scott Violet wrote:
> >>
> >> What does the complete stack look like?
> >>
> >> On Thu, Jul 11, 2013 at 11:51 AM,  <dzhioev@chromium.org <javascript:;>>
> wrote:
> >> > Hi Scott,
> >> >
> >> > I've found the reason why user session window wad stealing focus from
> >> > login
> >> > screen.
> >> > It was happenning because of error in
> >> > EventClientImpl::CanProcessEventsWithinSubtree.
> >> > Can you suggest a place where to put test case for this?
> >> >
> >> >
> >> > On 2013/07/08 17:15:18, sky wrote:
> >> >>
> >> >> This definitely isn't the right way to go about this. You should
> >> >> investigate why BaseFocusRules allowed the focus change. That'll help
> >> >> you understand what is happening during the OOBE. Also, you'll want a
> >> >> test case for this.
> >> >
> >> >
> >> >>    -Scott
> >> >
> >> >
> >> >> On Mon, Jul 8, 2013 at 8:43 AM, 
<mailto:dzhioev@chromium.org<javascript:;>>
> wrote:
> >> >> > Reviewers: Nikita Kostylev,
> >> >> >
> >> >> > Message:
> >> >> > +sky@
> >> >> > Scott, we had a problem that window opened in user session stole
> >> >> > focus
> >> >> > from
> >> >> > login screen. I've fixed it by observing for focused window and
> >> >> > returning
> >> >> > focus
> >> >> > if it was stolen from login screen. I can't found better way to do
> >> >> > keep
> >> >> > focus.
> >> >> > Do you know if better way exists?
> >> >> >
> >> >> > Description:
> >> >> > Focus kept on login screen.
> >> >> >
> >> >> > BUG=253811
> >> >> > TEST=manually
> >> >> >
> >> >> > Please review this at https://codereview.chromium.org/18850003/
> >> >> >
> >> >> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >> >> >
> >> >> > Affected files:
> >> >> >   A chrome/browser/chromeos/login/focus_keeper.h
> >> >> >   A chrome/browser/chromeos/login/focus_keeper.cc
> >> >> >   M chrome/browser/chromeos/login/webui_login_view.h
> >> >> >   M chrome/browser/chromeos/login/webui_login_view.cc
> >> >> >   M chrome/chrome_browser_chromeos.gypi
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >> > https://codereview.chromium.org/18850003/
>


-- 
Pavel Sergeev
teams/dzhioev

[sky, 2013-07-15 22:23:35]

Sorry, this looks right. I'm hoping we can avoid a new method on
SessionStateDelegate though.

https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:60:
chromeos::UserAddingScreen::Get()->IsRunning();
Is that last statement really needed?

[dzhioev (left Google), 2013-07-16 09:54:36]

About IsUserSessionBlocked: I want to use in following CL that fixes problem
related to user adding UI and modal windows.

https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:60:
chromeos::UserAddingScreen::Get()->IsRunning();
On 2013/07/15 22:23:36, sky wrote:
> Is that last statement really needed?
Yes, it is new UI similar to login/oobe/lock needed to add users into
multi-profile session.

[dzhioev (left Google), 2013-07-16 10:04:11]

On 2013/07/16 09:54:36, dzhioev wrote:
> About IsUserSessionBlocked: I want to use in following CL that fixes problem
> related to user adding UI and modal windows.
> 
>
https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
> File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):
> 
>
https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
> chrome/browser/ui/ash/session_state_delegate_chromeos.cc:60:
> chromeos::UserAddingScreen::Get()->IsRunning();
> On 2013/07/15 22:23:36, sky wrote:
> > Is that last statement really needed?
> Yes, it is new UI similar to login/oobe/lock needed to add users into
> multi-profile session.

Fix: I want to use *it* in following CL

[sky, 2013-07-16 13:45:08]

Please add it that patch. That way we can keep this one focused on
just what is needed now.

On Tue, Jul 16, 2013 at 2:54 AM,  <dzhioev@chromium.org> wrote:
> About IsUserSessionBlocked: I want to use in following CL that fixes problem
> related to user adding UI and modal windows.
>
>
>
>
https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
> File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):
>
>
https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
> chrome/browser/ui/ash/session_state_delegate_chromeos.cc:60:
> chromeos::UserAddingScreen::Get()->IsRunning();
> On 2013/07/15 22:23:36, sky wrote:
>>
>> Is that last statement really needed?
>
> Yes, it is new UI similar to login/oobe/lock needed to add users into
> multi-profile session.
>
> https://codereview.chromium.org/18850003/

[dzhioev (left Google), 2013-07-17 13:38:52]

On 2013/07/16 13:45:08, sky wrote:
> Please add it that patch. That way we can keep this one focused on
> just what is needed now.
> 
> On Tue, Jul 16, 2013 at 2:54 AM,  <mailto:dzhioev@chromium.org> wrote:
> > About IsUserSessionBlocked: I want to use in following CL that fixes problem
> > related to user adding UI and modal windows.
> >
> >
> >
> >
>
https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
> > File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):
> >
> >
>
https://codereview.chromium.org/18850003/diff/10001/chrome/browser/ui/ash/ses...
> > chrome/browser/ui/ash/session_state_delegate_chromeos.cc:60:
> > chromeos::UserAddingScreen::Get()->IsRunning();
> > On 2013/07/15 22:23:36, sky wrote:
> >>
> >> Is that last statement really needed?
> >
> > Yes, it is new UI similar to login/oobe/lock needed to add users into
> > multi-profile session.
> >
> > https://codereview.chromium.org/18850003/

Done. Also added test.

[sky, 2013-07-17 15:55:07]

LGTM

[commit-bot: I haz the power, 2013-07-17 16:01:00]

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

[commit-bot: I haz the power, 2013-07-17 18:20:58]

Retried try job too often on linux_chromeos for step(s) browser_tests,
interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-07-18 10:34:57]

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

[commit-bot: I haz the power, 2013-07-18 11:54:05]

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

[commit-bot: I haz the power, 2013-07-18 14:00:54]

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

[commit-bot: I haz the power, 2013-07-18 17:34:47]

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

[commit-bot: I haz the power, 2013-07-19 15:48:25]

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

[commit-bot: I haz the power, 2013-07-19 20:49:08]

Change committed as 212651