Move call path for resetInputMethod

Move call path for resetInputMethod

This is mostly about mechanical changes such as:

- Move InputMethodController-related logic inside InputMethodController
  and call it first - such that we can expand the logic there.
- Rename method name for better readability.

Also, there are subtle logical changes:

1) Try finishComposingText() even when the current input type is NONE.
   This should not have any effect.

2) When finishComposingText() returns false, we no longer call
   UpdateTextInputState(). This should not have any effect.

3) When finishComposingText() returns false, we no long call
   UpdateCompositionInfo(). This is used by Android and Mac.
   Previously, there was a chance that we propagate InvalidRange() all the
   way up to the keyboard app. Now ShouldUpdateCompositionInfo() returns
   false to make it clear.

This is originally found in resolving crbug.com/664317, but is also
needed for crbug.com/630137 because if we no longer cancel composition
on selection change, then WebViewTest may fail because it uses
TestWebViewClient which does not implement resetInputMethod().

BUG=664317, 630137
Committed: https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b
Cr-Commit-Position: refs/heads/master@{#437854}

[Changwan Ryu, 2016-12-07 08:08:38]

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

[commit-bot: I haz the power, 2016-12-07 08:08:52]

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

[Changwan Ryu, 2016-12-07 08:09:25]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org, yosin@chromium.org

[Changwan Ryu, 2016-12-07 08:09:26]

PTAL

[yosin_UTC9, 2016-12-07 09:38:19]

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1196: void
InputMethodController::willChangeFocusTo(Element* newElement) {
Q: Do we need to take |newElement|? It can be in another frame.

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1197: if
(finishComposingText(DoNotKeepSelection))
nit: Could you make early return style?

if (!finishComposingText(DoNotKeepSelection))
  return;
frame().chromeClient().resetInputMethod();

Q: Should we reset IME always? 
finishComposingText() returns false only if JS event handlers changes selection
or document.
What will be happened when we don't reset IME?
It is OK to leave IME thinks composition is still on going?

# Self note for understanding the change:
This code means if we insert composition, we reset IME.
Otherwise, do nothing.

finishComposingText() returns false if
 - no composition
 - JS change selection, by selectionstart or foucs event handler or other events
during |selectComposition()|
 - document is gone. same reason as above.

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1133:
oldFocusedFrame->inputMethodController().willChangeFocusTo(element);
yosin@'s self note:

Introducing |willChangeFocusTo()| inspires to me introducing
|didChangeFocusTo(element)| for replacement of 


Document::setFocuusElement()
    if (isRootEditableElement(*m_focusedElement))
      frame()->spellChecker().didBeginEditing(m_focusedElement.get());


void HTMLInputElement::beginEditing() {
  DCHECK(document().isActive());
  if (!document().isActive())
    return;

  if (!isTextField())
    return;

  document().frame()->spellChecker().didBeginEditing(this);
}

But, once we have idle time spell checker, we don't need to have
|SpellChecker::didBeginEditing()|.

[yosin_UTC9, 2016-12-07 09:46:00]

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

https://codereview.chromium.org/2561543003/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:2140: if (text_input_type_ !=
ui::TEXT_INPUT_TYPE_NONE)
Q: What is happened when we call ImeCancelComposition with INPUT_TYPE_NONE?
Q: Why does Blink call this function with INPUT_TYPE_NONE?

[yosin_UTC9, 2016-12-07 10:01:19]

BTW, to live with state mismatch between IME in browser process and Blink in
renderer process, I would like to introduce session id. When Blink change focus
to element, Blink start new IME session and tells IME session id, then IME send
request and event with session id. If IME's session id is different from
rendere's IME session id, we can ignore them.

WDYT?

[commit-bot: I haz the power, 2016-12-07 10:13:55]

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

[commit-bot: I haz the power, 2016-12-07 10:13:57]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2016-12-07 10:17:58]

lgtm about code changes since it is just re-structuring.
My questions are regarding to improving robustness since we would like manage
situation to remove guard codes which we can forget when change code.

[Changwan Ryu, 2016-12-08 01:16:15]

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

[Changwan Ryu, 2016-12-08 01:16:30]

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

https://codereview.chromium.org/2561543003/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:2140: if (text_input_type_ !=
ui::TEXT_INPUT_TYPE_NONE)
On 2016/12/07 09:46:00, Yosi_UTC9 wrote:
> Q: What is happened when we call ImeCancelComposition with INPUT_TYPE_NONE?

The browser process may ignore it. If it does not ignore it, then it's just an
extra reset that is not needed.

> Q: Why does Blink call this function with INPUT_TYPE_NONE?

Hmm... Actually this should not happen because there was some composition, but I
encountered some layout test failures. Let me investigate it a bit more.

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1196: void
InputMethodController::willChangeFocusTo(Element* newElement) {
On 2016/12/07 09:38:19, Yosi_UTC9 wrote:
> Q: Do we need to take |newElement|? It can be in another frame.

Removed this parameter for now.

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/InputMethodController.cpp:1197: if
(finishComposingText(DoNotKeepSelection))
On 2016/12/07 09:38:19, Yosi_UTC9 wrote:
> nit: Could you make early return style?
> 
> if (!finishComposingText(DoNotKeepSelection))
>   return;
> frame().chromeClient().resetInputMethod();
Done

> 
> Q: Should we reset IME always? 
> finishComposingText() returns false only if JS event handlers changes
selection
> or document.
> What will be happened when we don't reset IME?
> It is OK to leave IME thinks composition is still on going?
> 
> # Self note for understanding the change:
> This code means if we insert composition, we reset IME.
> Otherwise, do nothing.
> 
> finishComposingText() returns false if
>  - no composition
>  - JS change selection, by selectionstart or foucs event handler or other
events
> during |selectComposition()|
>  - document is gone. same reason as above.

Hmm... I think the real issue here is that we're 'selecting' composition in the
process of finishComposingText and emitting JS event in the first place. We
should try to fix that.

Now I think that when there is composition and DOM changes then we should let
browser process know about the composition change, (resetting input method for
now). But it shouldn't be concerning in this CL.

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2561543003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1133:
oldFocusedFrame->inputMethodController().willChangeFocusTo(element);
On 2016/12/07 09:38:19, Yosi_UTC9 wrote:
> yosin@'s self note:
> 
> Introducing |willChangeFocusTo()| inspires to me introducing
> |didChangeFocusTo(element)| for replacement of 
> 
> 
> Document::setFocuusElement()
>     if (isRootEditableElement(*m_focusedElement))
>       frame()->spellChecker().didBeginEditing(m_focusedElement.get());
> 
> 
> void HTMLInputElement::beginEditing() {
>   DCHECK(document().isActive());
>   if (!document().isActive())
>     return;
> 
>   if (!isTextField())
>     return;
> 
>   document().frame()->spellChecker().didBeginEditing(this);
> }
> 
> But, once we have idle time spell checker, we don't need to have
> |SpellChecker::didBeginEditing()|.

This sounds like a great idea. I was thinking of adding it myself for another
reason. If we have one, we can separate the logic further as

- willChangeFocusTo() finishes the current composition.
- didChangeFocusTo() updates text input state and then resets input method.

such that way input method can be restarted with the updated text input state.
Currently, it doesn't and it's causing some problem when you move focus to
another input element. (crbug.com/582576)

[commit-bot: I haz the power, 2016-12-08 01:17:05]

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-12-08 01:31:30]

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

[commit-bot: I haz the power, 2016-12-08 01:31:31]

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

[Changwan Ryu, 2016-12-08 01:34:57]

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

[commit-bot: I haz the power, 2016-12-08 01:35:34]

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-12-08 02:20:32]

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

[commit-bot: I haz the power, 2016-12-08 02:20:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yosin_UTC9, 2016-12-08 05:04:48]

On 2016/12/08 at 01:16:30, changwan wrote:
> - willChangeFocusTo() finishes the current composition.
> - didChangeFocusTo() updates text input state and then resets input method.
> 
> such that way input method can be restarted with the updated text input state.
Currently, it doesn't and it's causing some problem when you move focus to
another input element. (crbug.com/582576)

Good to go, since, "will" and "did" should be pair.

[essayspecialist11, 2016-12-08 05:09:30]

Reset input method creates logical changes inside the Input output method.The
input method can be restarted with the updated text input state.
for more visit-http://essaywritersworld.com/

[Changwan Ryu, 2016-12-08 09:55:26]

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

[commit-bot: I haz the power, 2016-12-08 09:55:43]

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

[Changwan Ryu, 2016-12-08 09:56:07]

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

https://codereview.chromium.org/2561543003/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:2140: if (text_input_type_ !=
ui::TEXT_INPUT_TYPE_NONE)
On 2016/12/08 01:16:29, Changwan Ryu wrote:
> On 2016/12/07 09:46:00, Yosi_UTC9 wrote:
> > Q: What is happened when we call ImeCancelComposition with INPUT_TYPE_NONE?
> 
> The browser process may ignore it. If it does not ignore it, then it's just an
> extra reset that is not needed.
> 
> > Q: Why does Blink call this function with INPUT_TYPE_NONE?
> 
> Hmm... Actually this should not happen because there was some composition, but
I
> encountered some layout test failures. Let me investigate it a bit more.

Removed this check.

[commit-bot: I haz the power, 2016-12-08 11:04:44]

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

[commit-bot: I haz the power, 2016-12-08 11:04:45]

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

[Changwan Ryu, 2016-12-09 00:48:33]

changwan@chromium.org changed reviewers:
+ bokan@chromium.org

[Changwan Ryu, 2016-12-09 00:48:33]

bokan@chromium.org: Please review changes in 

third_party/WebKit/Source/core/page

aelias@chromium.org: could you review the rest?

Thanks!

[aelias_OOO_until_Jul13, 2016-12-09 01:44:59]

lgtm

[Changwan Ryu, 2016-12-09 01:54:40]

On 2016/12/07 10:01:19, Yosi_UTC9 wrote:
> BTW, to live with state mismatch between IME in browser process and Blink in
> renderer process, I would like to introduce session id. When Blink change
focus
> to element, Blink start new IME session and tells IME session id, then IME
send
> request and event with session id. If IME's session id is different from
> rendere's IME session id, we can ignore them.
> 
> WDYT?

Sorry I missed this comment. I think session ID sounds like a great idea,
and it can definitely fix browser-to-renderer side of the sync issue.

[aelias_OOO_until_Jul13, 2016-12-09 02:00:04]

On 2016/12/09 at 01:54:40, changwan wrote:
> On 2016/12/07 10:01:19, Yosi_UTC9 wrote:
> > BTW, to live with state mismatch between IME in browser process and Blink in
> > renderer process, I would like to introduce session id. When Blink change
focus
> > to element, Blink start new IME session and tells IME session id, then IME
send
> > request and event with session id. If IME's session id is different from
> > rendere's IME session id, we can ignore them.
> > 
> > WDYT?
> 
> Sorry I missed this comment. I think session ID sounds like a great idea,
> and it can definitely fix browser-to-renderer side of the sync issue.

Hmm, I made an alternate proposal at
https://bugs.chromium.org/p/chromium/issues/detail?id=650204#c10 which does not
involve session ID.  More importantly, it doesn't involve Blink ever needing to
ignore events.  Anyway, it sounds like this discussion is out of scope of this
patch (and yosin@ is on vacation starting today for a month).

[bokan, 2016-12-09 17:05:43]

Source/core/page lgtm

[Changwan Ryu, 2016-12-09 23:48:21]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-12-09 23:48:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2561543003/#ps60001
(title: "removed NONE check")

[commit-bot: I haz the power, 2016-12-09 23:49:34]

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-12-09 23:54:16]

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

[commit-bot: I haz the power, 2016-12-09 23:54:18]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Changwan Ryu, 2016-12-12 08:42:33]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-12-12 08:42:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
aelias@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2561543003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-12 08:42:48]

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-12-12 10:20:48]

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

[commit-bot: I haz the power, 2016-12-12 10:20:50]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Changwan Ryu, 2016-12-12 12:07:41]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2016-12-12 12:07:55]

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-12-12 12:43:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481544461683080,
"parent_rev": "bc8c02d3996d81becf73fd75f6977e51892df8b7", "commit_rev":
"bb98e5fa32592f4aa82692370ff06f548750dde8"}

[commit-bot: I haz the power, 2016-12-12 12:43:40]

Description was changed from

==========
Move call path for resetInputMethod

This is mostly about mechanical changes such as:

