[refactor] - Move textInputInfo() and textInputType() from WebWidget to WebInputMethodController

[refactor] - Move textInputInfo() and textInputType() from WebWidget to WebInputMethodController

In line with refactoring the duplicated IME code, the implementation of
WebWidget::textInputInfo() and WebWidget::textInputType() have been
moved to WebINputMethodController. The benefits of this refactor are:

 1- This is compliant by the fact that the actual logic lives in
 InputMethodController class. Therefore, it makes sense to have the
 calles use the public wrapper instead of WebWidget.

 2- This helps with the separation of WebView and WebWidget as the
 refactored methods are removed from both.

This CL follows up on the original refactoring CL which landed
WebInputMethodController (https://codereview.chromium.org/2333813002/).

Doc:
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1XQQ/edit?usp=sharing

BUG=629721
Committed: https://crrev.com/59cd849d12cffcc4f31281e758deb18111717796
Cr-Commit-Position: refs/heads/master@{#440046}

[EhsanK, 2016-11-18 23:01:42]

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

[commit-bot: I haz the power, 2016-11-18 23:03:07]

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

[EhsanK, 2016-11-18 23:19:56]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org, dglazkov@chromium.org,
dtapuska@chromium.org, lfg@chromium.org

[EhsanK, 2016-11-18 23:19:57]

Please, take a look.

dglazkov@ or dcheng@: as blink owners.
dtapuska@: for blink IME
lfg@: for Widget/View split
creis@: as content/ owner.

Thanks!

https://codereview.chromium.org/2508363003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputMethodControllerImpl.h (left):

https://codereview.chromium.org/2508363003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputMethodControllerImpl.h:40: // Called to
ask the WebInputMethodController to confirm an ongoing
Comments already exists in the abstract base class.

[dglazkov, 2016-11-19 16:16:54]

LGTM, this is great!

[dtapuska, 2016-11-23 20:35:40]

On 2016/11/19 16:16:54, dglazkov (OOO until Nov 28) wrote:
> LGTM, this is great!

lgtm

[Charlie Reis, 2016-11-23 23:35:09]

Thanks!  content/ LGTM with one question.  (No need to wait for me to land it
once you figure out the answer.)

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

https://codereview.chromium.org/2508363003/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:2176: if (controller &&
controller->textInputInfo().value.isEmpty())
Is this right?  This looks like it's hiding the IME if the TextInputInfo is
empty.  Maybe we should also be taking that branch if there's no controller? 
(Or is IF_NEEDED the right thing to do in that case?)

Just wondering if this should be:
  if (!controller || controller->textInputInfo...)

[EhsanK, 2016-11-24 02:27:45]

Thanks Charlie! I will try to land it. I made a slight change in response to
your comment.

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

https://codereview.chromium.org/2508363003/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:2176: if (controller &&
controller->textInputInfo().value.isEmpty())
On 2016/11/23 23:35:09, Charlie Reis (slow) wrote:
> Is this right?  This looks like it's hiding the IME if the TextInputInfo is
> empty.  
Yes. Apparently, this is because we should show a paste menu when long pressing
<input> and not IME. This is the CL that did this:
https://chromium.googlesource.com/chromium/src/+/07c70d207c4ed217286cdf842365...

> Maybe we should also be taking that branch if there's no controller? 
> (Or is IF_NEEDED the right thing to do in that case?)
> 
> Just wondering if this should be:
>   if (!controller || controller->textInputInfo...)
That is very good question. The way I see it, there is two scenarios that will
lead to |controller| being null.

1- GetWebWidget() is not a frame widget which is a case we should not update
anything in my opinion. This is probably the case where WebViewImpl is returned
by GetWebWidget(), i.e., through navigating back and forward. But as I
understand RWH for the main frame is destroyed anyway so the TextInputState will
reset and there will be no Ime when it is destroyed.

2- We do have a focused frame, but it does not belong to this local root
(subtree). Which then begs the question, why are we handling a long gesture
here? So |controller| cannot be null.

