In TextInputManager, reset input type to none before switching active widgets.

In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171, 578168, 602723
Committed: https://crrev.com/44c242985f1c9f81dc56e7d5122776250b1f1a66
Cr-Commit-Position: refs/heads/master@{#408239}

[EhsanK, 2016-07-20 21:33:41]

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

[commit-bot: I haz the power, 2016-07-20 21:34:14]

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, 2016-07-20 22:57:53]

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

[commit-bot: I haz the power, 2016-07-20 22:57:54]

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

[EhsanK, 2016-07-21 03:23:19]

Description was changed from

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIF in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrvie from different RenderWidgets and the order or arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
in both cases, in the latter and in Aura platforms, the InputMethod is not
notified of jumping in between <input> elements. This is due to the
missing fraction of time where the input type is supposed to be none.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

to

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it still does
not mimic the exact same behavior as in the single RenderWidget case. In Aura
platforms, without setting the TextInputState to none before changing the active
widget, the IME behavior might go wrong and InputMethod behave out of sync with
the actual TextInputState. The reason is that the InputMethod would believe that
the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

[EhsanK, 2016-07-21 03:23:49]

Description was changed from

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it still does
not mimic the exact same behavior as in the single RenderWidget case. In Aura
platforms, without setting the TextInputState to none before changing the active
widget, the IME behavior might go wrong and InputMethod behave out of sync with
the actual TextInputState. The reason is that the InputMethod would believe that
the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

to

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

[EhsanK, 2016-07-21 19:57:47]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org

[EhsanK, 2016-07-21 19:57:48]

Ken, Charlie: PTAL.

[Charlie Reis, 2016-07-21 23:48:18]

Seems reasonable if Ken's happy with it.  A few nits below.

https://codereview.chromium.org/2171443003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2171443003/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.cc:87: // |active_view_| is
only updated when the state for |view| is not none.
Let's update this as well.  Maybe that it "only needs to be updated if it is
changing and the state for |view| is not NONE."

https://codereview.chromium.org/2171443003/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.cc:95:
UpdateTextInputState(active_view_, TextInputState());
This has a small risk of becoming an infinite recursion if we ever change the
default |type| on TextInputState.  It would be safer (and clearer) if you set
the state to TEXT_INPUT_TYPE_NONE explicitly.

[EhsanK, 2016-07-25 21:09:57]

Thanks for the reviews. PTAL.

https://codereview.chromium.org/2171443003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2171443003/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.cc:87: // |active_view_| is
only updated when the state for |view| is not none.
On 2016/07/21 23:48:18, Charlie Reis wrote:
> Let's update this as well.  Maybe that it "only needs to be updated if it is
> changing and the state for |view| is not NONE."

Acknowledged.

https://codereview.chromium.org/2171443003/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.cc:95:
UpdateTextInputState(active_view_, TextInputState());
On 2016/07/21 23:48:18, Charlie Reis wrote:
> This has a small risk of becoming an infinite recursion if we ever change the
> default |type| on TextInputState.  It would be safer (and clearer) if you set
> the state to TEXT_INPUT_TYPE_NONE explicitly.

The only reason I used recursion like this was to 1) reduce lines of code and 2)
let initialization of TextInputState happen organically through ctor.

I actually did not think of potential recursion if default type changes which is
an interesting point. But another reason for NOT having recursion could be the
discussion in this CL:
https://codereview.chromium.org/1894473002/

Where by calling TextInputStateChanged we end up sending an IPC to renderer
which is a waste of resources; or we could avoid that and endorse a crash on
input thread in android which is even worse.

I will try to implement this explicitly and avoid recursion for the reasons
above.

[kenrb, 2016-07-26 20:40:38]

It's not exactly ideal to add browser-side assumptions about Blink's IME
implementation, but given the offline discussion about raciness I think this is
the best approach for now.

A better approach overall, which I think is what you were saying earlier, is to
track the focused widget to decide which one is active, and never have to
discard latent updates from previously-active widgets.

lgtm

https://codereview.chromium.org/2171443003/diff/100001/content/browser/render...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2171443003/diff/100001/content/browser/render...
content/browser/renderer_host/text_input_manager.cc:104: // guarantee in the
order of IPCs from different RenderWidges and another
nit: Typo 'RenderWidges'

[EhsanK, 2016-07-26 21:27:56]

Thanks for the reviews!

I will file a bug for the race condition for TextInputState specifically so that
we can track the issue.

kenrb@: I agree with your argument. We could, instead of doing this, manually
reset the InputMethod for Aura by setting the focused client to none and then
the tab's view. But I was not sure about the correct solution on Android and
Mac.

I will revisit this after we find a solution for the race condition we currently
have with slow renderers.

FYI, I will commit after dry-run.

https://codereview.chromium.org/2171443003/diff/100001/content/browser/render...
File content/browser/renderer_host/text_input_manager.cc (right):

https://codereview.chromium.org/2171443003/diff/100001/content/browser/render...
content/browser/renderer_host/text_input_manager.cc:104: // guarantee in the
order of IPCs from different RenderWidges and another
On 2016/07/26 20:40:38, kenrb wrote:
> nit: Typo 'RenderWidges'

Done.

[EhsanK, 2016-07-26 23:13:25]

On 2016/07/26 21:27:56, EhsanK wrote:
> Thanks for the reviews!
> 
> I will file a bug for the race condition for TextInputState specifically so
that
> we can track the issue.
> 
> kenrb@: I agree with your argument. We could, instead of doing this, manually
> reset the InputMethod for Aura by setting the focused client to none and then
> the tab's view. But I was not sure about the correct solution on Android and
> Mac.
> 
> I will revisit this after we find a solution for the race condition we
currently
> have with slow renderers.
> 
> FYI, I will commit after dry-run.
> 
>
https://codereview.chromium.org/2171443003/diff/100001/content/browser/render...
> File content/browser/renderer_host/text_input_manager.cc (right):
> 
>
https://codereview.chromium.org/2171443003/diff/100001/content/browser/render...
> content/browser/renderer_host/text_input_manager.cc:104: // guarantee in the
> order of IPCs from different RenderWidges and another
> On 2016/07/26 20:40:38, kenrb wrote:
> > nit: Typo 'RenderWidges'
> 
> Done.
Actually, just noticed I am missing Charlie's LGTM.

[Charlie Reis, 2016-07-26 23:52:11]

LGTM.

[Charlie Reis, 2016-07-26 23:52:20]

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

[commit-bot: I haz the power, 2016-07-26 23:52:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-07-26 23:54:14]

Thank you Charlie for the review/LGTM.

[commit-bot: I haz the power, 2016-07-27 00:16:02]

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

[commit-bot: I haz the power, 2016-07-27 00:16:03]

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

[EhsanK, 2016-07-27 16:13:00]

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

[commit-bot: I haz the power, 2016-07-27 16:13:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-07-27 16:17:44]

PTAL. I think we should be fine now. Waiting for dry-run results.

https://codereview.chromium.org/2171443003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2171443003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2984:
updated_view->GetRenderWidgetHost())
The intention for adding this logic was to make sure we do not process the late
arriving IPC to reset state since we have already synthesized it. But
unfortunately, GetRenderWidgetHost() is pure virtual, and we may get here during
the destruction path for the active view as well. This was causing crashes.

Now the early return stays in TextInputManager::UpdateTextInputState. The only
thing we betray is that we never make an observer call when the stale IPC
arrives (against the fact that we have named the observer method
OnUpdateTextInputStateCalled). But I believe it is correct, and fine, since the
stale IPC has already been synthesized/emulated/simulated (not sure which word
fits here).

[commit-bot: I haz the power, 2016-07-27 16:58:14]

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

[commit-bot: I haz the power, 2016-07-27 16:58:15]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-07-27 18:39:55]

Yeah, this is a bit awkward but I don't see a better way to handle it.  It's
also arguably better to have both cases next to each other in
UpdateTextInputState (as in the latest patch set), so I'm ok with it.

LGTM.

[EhsanK, 2016-07-27 18:43:05]

On 2016/07/27 18:39:55, Charlie Reis wrote:
> Yeah, this is a bit awkward but I don't see a better way to handle it.  It's
> also arguably better to have both cases next to each other in
> UpdateTextInputState (as in the latest patch set), so I'm ok with it.
> 
> LGTM.

Thanks Charlie! I will revisit this whole logic after finishing IME and see if
I can totally base it on GetFocusedFrame() logic. But I doubt this part of it
might change much (we will still have to synthesize a state reset perhaps).

FYI, I will CQ after dry-run.

[EhsanK, 2016-07-27 18:43:13]

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

[EhsanK, 2016-07-27 18:43:20]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-27 18:43:20]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-07-27 18:43:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2171443003/#ps140001
(title: "Fix crashes due to pure virtual calls")

[commit-bot: I haz the power, 2016-07-27 18:43:27]

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, 2016-07-27 18:48:40]

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

[commit-bot: I haz the power, 2016-07-27 18:48:41]

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...)

[EhsanK, 2016-07-27 20:27:12]

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

[commit-bot: I haz the power, 2016-07-27 20:27:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-07-27 20:53:02]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-07-27 20:53:02]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-07-27 20:53:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2171443003/#ps160001
(title: "Rebased")

[commit-bot: I haz the power, 2016-07-27 20:53:12]

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, 2016-07-27 21:17:46]

Description was changed from

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

to

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

[commit-bot: I haz the power, 2016-07-27 21:17:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-27 21:19:09]

Description was changed from

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723
==========

to

==========
In TextInputManager, reset input type to none before switching active widgets.

When there are no OOPIFs in a page, if we switch focus from one <input>
into another (i.e., through a mouse click), the TextInputState will be
first set to NONE and then back to TEXT.

This however does not always happen when there are OOPIFs since the two IPCs
might
arrive from different RenderWidgets and the order of arrival is not
deterministic (e.g., the IPC to set to TEXT could arrive sooner).

Although TextInputManager tracks the state and active widget correctly
(regardless of what order the IPCs arrive from the RenderWidgets), it
still does not mimic the exact same behavior as in the single
RenderWidget case. In Aura platforms, without setting the
TextInputState to none before changing the active widget, the IME
behavior might go wrong and InputMethod behave out of sync with the
actual TextInputState. The reason is that the InputMethod would believe
that the TextInputClient is the same but the IME session has ended.

This CL will try to simulate the same behavior for all scenarios by
injecting a TextInputState of none in between an active widget change.

BUG=626171,578168,602723

Committed: https://crrev.com/44c242985f1c9f81dc56e7d5122776250b1f1a66
Cr-Commit-Position: refs/heads/master@{#408239}
==========

[commit-bot: I haz the power, 2016-07-27 21:19:10]

Patchset 9 (id:??) landed as
https://crrev.com/44c242985f1c9f81dc56e7d5122776250b1f1a66
Cr-Commit-Position: refs/heads/master@{#408239}