Make keyboard controller a singleton.

Make keyboard controller a singleton.

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

[kevers, 2014-03-31 17:54:39]

Hi Sadrul,

Can you please take a look at this CL.

[sadrul, 2014-03-31 18:44:10]

LGTM

https://codereview.chromium.org/219503003/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/219503003/diff/1/ash/shell.h#newcode51
ash/shell.h:51: class KeyboardController;
You can remove this now.

https://codereview.chromium.org/219503003/diff/1/ui/keyboard/keyboard_control...
File ui/keyboard/keyboard_controller.h (right):

https://codereview.chromium.org/219503003/diff/1/ui/keyboard/keyboard_control...
ui/keyboard/keyboard_controller.h:84: // Sets the active keyboard controller.
Mention that the caller does not own the controller anymore (i.e.
KeyboardController takes ownership of the controller), and calling
ResetInstance() again will destroy the controller.

[kevers, 2014-04-01 15:57:10]

+jamescook@ for ash OWNERS
+shuchen@ for input_method OWNERS
+kalman@ for extensions OWNERS

https://codereview.chromium.org/219503003/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/219503003/diff/1/ash/shell.h#newcode51
ash/shell.h:51: class KeyboardController;
On 2014/03/31 18:44:10, sadrul wrote:
> You can remove this now.

Done.

https://codereview.chromium.org/219503003/diff/1/ui/keyboard/keyboard_control...
File ui/keyboard/keyboard_controller.h (right):

https://codereview.chromium.org/219503003/diff/1/ui/keyboard/keyboard_control...
ui/keyboard/keyboard_controller.h:84: // Sets the active keyboard controller.
On 2014/03/31 18:44:10, sadrul wrote:
> Mention that the caller does not own the controller anymore (i.e.
> KeyboardController takes ownership of the controller), and calling
> ResetInstance() again will destroy the controller.

Done.

[not at google - send to devlin, 2014-04-01 16:05:11]

the changes to extensions are trivial and given the keyboard change seem fine.
however, I have a question about using base::Singleton.

and another: are you sure the singleton should be per-process and not
per-profile?

https://codereview.chromium.org/219503003/diff/20001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/219503003/diff/20001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:182: KeyboardController*
KeyboardController::instance_ = NULL;
typically you would use base::Singleton for singletons.

[Shu Chen, 2014-04-01 16:12:16]

lgtm

lgtm for files under chrome/browser/chromeos/input_method/...

[James Cook, 2014-04-01 16:49:26]

ash/ lgtm

[kevers, 2014-04-01 17:14:34]

On 2014/04/01 16:05:11, kalman wrote:
> the changes to extensions are trivial and given the keyboard change seem fine.
> however, I have a question about using base::Singleton.
> 
> and another: are you sure the singleton should be per-process and not
> per-profile?
> 
>
https://codereview.chromium.org/219503003/diff/20001/ui/keyboard/keyboard_con...
> File ui/keyboard/keyboard_controller.cc (right):
> 
>
https://codereview.chromium.org/219503003/diff/20001/ui/keyboard/keyboard_con...
> ui/keyboard/keyboard_controller.cc:182: KeyboardController*
> KeyboardController::instance_ = NULL;
> typically you would use base::Singleton for singletons.

I opted not to use base::Singleton in order to make the process of
reinitializing the instance clear. The single instance is managed by ash::Shell,
which reinitializes and tears down the controller as required.  For example,
switching profiles after loading causes a reinitialization.  Enabling or
disabling the on-screen keyboard from a11y settings similarly initializes or
tears down the controller. The reason for moving the instance out of ash::shell
is to allow non-ash code access to the instance.  In particular, a followup CL
will require access to the keyboard controller from RenderWidgetHostViewAura in
order to support virtual keyboard overscrolling in Blink.

[not at google - send to devlin, 2014-04-01 17:22:03]

ok lgtm

[kevers, 2014-04-01 17:30:51]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-04-01 17:31:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/219503003/20001

[commit-bot: I haz the power, 2014-04-01 18:25:40]

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

[commit-bot: I haz the power, 2014-04-01 18:25:40]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kevers, 2014-04-04 17:55:43]

+nkostylev@ for login OWNERS.

[Nikita (slow), 2014-04-07 05:15:50]

lgtm

[kevers, 2014-04-07 13:31:31]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-04-07 13:31:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/219503003/40001

[commit-bot: I haz the power, 2014-04-07 15:48:16]

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

[commit-bot: I haz the power, 2014-04-07 15:48:17]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kevers, 2014-04-07 16:53:23]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-04-07 16:53:39]

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

[commit-bot: I haz the power, 2014-04-07 19:26:05]

Change committed as 262177