Given all this, I think (1) is the only scenario were controller can be null and
in that case, we should not even bother sending an IPC back.

[EhsanK, 2016-11-24 02:28:11]

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

[commit-bot: I haz the power, 2016-11-24 02:28:50]

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-11-24 02:42:44]

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

[commit-bot: I haz the power, 2016-11-24 02:42:45]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2016-11-24 03:25:28]

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

[commit-bot: I haz the power, 2016-11-24 03:25:53]

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

[EhsanK, 2016-11-24 22:13:08]

On 2016/11/24 02:27:45, EhsanK wrote:
> Thanks Charlie! I will try to land it. I made a slight change in response to
> your comment.
> 
>
https://codereview.chromium.org/2508363003/diff/20001/content/renderer/render...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/2508363003/diff/20001/content/renderer/render...
> content/renderer/render_widget.cc:2176: if (controller &&
> controller->textInputInfo().value.isEmpty())
> On 2016/11/23 23:35:09, Charlie Reis (slow) wrote:
> > Is this right?  This looks like it's hiding the IME if the TextInputInfo is
> > empty.  
> Yes. Apparently, this is because we should show a paste menu when long
pressing
> <input> and not IME. This is the CL that did this:
>
https://chromium.googlesource.com/chromium/src/+/07c70d207c4ed217286cdf842365...
> 
> > Maybe we should also be taking that branch if there's no controller? 
> > (Or is IF_NEEDED the right thing to do in that case?)
> > 
> > Just wondering if this should be:
> >   if (!controller || controller->textInputInfo...)
> That is very good question. The way I see it, there is two scenarios that will
> lead to |controller| being null.
> 
> 1- GetWebWidget() is not a frame widget which is a case we should not update
> anything in my opinion. This is probably the case where WebViewImpl is
returned
> by GetWebWidget(), i.e., through navigating back and forward. But as I
> understand RWH for the main frame is destroyed anyway so the TextInputState
will
> reset and there will be no Ime when it is destroyed.
> 
> 2- We do have a focused frame, but it does not belong to this local root
> (subtree). Which then begs the question, why are we handling a long gesture
> here? So |controller| cannot be null.
> 
> Given all this, I think (1) is the only scenario were controller can be null
and
> in that case, we should not even bother sending an IPC back.

Most of the tests (if not all) we failing due to the CHECK inside
RenderWidget::GetInputMethodController().
Since that check is already causing headaches in M56. For now this CL will be
blocked on the CL:
https://codereview.chromium.org/2522353002/. After fixing the issue with the
CHECK I believe this CL
is good to go (all tests are passing when trying locally after I remove the
check).

[EhsanK, 2016-12-20 22:26:34]

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

[commit-bot: I haz the power, 2016-12-20 22:27:32]

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-21 00:00:00]

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

[commit-bot: I haz the power, 2016-12-21 00:00:01]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-12-21 06:53:00]

After the rebase all bots seem to be green. I will try to CQ. Thanks!

[EhsanK, 2016-12-21 06:54:45]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-12-21 06:54:45]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
dtapuska@chromium.org, dglazkov@chromium.org
Link to the patchset: https://codereview.chromium.org/2508363003/#ps180001
(title: "Fixed Rebase")

[commit-bot: I haz the power, 2016-12-21 06:54:57]

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-21 07:01:27]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482303285570910,
"parent_rev": "b1ac0f7dc7f9d9c13fe5865e35e3b3984236c5ea", "commit_rev":
"013c571926b3880f4d69b8a6184b0bc221c56c38"}

[commit-bot: I haz the power, 2016-12-21 07:02:19]

Description was changed from

==========
[refactor] - Move textInputInfo() and textInputType() from WebWidget to
WebInputMethodController

