Limit display of the virtual keyboard to state changes triggered from a user gesture.

Limit display of the virtual keyboard to state changes triggered from a user gesture.

This patch reuses OnTextInputStateChanged, which was previously Android specific, but which includes not only type information, but whether the virtual keyboard should be displayed. This is the first step in consolidating the IPC messages between RenderWidget and RenderWidgetHostView for IME messages.  Ideally, we can phase out OnTextInputTypeChanged in favor of the approach used by Android across all platforms.



BUG=289659, 294191, 331690
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245932

[kevers, 2013-10-18 21:37:15]

Hi Sadrul,

Can you please take a look at this CL to see if it is a suitable approach.

[sadrul, 2013-10-21 02:34:56]

On 2013/10/18 21:37:15, kevers wrote:
> Hi Sadrul,
> 
> Can you please take a look at this CL to see if it is a suitable approach.

Yep. This looks pretty reasonable to me. It would be great to have someone who
is familiar with the android IME code to also take a look.

[kevers, 2013-10-21 14:43:16]

+nyquist@ for Android IME
+nona@ for ChromeOS IME

Can you please take a look to see if this strategy looks reasonable. Modelling
the show/hide behavior of the virtual keyboard in ChromeOS after Android.  The
patch still needs tweaks before being ready for commit to fix IME and VK unit
tests, and handling of native text fields.  Would appreciate some early
feedback.

[Seigo Nonaka, 2013-10-21 16:35:03]

Your approach looks reasonable to me :)
Let me leave one minor comment for IPC but it can't block your real
implementation.

https://codereview.chromium.org/29943002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/29943002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1069: // Aura uses
TextInputStateChanged instead of TextInputTypeChanged.
This means we have duplicated IPCs for text input type.
Need extra work but I prefer to unify these IPC to one even disabled by Macro.
And I also prefer to dispatch this IPC in RenderWidgetHostViewImpl rather than
Aura specific class.
This IPC is also used on Windows, Linux and Mac OSX.

However, as you know, Windows/Linux are going to Aura, so seems leaving comment
and revisiting later seems fine :)

[kevers, 2013-10-22 19:01:41]

https://codereview.chromium.org/29943002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/29943002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:1069: // Aura uses
TextInputStateChanged instead of TextInputTypeChanged.
On 2013/10/21 16:35:03, Seigo Nonaka wrote:
> This means we have duplicated IPCs for text input type.
> Need extra work but I prefer to unify these IPC to one even disabled by Macro.
> And I also prefer to dispatch this IPC in RenderWidgetHostViewImpl rather than
> Aura specific class.
> This IPC is also used on Windows, Linux and Mac OSX.
> 
> However, as you know, Windows/Linux are going to Aura, so seems leaving
comment
> and revisiting later seems fine :)

Added TODOs to complete the task of consolidating TextInput<State|Type>Changed.

[nyquist, 2013-10-22 19:42:52]

Adding aurimas@

[kevers, 2014-01-08 15:59:08]

Sadrul and Nonaka-san,

Can you please have a look at this CL.

[Shu Chen, 2014-01-09 04:21:16]

https://codereview.chromium.org/29943002/diff/570002/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/29943002/diff/570002/content/renderer/render_...
content/renderer/render_widget.cc:2543: #endif
This change is unnecessary if pass send_ime_ack correctly. Pls see my comment at
line 2724.

https://codereview.chromium.org/29943002/diff/570002/content/renderer/render_...
content/renderer/render_widget.cc:2724: UpdateTextInputState(true, true);
The 2nd parameter should be false if defined(USE_AURA), right?

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

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:268:
container_->layout_manager()->OnWindowResized();
unnecessary call to OnWindowResized(), because now its OnWindowAddedToLayout()
will call ResizeKeyboardToDefault().

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:287:
OnKeyboardBoundsChanging(container_->children()[0]->bounds()));
Why not use this?
NotifyKeyboardBoundsChanging(container_->children()[0]->bounds());

[Seigo Nonaka, 2014-01-09 06:08:42]

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/input_method...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/input_method...
ui/base/ime/input_method_base.cc:86: OnShowImeIfNeeded());
nit: can be one line?

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/mock_input_m...
File ui/base/ime/mock_input_method.cc (right):

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/mock_input_m...
ui/base/ime/mock_input_method.cc:107: OnShowImeIfNeeded());
ditto

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller_unittest.cc:202:
ui::SetUpInputMethodFactoryForTesting();
I think this function can be called multiple times as far as calling at lease
one time before any ui::CreateInputMethod function.

[kevers, 2014-01-09 15:29:08]

https://codereview.chromium.org/29943002/diff/570002/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/29943002/diff/570002/content/renderer/render_...
content/renderer/render_widget.cc:2543: #endif
On 2014/01/09 04:21:17, Shu Chen wrote:
> This change is unnecessary if pass send_ime_ack correctly. Pls see my comment
at
> line 2724.

IncrementOutstandingImeEventAcks is Android only. Removing this check requires
exposing other Android specific areas of code cross platform.  With a followup
CL, I would like to phase out of UpdateTextInputTypeChange in favor of
UpdateTextInputStateChange. At that point, we'll simply want to ignore the ack
parameter on non-Android builds. This check keeps the logic localized.

https://codereview.chromium.org/29943002/diff/570002/content/renderer/render_...
content/renderer/render_widget.cc:2724: UpdateTextInputState(true, true);
On 2014/01/09 04:21:17, Shu Chen wrote:
> The 2nd parameter should be false if defined(USE_AURA), right?

Correct. See my comment above.

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/input_method...
File ui/base/ime/input_method_base.cc (right):

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/input_method...
ui/base/ime/input_method_base.cc:86: OnShowImeIfNeeded());
On 2014/01/09 06:08:43, Seigo Nonaka wrote:
> nit: can be one line?

Done.

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/mock_input_m...
File ui/base/ime/mock_input_method.cc (right):

https://codereview.chromium.org/29943002/diff/570002/ui/base/ime/mock_input_m...
ui/base/ime/mock_input_method.cc:107: OnShowImeIfNeeded());
On 2014/01/09 06:08:43, Seigo Nonaka wrote:
> ditto

Done.

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

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:268:
container_->layout_manager()->OnWindowResized();
On 2014/01/09 04:21:17, Shu Chen wrote:
> unnecessary call to OnWindowResized(), because now its OnWindowAddedToLayout()
> will call ResizeKeyboardToDefault().

Done.

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:287:
OnKeyboardBoundsChanging(container_->children()[0]->bounds()));
On 2014/01/09 04:21:17, Shu Chen wrote:
> Why not use this?
> NotifyKeyboardBoundsChanging(container_->children()[0]->bounds());

Done.

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/29943002/diff/570002/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller_unittest.cc:202:
ui::SetUpInputMethodFactoryForTesting();
On 2014/01/09 06:08:43, Seigo Nonaka wrote:
> I think this function can be called multiple times as far as calling at lease
> one time before any ui::CreateInputMethod function.

This change is to address 331690.

Actually would have been easier to modify setUpInputMehtodFactoryForTesting to
test if the flag g_input_method_set_for_testing is already set before the Check
call.  I've made the necessary change.

[Shu Chen, 2014-01-10 01:22:07]

lgtm

[sadrul, 2014-01-10 19:53:21]

Sorry about the review delay.

I think this looks largely reasonable. A few comments/questions:

https://codereview.chromium.org/29943002/diff/880001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/29943002/diff/880001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:113: virtual void
ShowImeIfNeeded() const OVERRIDE;
Can this just be a private method in OmniboxViewViews, and not in the
OmniboxView interface? Also, I don't think this should be a const method.

https://codereview.chromium.org/29943002/diff/880001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/29943002/diff/880001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:502: // messages
for TextInput<State|Type>Changed.
Please also mention in the comment that the corresponding code in
RenderWidgetHostViewAndroid should also be moved at the same time.

https://codereview.chromium.org/29943002/diff/880001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/29943002/diff/880001/content/renderer/render_...
content/renderer/render_widget.cc:2543: #endif
Do you need this block of change? Looks like UpdateTextInputState() is called
with |send_ime_ack| == false for aura.

An alternate to this would be to always set |send_ime_ack| to false towards the
beginning of this function.

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

https://codereview.chromium.org/29943002/diff/880001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:231: if (keyboard_visible_ /* && 
!IsKeyboardUsabilityExperimentEnabled() */) {
Remove the /* comment

https://codereview.chromium.org/29943002/diff/880001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:247: proxy_->SetUpdateInputType(type);
Don't you need to show the keyboard window here?

https://codereview.chromium.org/29943002/diff/880001/ui/views/controls/textfi...
File ui/views/controls/textfield/native_textfield_views.h (right):

https://codereview.chromium.org/29943002/diff/880001/ui/views/controls/textfi...
ui/views/controls/textfield/native_textfield_views.h:150: void ShowImeIfNeeded()
const;
I don't think this should be a const method

https://codereview.chromium.org/29943002/diff/880001/ui/views/ime/input_method.h
File ui/views/ime/input_method.h (right):

https://codereview.chromium.org/29943002/diff/880001/ui/views/ime/input_metho...
ui/views/ime/input_method.h:118: virtual void ShowImeIfNeeded() const = 0;
non-const here too?

[kevers, 2014-01-13 16:59:09]

https://codereview.chromium.org/29943002/diff/880001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/29943002/diff/880001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:113: virtual void
ShowImeIfNeeded() const OVERRIDE;
On 2014/01/10 19:53:21, sadrul wrote:
> Can this just be a private method in OmniboxViewViews, and not in the
> OmniboxView interface? Also, I don't think this should be a const method.

Removed const.  Requires being part of the OmniboxView interface since it is
called from SearchTabHelper::FocusOmnibox.

https://codereview.chromium.org/29943002/diff/880001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/29943002/diff/880001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:502: // messages
for TextInput<State|Type>Changed.
On 2014/01/10 19:53:21, sadrul wrote:
> Please also mention in the comment that the corresponding code in
> RenderWidgetHostViewAndroid should also be moved at the same time.

Done.

https://codereview.chromium.org/29943002/diff/880001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/29943002/diff/880001/content/renderer/render_...
content/renderer/render_widget.cc:2543: #endif
On 2014/01/10 19:53:21, sadrul wrote:
> Do you need this block of change? Looks like UpdateTextInputState() is called
> with |send_ime_ack| == false for aura.
> 
> An alternate to this would be to always set |send_ime_ack| to false towards
the
> beginning of this function.

Conditional compilation required for IncrementOutstandingImeEventAcks since it
is only defined for Android.  Moved the Aura logic as suggested.

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

https://codereview.chromium.org/29943002/diff/880001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:231: if (keyboard_visible_ /* && 
!IsKeyboardUsabilityExperimentEnabled() */) {
On 2014/01/10 19:53:21, sadrul wrote:
> Remove the /* comment

Done.

https://codereview.chromium.org/29943002/diff/880001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:247: proxy_->SetUpdateInputType(type);
On 2014/01/10 19:53:21, sadrul wrote:
> Don't you need to show the keyboard window here?

Added a comment to clarify. We only wish to show the VK in response to a user
gesture, requiring an explicit call to OnShowImeIfNeeded.  We prevent hiding the
keyboard here if focus-cycling while the keyboard is visible (e.g. hitting the
tab key to move to the next input field).

https://codereview.chromium.org/29943002/diff/880001/ui/views/controls/textfi...
File ui/views/controls/textfield/native_textfield_views.h (right):

https://codereview.chromium.org/29943002/diff/880001/ui/views/controls/textfi...
ui/views/controls/textfield/native_textfield_views.h:150: void ShowImeIfNeeded()
const;
On 2014/01/10 19:53:21, sadrul wrote:
> I don't think this should be a const method

Removed const.

https://codereview.chromium.org/29943002/diff/880001/ui/views/ime/input_method.h
File ui/views/ime/input_method.h (right):

https://codereview.chromium.org/29943002/diff/880001/ui/views/ime/input_metho...
ui/views/ime/input_method.h:118: virtual void ShowImeIfNeeded() const = 0;
On 2014/01/10 19:53:21, sadrul wrote:
> non-const here too?

Done.

[sadrul, 2014-01-13 20:37:46]

ui/keyboard/ ui/views/ lgtm

https://codereview.chromium.org/29943002/diff/980001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/29943002/diff/980001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller_unittest.cc:345: 
remove

[Seigo Nonaka, 2014-01-14 01:38:30]

lgtm

lgtm, I'm sorry for my late response.

[kevers, 2014-01-14 16:55:01]

+ sky for chrome/browser/ui
+ jamesr for content/renderer

https://codereview.chromium.org/29943002/diff/980001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller_unittest.cc (right):

https://codereview.chromium.org/29943002/diff/980001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller_unittest.cc:345: 
On 2014/01/13 20:37:47, sadrul wrote:
> remove

Done.

[sky, 2014-01-14 19:47:26]

c/b/u LGTM

[jamesr, 2014-01-14 21:12:52]

content/renderer/ lgtm - might be good to let aurimas take a look as well

https://codereview.chromium.org/29943002/diff/1040001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/29943002/diff/1040001/content/renderer/render...
content/renderer/render_widget.cc:2530: p.require_ack = false;
nit: can you move this down to be next to the place that the OS(ANDROID) block
sets require_ack ?

https://codereview.chromium.org/29943002/diff/1040001/content/renderer/render...
content/renderer/render_widget.cc:2543: IncrementOutstandingImeEventAcks();
why do you not need this logic for USE_AURA ?

[kevers, 2014-01-14 22:16:44]

https://codereview.chromium.org/29943002/diff/1040001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/29943002/diff/1040001/content/renderer/render...
content/renderer/render_widget.cc:2530: p.require_ack = false;
On 2014/01/14 21:12:53, jamesr wrote:
> nit: can you move this down to be next to the place that the OS(ANDROID) block
> sets require_ack ?

This block was originally implemented as an #else below, but moved upstream at
Sadrul's request.

https://codereview.chromium.org/29943002/diff/1040001/content/renderer/render...
content/renderer/render_widget.cc:2543: IncrementOutstandingImeEventAcks();
On 2014/01/14 21:12:53, jamesr wrote:
> why do you not need this logic for USE_AURA ?

From the render widget host view side, IME is handled very differently for
Android and Aura. Whereas Android requires synchronization for its IME handling
in some circumstances, no synchronization is required for Aura.

With this patch, the only call into this method from Aura has its second
argument explicitly set to false. The plan is to consolidate on using
TextInputStateChange instead of TextInputTypeChanged on all platforms, and the
ack parameter is to be ignored on Aura.

[aurimas (slooooooooow), 2014-01-14 22:56:22]

lgtm

[commit-bot: I haz the power, 2014-01-15 16:29:09]

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

[commit-bot: I haz the power, 2014-01-15 16:29:22]

Failed to apply patch for chrome/browser/ui/views/omnibox/omnibox_view_views.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/omnibox/omnibox_view_views.h
  Hunk #1 FAILED at 110.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/omnibox/omnibox_view_views.h.rej

Patch:       chrome/browser/ui/views/omnibox/omnibox_view_views.h
Index: chrome/browser/ui/views/omnibox/omnibox_view_views.h
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.h
b/chrome/browser/ui/views/omnibox/omnibox_view_views.h
index
70ee35ca7338f7c43095b84fa93792d15b572213..10cd7d2c1c6e79c178dd23576e8ec809d149f91b
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.h
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.h
@@ -110,6 +110,7 @@ class OmniboxViewViews
   virtual int GetWidth() const OVERRIDE;
   virtual bool IsImeComposing() const OVERRIDE;
   virtual bool IsImeShowingPopup() const OVERRIDE;
+  virtual void ShowImeIfNeeded() OVERRIDE;
 
   // views::TextfieldController:
   virtual void ContentsChanged(views::Textfield* sender,

[commit-bot: I haz the power, 2014-01-15 18:23:53]

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

[commit-bot: I haz the power, 2014-01-16 00:06:03]

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, 2014-01-20 16:15:26]

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

[commit-bot: I haz the power, 2014-01-20 18:18:06]

Change committed as 245932