Allow tap gestures to open the IME on Android.

Allow tap gestures to open the IME on Android.

This CL allows tap gestures to cause the IME to be shown on Android

It uses the new didHandleGestureEvent of WebViewCient that was added in http://trac.webkit.org/changeset/129029
When the IME is shown, we need to receive a text input state update to ensure the IME has
the correct state. For this reason, the message to show the IME has been added as part of
ViewHostMsg_TextInputStateChanged. To this end, a new parameter has been added to UpdateTextInputState
to specify whether the updtae should cause the IME to be shown or not.

BUG=147226
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158042

[James Su, 2012-09-20 15:10:12]

LGTM, except one nit.

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1660: if (show_ime_if_needed ||
(text_input_type_ != new_type
I'm wondering if it's better to change it to:
if ((show_ime_if_needed && !input_method_is_active_) || ...)

As we don't need to send the message if nothing else changed and the ime is
already shown.

[olilan, 2012-09-20 15:20:46]

Thanks for the review. Reply added.

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1660: if (show_ime_if_needed ||
(text_input_type_ != new_type
On Android it's possible for input_method_is_active_ to be true but the ime not
be visible, for example if an input field is focused but the user presses Back
to dismiss the keyboard. In this case we'd still need the user to be able to
show the ime by tapping the input field, so for us I don't think we can change
it as you suggest.

On 2012/09/20 15:10:12, James Su wrote:
> I'm wondering if it's better to change it to:
> if ((show_ime_if_needed && !input_method_is_active_) || ...)
> 
> As we don't need to send the message if nothing else changed and the ime is
> already shown.

[James Su, 2012-09-20 15:42:42]

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1660: if (show_ime_if_needed ||
(text_input_type_ != new_type
On 2012/09/20 15:20:46, olilan wrote:
> On Android it's possible for input_method_is_active_ to be true but the ime
not
> be visible, for example if an input field is focused but the user presses Back
> to dismiss the keyboard. In this case we'd still need the user to be able to
> show the ime by tapping the input field, so for us I don't think we can change
> it as you suggest.
> 
> On 2012/09/20 15:10:12, James Su wrote:
> > I'm wondering if it's better to change it to:
> > if ((show_ime_if_needed && !input_method_is_active_) || ...)
> > 
> > As we don't need to send the message if nothing else changed and the ime is
> > already shown.
> 

Make sense. Thanks for clarification.

[olilan, 2012-09-20 16:04:49]

jam, could you take a look as a content owner please?

[jam, 2012-09-20 18:01:22]

lgtm

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2075: bool eventSwallowed) {
nit: event_swallowed per style guide (also in header)

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2078: || event.type ==
WebInputEvent::GestureLongPress) {
nit: || should be on previous line per style guide

[olilan, 2012-09-21 11:23:55]

Thanks for the review. Nits fixed in the next patch.

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2075: bool eventSwallowed) {
On 2012/09/20 18:01:22, John Abd-El-Malek wrote:
> nit: event_swallowed per style guide (also in header)

Done.

https://codereview.chromium.org/10962011/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:2078: || event.type ==
WebInputEvent::GestureLongPress) {
On 2012/09/20 18:01:22, John Abd-El-Malek wrote:
> nit: || should be on previous line per style guide

Done.

[commit-bot: I haz the power, 2012-09-21 11:26:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10962011/9001

[commit-bot: I haz the power, 2012-09-21 11:55:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-21 15:01:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10962011/7003

[commit-bot: I haz the power, 2012-09-21 15:26:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-21 17:04:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/olilan@chromium.org/10962011/12007

[commit-bot: I haz the power, 2012-09-21 19:20:47]

Change committed as 158042