cros: Allow JS to control virtual keyboard on sign-in screen.

cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=623344
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2814863002
Cr-Commit-Position: refs/heads/master@{#465707}
Committed: https://chromium.googlesource.com/chromium/src/+/3bb91b6589418cfe73938b74369b520324840ecd

[jdufault, 2017-04-11 20:20:08]

Description was changed from

==========
cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=
==========

to

==========
cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2017-04-11 20:20:11]

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

[commit-bot: I haz the power, 2017-04-11 20:20:55]

Dry run: 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-11 20:24:17]

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

[commit-bot: I haz the power, 2017-04-11 20:24:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jdufault, 2017-04-11 22:08:33]

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

[commit-bot: I haz the power, 2017-04-11 22:09:04]

Dry run: 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 00:38:24]

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

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

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-04-12 18:34:24]

Patchset #1 (id:1) has been deleted

[jdufault, 2017-04-12 18:34:46]

Description was changed from

==========
cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=623344
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2017-04-12 18:38:55]

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

[jdufault, 2017-04-12 18:40:12]

achuith@ PTAL. Thanks

[achuithb, 2017-04-12 19:00:26]

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:427: if
(keyboard::KeyboardController::GetInstance()) {
Let's use a temp var for this?

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:431: is_observing_keyboard_
= true;
Is the purpose of is_observing_keyboard_ just to prevent double insertion into
KeyboardController's ObserverList? We could just use HasObserver instead:
auto keyboard_controller = keyboard::KeyboardController::GetInstance();
if (!keyboard_controller->HasObserver(this))
  keyboard_controller->AddObserver(this);

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:447:
GetOobeUI()->GetCoreOobeView()->ShowControlBar(true);
Use a temp var for this?

CoreOobeView* core_oobe_view = GetOobeUI() ? GetOObeUI()->GetCoreOobeView() :
nullptr;

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/login_shared.js (right):

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/login_shared.js:415: * Shows/hides pin
keyboard on the lock screen.
Also update comment

[jdufault, 2017-04-12 19:31:30]

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

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

Dry run: 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 21:48:33]

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

[commit-bot: I haz the power, 2017-04-12 21:48:34]

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

[jdufault, 2017-04-12 22:26:09]

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

[commit-bot: I haz the power, 2017-04-12 22:27:01]

Dry run: 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:44:01]

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

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

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-04-13 00:33:56]

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

[jdufault, 2017-04-13 00:34:41]

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

[jdufault, 2017-04-13 00:34:42]

cc xiyuan@ for mash workaround

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:427: if
(keyboard::KeyboardController::GetInstance()) {
On 2017/04/12 19:00:26, achuithb wrote:
> Let's use a temp var for this?

Done.

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:431: is_observing_keyboard_
= true;
On 2017/04/12 19:00:26, achuithb wrote:
> Is the purpose of is_observing_keyboard_ just to prevent double insertion into
> KeyboardController's ObserverList? We could just use HasObserver instead:
> auto keyboard_controller = keyboard::KeyboardController::GetInstance();
> if (!keyboard_controller->HasObserver(this))
>   keyboard_controller->AddObserver(this);

Done.

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:447:
GetOobeUI()->GetCoreOobeView()->ShowControlBar(true);
On 2017/04/12 19:00:26, achuithb wrote:
> Use a temp var for this?
> 
> CoreOobeView* core_oobe_view = GetOobeUI() ? GetOObeUI()->GetCoreOobeView() :
> nullptr;

Done.

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/login_shared.js (right):

https://codereview.chromium.org/2814863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/login_shared.js:415: * Shows/hides pin
keyboard on the lock screen.
On 2017/04/12 19:00:26, achuithb wrote:
> Also update comment

Done.

[xiyuan, 2017-04-13 05:03:07]

Skip adding/removing as ShellObserver for mash like this is fine. Thanks for
adding the bug ref. I will update the code when I get to it.

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:447:
view->ShowControlBar(true);
We should update LoginDisplayHostImpl [1] to consolidate the VK code.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/ui/login_d...

[jdufault, 2017-04-13 19:09:24]

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

[commit-bot: I haz the power, 2017-04-13 19:10:00]

Dry run: 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-13 20:20:25]

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

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

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-04-13 20:29:51]

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:447:
view->ShowControlBar(true);
On 2017/04/13 05:03:07, xiyuan wrote:
> We should update LoginDisplayHostImpl [1] to consolidate the VK code.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/ui/login_d...

Done.

[achuithb, 2017-04-13 21:49:41]

lgtm

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:184: if
(keyboard::KeyboardController::GetInstance())
Don't we need a HasObserver test here? In case AddObserver was not called

[achuithb, 2017-04-13 21:51:29]

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/webui_login_view.cc (right):

https://codereview.chromium.org/2814863002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/webui_login_view.cc:184: if
(keyboard::KeyboardController::GetInstance())
On 2017/04/13 21:49:41, achuithb wrote:
> Don't we need a HasObserver test here? In case AddObserver was not called

Actually feel free to ignore this

[jdufault, 2017-04-17 18:46:45]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2017-04-17 18:47:13]

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-17 18:55:22]

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

[commit-bot: I haz the power, 2017-04-17 18:55:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jdufault, 2017-04-17 20:45:56]

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

[jdufault, 2017-04-17 20:46:12]

bshe@ PTAL at ui/keyboard/keyboard_controller.h/cc

[bshe, 2017-04-19 13:46:28]

On 2017/04/17 20:46:12, jdufault wrote:
> bshe@ PTAL at ui/keyboard/keyboard_controller.h/cc

ui/keyboard/keyboard_controller lgtm

[jdufault, 2017-04-19 18:27:07]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2017-04-19 18:28:08]

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-19 19:29:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492626427061160,
"parent_rev": "6fa7b73ae0d094df830c607dd59fd6593d086176", "commit_rev":
"3bb91b6589418cfe73938b74369b520324840ecd"}

[commit-bot: I haz the power, 2017-04-19 19:29:46]

Description was changed from

==========
cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=623344
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
cros: Allow JS to control virtual keyboard on sign-in screen.

This also renames the JS<->C++ API; C++ does not need to know about pin.

BUG=623344
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2814863002
Cr-Commit-Position: refs/heads/master@{#465707}
Committed:
https://chromium.googlesource.com/chromium/src/+/3bb91b6589418cfe73938b74369b...
==========

[commit-bot: I haz the power, 2017-04-19 19:29:47]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3bb91b6589418cfe73938b74369b...