- Move InputMethodController-related logic inside InputMethodController
  and call it first - such that we can expand the logic there.
- Rename method name for better readability.

Also, there are subtle logical changes:

1) Try finishComposingText() even when the current input type is NONE.
   This should not have any effect.

2) When finishComposingText() returns false, we no longer call
   UpdateTextInputState(). This should not have any effect.

3) When finishComposingText() returns false, we no long call
   UpdateCompositionInfo(). This is used by Android and Mac.
   Previously, there was a chance that we propagate InvalidRange() all the
   way up to the keyboard app. Now ShouldUpdateCompositionInfo() returns
   false to make it clear.

This is originally found in resolving crbug.com/664317, but is also
needed for crbug.com/630137 because if we no longer cancel composition
on selection change, then WebViewTest may fail because it uses
TestWebViewClient which does not implement resetInputMethod().

BUG=664317, 630137
==========

to

==========
Move call path for resetInputMethod

This is mostly about mechanical changes such as:

- Move InputMethodController-related logic inside InputMethodController
  and call it first - such that we can expand the logic there.
- Rename method name for better readability.

Also, there are subtle logical changes:

1) Try finishComposingText() even when the current input type is NONE.
   This should not have any effect.

2) When finishComposingText() returns false, we no longer call
   UpdateTextInputState(). This should not have any effect.

3) When finishComposingText() returns false, we no long call
   UpdateCompositionInfo(). This is used by Android and Mac.
   Previously, there was a chance that we propagate InvalidRange() all the
   way up to the keyboard app. Now ShouldUpdateCompositionInfo() returns
   false to make it clear.

This is originally found in resolving crbug.com/664317, but is also
needed for crbug.com/630137 because if we no longer cancel composition
on selection change, then WebViewTest may fail because it uses
TestWebViewClient which does not implement resetInputMethod().

BUG=664317, 630137

Review-Url: https://codereview.chromium.org/2561543003
==========

[commit-bot: I haz the power, 2016-12-12 12:43:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-12 15:11:42]

Description was changed from

==========
Move call path for resetInputMethod

This is mostly about mechanical changes such as:

- Move InputMethodController-related logic inside InputMethodController
  and call it first - such that we can expand the logic there.
- Rename method name for better readability.

Also, there are subtle logical changes:

1) Try finishComposingText() even when the current input type is NONE.
   This should not have any effect.

2) When finishComposingText() returns false, we no longer call
   UpdateTextInputState(). This should not have any effect.

3) When finishComposingText() returns false, we no long call
   UpdateCompositionInfo(). This is used by Android and Mac.
   Previously, there was a chance that we propagate InvalidRange() all the
   way up to the keyboard app. Now ShouldUpdateCompositionInfo() returns
   false to make it clear.

This is originally found in resolving crbug.com/664317, but is also
needed for crbug.com/630137 because if we no longer cancel composition
on selection change, then WebViewTest may fail because it uses
TestWebViewClient which does not implement resetInputMethod().

BUG=664317, 630137

Review-Url: https://codereview.chromium.org/2561543003
==========

to

==========
Move call path for resetInputMethod

This is mostly about mechanical changes such as:

- Move InputMethodController-related logic inside InputMethodController
  and call it first - such that we can expand the logic there.
- Rename method name for better readability.

Also, there are subtle logical changes:

1) Try finishComposingText() even when the current input type is NONE.
   This should not have any effect.

2) When finishComposingText() returns false, we no longer call
   UpdateTextInputState(). This should not have any effect.

3) When finishComposingText() returns false, we no long call
   UpdateCompositionInfo(). This is used by Android and Mac.
   Previously, there was a chance that we propagate InvalidRange() all the
   way up to the keyboard app. Now ShouldUpdateCompositionInfo() returns
   false to make it clear.

This is originally found in resolving crbug.com/664317, but is also
needed for crbug.com/630137 because if we no longer cancel composition
on selection change, then WebViewTest may fail because it uses
TestWebViewClient which does not implement resetInputMethod().

BUG=664317, 630137

Committed: https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b
Cr-Commit-Position: refs/heads/master@{#437854}
==========

[commit-bot: I haz the power, 2016-12-12 15:11:43]

Patchset 5 (id:??) landed as
https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b
Cr-Commit-Position: refs/heads/master@{#437854}