Login screen IME policy does not apply in user session

Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on if the
  sign-in screen is visible. This prevents a SigninScreenHandler
  instance which exists during the user session from enforcing input 
  methods in the user session.
+ SigninScreenHandler re-enforces input methods on the "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324, 709313
TEST=Manual test && browser_tests --gtest_filter=LoginScreen*

Review-Url: https://codereview.chromium.org/2816543004
Cr-Commit-Position: refs/heads/master@{#464204}
Committed: https://chromium.googlesource.com/chromium/src/+/ca7672ad2527bb65ca0a1bc76d8e369f4a4c0729

[pmarko, 2017-04-12 13:05:56]

Description was changed from

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on its own IME
  state and only if the sign-in screen is visible. This prevents a
  SigninScreenHandler instance which exists during the user session from
  enforcing input methods in the user session.
+ SigninScreenHandler re-enforces input methods when on "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.

BUG=373324,709313
TEST=Manual test
==========

to

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on its own IME
  state and only if the sign-in screen is visible. This prevents a
  SigninScreenHandler instance which exists during the user session from
  enforcing input methods in the user session.
+ SigninScreenHandler re-enforces input methods when on "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test
==========

[pmarko, 2017-04-12 13:26:09]

The CQ bit was checked by pmarko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 13:26:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pmarko, 2017-04-12 13:28:03]

Description was changed from

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on its own IME
  state and only if the sign-in screen is visible. This prevents a
  SigninScreenHandler instance which exists during the user session from
  enforcing input methods in the user session.
+ SigninScreenHandler re-enforces input methods when on "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test
==========

to

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on if the
  sign-in screen is visible. This prevents a SigninScreenHandler
  instance which exists during the user session from enforcing input 
  methods in the user session.
+ SigninScreenHandler re-enforces input methods on the "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test
==========

[commit-bot: I haz the power, 2017-04-12 14:44:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 14:44:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pmarko, 2017-04-12 16:30:10]

The CQ bit was checked by pmarko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 16:31:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pmarko, 2017-04-12 16:35:13]

Description was changed from

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on if the
  sign-in screen is visible. This prevents a SigninScreenHandler
  instance which exists during the user session from enforcing input 
  methods in the user session.
+ SigninScreenHandler re-enforces input methods on the "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test
==========

to

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on if the
  sign-in screen is visible. This prevents a SigninScreenHandler
  instance which exists during the user session from enforcing input 
  methods in the user session.
+ SigninScreenHandler re-enforces input methods on the "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test && browser_tests --gtest_filter=LoginScreen*
==========

[pmarko, 2017-04-12 16:39:37]

pmarko@chromium.org changed reviewers:
+ xiyuan@chromium.org

[pmarko, 2017-04-12 16:39:37]

Hello xiyuan,
please review the bugfix. The original CL (2652793003) was written under the
assumption that SigninScreenHandler only exists on the sign-in screen, which
turned out to be wrong.
As a consequence, a SigninScreenHandler instance which is active during user
session is messing with the user's input methods.
This CL fixes that by only messing with input methods if the sign-in screen
webui is actually visible.

Thanks!

[commit-bot: I haz the power, 2017-04-12 17:40:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 17:40:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2017-04-12 17:42:06]

xiyuan@chromium.org changed reviewers:
+ jdufault@chromium.org

[xiyuan, 2017-04-12 17:42:07]

+jdufault

> please review the bugfix. The original CL (2652793003) was written under the
> assumption that SigninScreenHandler only exists on the sign-in screen, which
> turned out to be wrong.

I think it probably because we preload the lock screen after user session is
started. 

jdufault@, do you know whether SigninScreenHandler::HandleLoginVisible would be
called for the preloaded lock screen before it is shown?

https://codereview.chromium.org/2816543004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/2816543004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:883:
imm->GetInputMethodUtil()->GetHardwareLoginInputMethodIds();
nit: wrap with {} and else since this takes more than one line

[jdufault, 2017-04-12 18:13:10]

On 2017/04/12 17:42:07, xiyuan wrote:
> +jdufault
> 
> > please review the bugfix. The original CL (2652793003) was written under the
> > assumption that SigninScreenHandler only exists on the sign-in screen, which
> > turned out to be wrong.
> 
> I think it probably because we preload the lock screen after user session is
> started. 
> 
> jdufault@, do you know whether SigninScreenHandler::HandleLoginVisible would
be
> called for the preloaded lock screen before it is shown?

SigninScreenHandler::HandleLoginVisible is not called before the webui is
visible. I don't believe we are sending any initialization messages to webui
when preloading so very few/none of the callbacks fire.

Instead of trying to figure out if login is actually visible you could hook into
the WizardController set of classes which should only be instantiated when
oobe/login/lock/user-add is being displayed.

[pmarko, 2017-04-12 19:01:19]

The CQ bit was checked by pmarko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 19:03:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pmarko, 2017-04-12 19:05:36]

Yes, the behavior is definitely related to lock screen preloading (which is also
why it was hard to detect - it is not triggered immediately).

I can confirm that manual testing also shows that the SigninScreenHandler's
HandleLoginVisible shows the expected behavior (not triggered during lock screen
preload).

If you're fine with it I would use this solution for now to reach the M59 branch
and possibly also merge back to fix on M58 and investigate other approaches
afterwards.

https://codereview.chromium.org/2816543004/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/2816543004/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:883:
imm->GetInputMethodUtil()->GetHardwareLoginInputMethodIds();
On 2017/04/12 17:42:07, xiyuan wrote:
> nit: wrap with {} and else since this takes more than one line

Done.

[xiyuan, 2017-04-12 19:33:44]

Thanks jdufault@ for chiming in and pmarko@ to confirm.

I am fine with the solution then.

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_screen_policy_browsertest.cc (left):

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_screen_policy_browsertest.cc:37:
command_line->AppendSwitch(switches::kForceLoginManagerInTests);
This would make the browser test to run in a stub user session. Calling
ShowLoginWizard later is not a well tested code path.

The failure is because switches::kLoginManager would cause login screen to be
created and ShowLoginWizard attempts to create it again. Think we should be just
remove ShowLoginWizard.

If OobeScreenWaiter does not work, we can use

  content::WindowedNotificationObserver(
      chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
      content::NotificationService::AllSources()).Wait();

, that are used by a few login related browser tests.

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_screen_policy_browsertest.cc (right):

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_screen_policy_browsertest.cc:119: FROM_HERE,
LoginDisplayHost::default_host());
Think this is not necessary.

[commit-bot: I haz the power, 2017-04-12 19:34:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:34:57]

Dry run: This issue passed the CQ dry run.

[pmarko, 2017-04-12 20:29:32]

As always, you are right :) this is much simpler now and works just fine (at
least locally). I tricked myself into the complicated solution because of
OobeScreenWaiter not proceeding.

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_screen_policy_browsertest.cc (left):

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_screen_policy_browsertest.cc:37:
command_line->AppendSwitch(switches::kForceLoginManagerInTests);
On 2017/04/12 19:33:44, xiyuan wrote:
> This would make the browser test to run in a stub user session. Calling
> ShowLoginWizard later is not a well tested code path.
> 
> The failure is because switches::kLoginManager would cause login screen to be
> created and ShowLoginWizard attempts to create it again. Think we should be
just
> remove ShowLoginWizard.
> 
> If OobeScreenWaiter does not work, we can use
> 
>   content::WindowedNotificationObserver(
>       chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
>       content::NotificationService::AllSources()).Wait();
> 
> , that are used by a few login related browser tests.

Done.

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_screen_policy_browsertest.cc (right):

https://codereview.chromium.org/2816543004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_screen_policy_browsertest.cc:119: FROM_HERE,
LoginDisplayHost::default_host());
On 2017/04/12 19:33:44, xiyuan wrote:
> Think this is not necessary.

Done.

[xiyuan, 2017-04-12 20:31:41]

lgtm

Thanks.

[pmarko, 2017-04-12 20:53:39]

The CQ bit was checked by pmarko@chromium.org

[commit-bot: I haz the power, 2017-04-12 20:54:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 23:24:29]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1492030419497760,
"parent_rev": "61beb8b2b22b455697a478b0e664a435efe16b07", "commit_rev":
"ca7672ad2527bb65ca0a1bc76d8e369f4a4c0729"}

[commit-bot: I haz the power, 2017-04-12 23:25:25]

Description was changed from

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on if the
  sign-in screen is visible. This prevents a SigninScreenHandler
  instance which exists during the user session from enforcing input 
  methods in the user session.
+ SigninScreenHandler re-enforces input methods on the "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test && browser_tests --gtest_filter=LoginScreen*
==========

to

==========
Login screen IME policy does not apply in user session

This CL fixes issues with the DeviceLoginScreenInputMethods policy:
+ SigninScreenHandler is now only enforcing input methods on if the
  sign-in screen is visible. This prevents a SigninScreenHandler
  instance which exists during the user session from enforcing input 
  methods in the user session.
+ SigninScreenHandler re-enforces input methods on the "addUser" event,
  because if only one user pod is present on the sign-in screen,
  pressing addUser does not de-focus the user pod before switching to
  the gaia screen.
+ Prefer policy-allowed input methods on the gaia screen ("Add Person")

BUG=373324,709313
TEST=Manual test && browser_tests --gtest_filter=LoginScreen*

Review-Url: https://codereview.chromium.org/2816543004
Cr-Commit-Position: refs/heads/master@{#464204}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca7672ad2527bb65ca0a1bc76d8e...
==========

[commit-bot: I haz the power, 2017-04-12 23:25:27]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/ca7672ad2527bb65ca0a1bc76d8e...