Fix Views Combobox and Tree View text input.

Fix Views Combobox and Tree View text input.

Make InputMethodBridge not a TextInputClient, use actual clients.
InputMethodBridge was simply forwarding to actual client views.
(e.g. NativeTextfieldViews and PrefixSelector for TreeView/Combobox)

Remove Views::InputMethod::On[Focus|Blur]() and their calls.
Simply update the host's TextInputClient on View focus changes. 
Fixes r216992's regression (input not sent to Combobox/TreeView).
The IME mode indicator also now shows in dialogs more often :)

Ignore Widget activation in InputMethodBase::GetTextInputClient.
(fixes linux_chromeos initial omnibox input with this patch, seems ok)

Call View::On[Focus|Blur] from Combobox and TreeView.
(mostly for correctness and NotifyAccessibilityEvent, not critical)
Remove the OnCaretBoundsChanged call from TreeView.
(unnecessary and doesn't set correct candidate window bounds)
Remove unused DesktopRootWindowHost::OnNativeWidget[Focus|Blur].
Handle NULL TextInputClient* objects in TextInputTestHelper.

BUG=276720, 271754, 265337
TEST=Test Win (desktop, Win8 Metro, and Aura builds), CrOS, and Linux Aura's Omnibox, Find Bar, Bookmark Bubble, and Edit Bookmark Dialog's textfield text entry (and text input handling to select combobox and tree view elements) with EN, JA, and other IMEs, and related IME usage (Win+Space to change IME on Win8, Alt+` to change modes, etc.). Also test with --enable-text-services-framework. Note that IME candidate windows may not appear or may appear at incorrect positions for tree views and comboboxes, but this seems like a separate issue.
R=sky@chromium.org

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

[msw, 2013-08-21 17:28:04]

Hey Scott, PTAL, this should be considered for M-30 merge.

I locally added a unit test, but it requires MockInputMethod to override
GetTextInputClient() (to ignore the active widget check used by InputMethodBase)
and it wouldn't have even caught this regression (passes without the fix). Also,
I can't figure out how to easily add an interactive_ui_test for this, but I'm
willing to learn and/or do it in a followup. Thoughts?

[sky, 2013-08-21 21:57:10]

https://codereview.chromium.org/23245012/diff/3001/ui/views/controls/tree/tre...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/23245012/diff/3001/ui/views/controls/tree/tre...
ui/views/controls/tree/tree_view.cc:604: GetInputMethod()->OnFocus();
Having to put this in each override is error prone. Is it possible to put this
in View::Focus and View::Blur?

[msw, 2013-08-21 22:57:19]

Please take another look; thanks!

https://codereview.chromium.org/23245012/diff/3001/ui/views/controls/tree/tre...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/23245012/diff/3001/ui/views/controls/tree/tre...
ui/views/controls/tree/tree_view.cc:604: GetInputMethod()->OnFocus();
On 2013/08/21 21:57:11, sky wrote:
> Having to put this in each override is error prone. Is it possible to put this
> in View::Focus and View::Blur?

Done.

[sky, 2013-08-21 23:35:29]

https://codereview.chromium.org/23245012/diff/13001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view.cc (left):

https://codereview.chromium.org/23245012/diff/13001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view.cc:606:
GetInputMethod()->OnCaretBoundsChanged(this);
I'm pretty sure I found that if I didn't invoke this inputmethod didn't work.
Could you verify if it's necessary?

[msw, 2013-08-22 18:07:10]

https://codereview.chromium.org/23245012/diff/13001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view.cc (left):

https://codereview.chromium.org/23245012/diff/13001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view.cc:606:
GetInputMethod()->OnCaretBoundsChanged(this);
On 2013/08/21 23:35:29, sky wrote:
> I'm pretty sure I found that if I didn't invoke this inputmethod didn't work.
> Could you verify if it's necessary?

I tested English and Japanese in Win7, Win8 Desktop, Win8 Metro, and the input
method works, but I found a tangential issue. With or without this call, Win7
misplaces the IME candidate window for the Tree View and Combobox. It seems that
this was broken before the general input regression (r216992). I recommend
landing and merging this fix to M-30 (with this unnecessary OnCaretBoundsChanged
call removed) and handling the candidate window placement issue separately.

[msw, 2013-08-22 18:23:20]

Actually, the documentation for InputMethod::On[Focus|Blur] says:
"// Called when the top-level system window gets keyboard focus.", etc.
I think they should probably be called on Widget or NativeWidget [de]activation.
That seems to work locally, I'll test/refine and have a new patch set up soon.
You advice is welcome, hopefully someone CC'ed has advice too.

[Yohei Yukawa, 2013-08-23 03:06:52]

On 2013/08/22 18:23:20, msw wrote:
> Actually, the documentation for InputMethod::On[Focus|Blur] says:
> "// Called when the top-level system window gets keyboard focus.", etc.
> I think they should probably be called on Widget or NativeWidget
[de]activation.
> That seems to work locally, I'll test/refine and have a new patch set up soon.
> You advice is welcome, hopefully someone CC'ed has advice too.

Thank you for your tough work. Your approach seems to be right thing to me.

Kochi-san, could you double check this CL?
It would be nice if someone in Chrome IME team in Tokyo is kept updated about
focus management for IME.

[msw, 2013-08-25 00:53:54]

I've made significant additional changes that seem much more correct.
Scott and Nonaka (or another IME expert), please take a look; thanks!
Please tests IMEs on CrOS if you can, I haven't figured that out yet.

[Seigo Nonaka, 2013-08-26 15:01:03]

lgtm

lgtm!

Thank you for your cleaning up!

We are probably also able to remove OnFocus/OnBlur from ui::InputMethod, but we
can split from this patch.

Thank you!

[sky, 2013-08-26 16:01:34]

Nice! LGTM

https://codereview.chromium.org/23245012/diff/72001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/23245012/diff/72001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:893: if (destroying_) {
nit: no {}

[msw, 2013-08-26 16:28:33]

Landing; thanks!

https://codereview.chromium.org/23245012/diff/72001/ui/views/widget/native_wi...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/23245012/diff/72001/ui/views/widget/native_wi...
ui/views/widget/native_widget_aura.cc:893: if (destroying_) {
On 2013/08/26 16:01:35, sky wrote:
> nit: no {}

Done.

[commit-bot: I haz the power, 2013-08-26 16:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/23245012/78001

[commit-bot: I haz the power, 2013-08-27 03:19:35]

Change committed as 219673

[kochi, 2013-08-27 08:49:40]

On 2013/08/27 03:19:35, I haz the power (commit-bot) wrote:
> Change committed as 219673

Thanks for the great cleanup!