In line with refactoring the duplicated IME code, the implementation of
WebWidget::textInputInfo() and WebWidget::textInputType() have been
moved to WebINputMethodController. The benefits of this refactor are:

 1- This is compliant by the fact that the actual logic lives in
 InputMethodController class. Therefore, it makes sense to have the
 calles use the public wrapper instead of WebWidget.

 2- This helps with the separation of WebView and WebWidget as the
 refactored methods are removed from both.

This CL follows up on the original refactoring CL which landed
WebInputMethodController (https://codereview.chromium.org/2333813002/).

Doc:
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

BUG=629721
==========

to

==========
[refactor] - Move textInputInfo() and textInputType() from WebWidget to
WebInputMethodController

In line with refactoring the duplicated IME code, the implementation of
WebWidget::textInputInfo() and WebWidget::textInputType() have been
moved to WebINputMethodController. The benefits of this refactor are:

 1- This is compliant by the fact that the actual logic lives in
 InputMethodController class. Therefore, it makes sense to have the
 calles use the public wrapper instead of WebWidget.

 2- This helps with the separation of WebView and WebWidget as the
 refactored methods are removed from both.

This CL follows up on the original refactoring CL which landed
WebInputMethodController (https://codereview.chromium.org/2333813002/).

Doc:
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

BUG=629721

Review-Url: https://codereview.chromium.org/2508363003
Committed:
https://chromium.googlesource.com/chromium/src/+/013c571926b3880f4d69b8a6184b...
==========

[commit-bot: I haz the power, 2016-12-21 07:02:20]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/013c571926b3880f4d69b8a6184b...

[commit-bot: I haz the power, 2016-12-21 07:04:01]

Description was changed from

==========
[refactor] - Move textInputInfo() and textInputType() from WebWidget to
WebInputMethodController

In line with refactoring the duplicated IME code, the implementation of
WebWidget::textInputInfo() and WebWidget::textInputType() have been
moved to WebINputMethodController. The benefits of this refactor are:

 1- This is compliant by the fact that the actual logic lives in
 InputMethodController class. Therefore, it makes sense to have the
 calles use the public wrapper instead of WebWidget.

 2- This helps with the separation of WebView and WebWidget as the
 refactored methods are removed from both.

This CL follows up on the original refactoring CL which landed
WebInputMethodController (https://codereview.chromium.org/2333813002/).

Doc:
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

BUG=629721

Review-Url: https://codereview.chromium.org/2508363003
Committed:
https://chromium.googlesource.com/chromium/src/+/013c571926b3880f4d69b8a6184b...
==========

to

==========
[refactor] - Move textInputInfo() and textInputType() from WebWidget to
WebInputMethodController

In line with refactoring the duplicated IME code, the implementation of
WebWidget::textInputInfo() and WebWidget::textInputType() have been
moved to WebINputMethodController. The benefits of this refactor are:

 1- This is compliant by the fact that the actual logic lives in
 InputMethodController class. Therefore, it makes sense to have the
 calles use the public wrapper instead of WebWidget.

 2- This helps with the separation of WebView and WebWidget as the
 refactored methods are removed from both.

This CL follows up on the original refactoring CL which landed
WebInputMethodController (https://codereview.chromium.org/2333813002/).

Doc:
https://docs.google.com/document/d/13Nidlg5yIsxQRqXgBPqmb-KcT8q85Z1ds9Z1-Vo1X...

BUG=629721

Committed: https://crrev.com/59cd849d12cffcc4f31281e758deb18111717796
Cr-Commit-Position: refs/heads/master@{#440046}
==========

[commit-bot: I haz the power, 2016-12-21 07:04:02]

Patchset 10 (id:??) landed as
https://crrev.com/59cd849d12cffcc4f31281e758deb18111717796
Cr-Commit-Position: refs/heads/master@{#440046}

[shimazu, 2016-12-21 08:38:55]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2592953003/ by shimazu@chromium.org.

The reason for reverting is: WebViewFocusInteractiveTest.Focus_FocusRestored got
flaky:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui....