[reland, refactor] - Move textInputInfo() and textInputType() from WebWidget to WebInputMethodContr…

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

This CL tries to reland the original CL https://codereview.chromium.org/2508363003
which was reverted since it was making WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://codereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will not process IME events if RenderWidget::has_focus_ is false. This
    is achieved by adding a new method ShouldHandleImeEvents() which will
    return false when there is no WebFrameWidget or page focus.

This CL also adds a RenderWidgetTest to prevent future regressions of this kind.

BUG=629721
Review-Url: https://codereview.chromium.org/2608293002
Cr-Commit-Position: refs/heads/master@{#441788}
Committed: https://chromium.googlesource.com/chromium/src/+/5aff1941d0f7a2a68e07fd000592de295944fcea

[EhsanK, 2017-01-03 16:56:44]

Patchset #2 (id:20001) has been deleted

[EhsanK, 2017-01-04 18:29:33]

Description was changed from

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

This CL tries to reland the original CL
https://codereview.chromium.org/2508363003
which was reverted since it was making
WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://odereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will NOT process IME events if the WebFrameWidget for the WebLocalFrame's
       local root does not have page focus.

This CL also adds a WebFrameTest to prevent future regressions of this kind. The
test verifies that not having page focus will drop IME calls, but there is still
a WebInputMethodController and its textInputInfo()/textInputType() is correct.

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

to

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

This CL tries to reland the original CL
https://codereview.chromium.org/2508363003
which was reverted since it was making
WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://odereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will not process IME events if RenderWidget::has_focus_ is false. This
    is achieved by adding a new method ShouldHandleImeEvents() which will
    return false when there is no WebFrameWidget or page focus.

This CL also adds a RenderWidgetTest to prevent future regressions of this kind.

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

[EhsanK, 2017-01-04 18:31:40]

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

[commit-bot: I haz the power, 2017-01-04 18:32:16]

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

[EhsanK, 2017-01-04 20:26:52]

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

[EhsanK, 2017-01-04 20:26:53]

Dimitri, Charlie, could you please take a look (as the the owner reviewers of
the patch from previous attempt)?

Note: I could not repro the test failure using previous patch but I think this
patch should resolve the issue.

Thanks!

https://codereview.chromium.org/2608293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2608293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:467: m_imeAcceptEvents =
true;
This is irrelevant to this change but I happened to notice that we were not
setting its value when widget gets focus.

I also think we might be able to get rid of this state altogether given that
RenderWidget will not make any IME calls when the page focus is lost.

[commit-bot: I haz the power, 2017-01-04 21:17:03]

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

[commit-bot: I haz the power, 2017-01-04 21:17:04]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-05 01:01:55]

creis@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[Charlie Reis, 2017-01-05 01:01:56]

Sorry, I'm not knowledgeable enough about input events to offer a useful review
on the diff here.  Ken's out for a bit, but maybe James can review?  I'm happy
to offer an owners stamp if needed.

[dglazkov, 2017-01-05 16:27:50]

lgtm

[wjmaclean, 2017-01-05 21:24:57]

lgtm, with comments

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

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:2127: blink::WebInputMethodController*
controller = GetInputMethodController();
Why do you explicitly declare the type here, but use auto* in earlier places in
the file? I personally think this is clearer, as the reader doesn't need to look
up the type returned by GetInputMethodController(), but in either case it would
be good to be consistent.

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
File content/renderer/render_widget_browsertest.cc (right):

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
content/renderer/render_widget_browsertest.cc:199: // No give focus back again
and commit text.
s/No/Now/ ?

https://codereview.chromium.org/2608293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2608293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:467: m_imeAcceptEvents =
true;
On 2017/01/04 20:26:53, EhsanK wrote:
> This is irrelevant to this change but I happened to notice that we were not
> setting its value when widget gets focus.
> 
> I also think we might be able to get rid of this state altogether given that
> RenderWidget will not make any IME calls when the page focus is lost.

Perhaps add a TODO to follow up on a potential removal in another CL?

[EhsanK, 2017-01-05 21:34:21]

Description was changed from

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

This CL tries to reland the original CL
https://codereview.chromium.org/2508363003
which was reverted since it was making
WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://odereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will not process IME events if RenderWidget::has_focus_ is false. This
    is achieved by adding a new method ShouldHandleImeEvents() which will
    return false when there is no WebFrameWidget or page focus.

This CL also adds a RenderWidgetTest to prevent future regressions of this kind.

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

to

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

This CL tries to reland the original CL
https://codereview.chromium.org/2508363003
which was reverted since it was making
WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://codereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will not process IME events if RenderWidget::has_focus_ is false. This
    is achieved by adding a new method ShouldHandleImeEvents() which will
    return false when there is no WebFrameWidget or page focus.

This CL also adds a RenderWidgetTest to prevent future regressions of this kind.

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

[EhsanK, 2017-01-05 21:46:43]

Thank you all for the review!

PTAL.

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

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:2127: blink::WebInputMethodController*
controller = GetInputMethodController();
On 2017/01/05 21:24:57, wjmaclean wrote:
> Why do you explicitly declare the type here, but use auto* in earlier places
in
> the file? I personally think this is clearer, as the reader doesn't need to
look
> up the type returned by GetInputMethodController(), but in either case it
would
> be good to be consistent.
This format goes back to this CL:
https://codereview.chromium.org/2333813002/ (Please see comments by creis@ and
dglazkov@). 

In short, using auto* makes it more concise. But creis@ pointed out that it is
better to use the exact type instead and only use the auto* when inside
conditionals. I could change all to auto* of course if everybody is OK with it.

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
File content/renderer/render_widget_browsertest.cc (right):

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
content/renderer/render_widget_browsertest.cc:199: // No give focus back again
and commit text.
On 2017/01/05 21:24:57, wjmaclean wrote:
> s/No/Now/ ?

Now. Done. Thanks!

https://codereview.chromium.org/2608293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2608293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:467: m_imeAcceptEvents =
true;
On 2017/01/05 21:24:57, wjmaclean wrote:
> On 2017/01/04 20:26:53, EhsanK wrote:
> > This is irrelevant to this change but I happened to notice that we were not
> > setting its value when widget gets focus.
> > 
> > I also think we might be able to get rid of this state altogether given that
> > RenderWidget will not make any IME calls when the page focus is lost.
> 
> Perhaps add a TODO to follow up on a potential removal in another CL?

Done. Thanks!

[Charlie Reis, 2017-01-05 22:25:03]

Thanks for taking a look, James.  RS LGTM for content/.

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

https://codereview.chromium.org/2608293002/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:2127: blink::WebInputMethodController*
controller = GetInputMethodController();
On 2017/01/05 21:46:43, EhsanK wrote:
> On 2017/01/05 21:24:57, wjmaclean wrote:
> > Why do you explicitly declare the type here, but use auto* in earlier places
> in
> > the file? I personally think this is clearer, as the reader doesn't need to
> look
> > up the type returned by GetInputMethodController(), but in either case it
> would
> > be good to be consistent.
> This format goes back to this CL:
> https://codereview.chromium.org/2333813002/ (Please see comments by creis@ and
> dglazkov@). 
> 
> In short, using auto* makes it more concise. But creis@ pointed out that it is
> better to use the exact type instead and only use the auto* when inside
> conditionals. I could change all to auto* of course if everybody is OK with
it.

Yeah, auto wasn't buying much here in terms of space, and it was more useful to
know the type (as noted
inhttps://codereview.chromium.org/2333813002/diff/490001/content/renderer/render_widget.cc#newcode1401).
 

I'm ok either way with the other uses of auto-- I tend to like having the
explicit type, but I can see how they save space.

[EhsanK, 2017-01-05 23:20:13]

Thanks everyone for the reviews! I will try to CQ and hopefully this CL will
stay landed.

[EhsanK, 2017-01-05 23:20:42]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2017-01-05 23:20:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dglazkov@chromium.org, wjmaclean@chromium.org
Link to the patchset: https://codereview.chromium.org/2608293002/#ps80001
(title: "Addressing wjmaclean@'s comments")

[commit-bot: I haz the power, 2017-01-05 23:21:25]

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, 2017-01-06 01:26:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483658442529610,
"parent_rev": "3194c19094e8d713487876ebfd15821752a31739", "commit_rev":
"5aff1941d0f7a2a68e07fd000592de295944fcea"}

[commit-bot: I haz the power, 2017-01-06 01:27:18]

Description was changed from

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

This CL tries to reland the original CL
https://codereview.chromium.org/2508363003
which was reverted since it was making
WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://codereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will not process IME events if RenderWidget::has_focus_ is false. This
    is achieved by adding a new method ShouldHandleImeEvents() which will
    return false when there is no WebFrameWidget or page focus.

This CL also adds a RenderWidgetTest to prevent future regressions of this kind.

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

to

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

This CL tries to reland the original CL
https://codereview.chromium.org/2508363003
which was reverted since it was making
WebViewInteractiveTest.Focus_FocusRestored
flake.

The issue in the initial attempt was that
WebFrameWidget::getActiveInputMethodController() would return nullptr when the
WebFrameWidget did not have page focus (m_imeAcceptEvents == false). This
assumption is not correct and resulted into reporting
ui::TEXT_INPUT_TYPE_NONE for TextInputState right after losing page focus, e.g.,
by moving focus from a <webview> to its embedder (in the reverted CL).

On the other hand, when WebFrameWidget does not have page focus, any IME calls
received from browser should be dropped. Specifically, by not doing so, a call
to WebInputMethodController::finishComposingText would lead to a second commit
for the last ongoing composition. This happens when switching tabs or moving
focus to another WebFrameWidget in OOPIFs. Returning nullptr for active
controller was used as the solution to this problem in the original CL that
landed WebInputMethodController (https://codereview.chromium.org/2333813002/).
This is however incorrect since an unfocused WebFrameWidget could still have
IME state such as TextInputState.

The new attempt will reland the reverted CL and fix the issues by:

    1- Returning WebInputMethodController iff there is a focused local frame
    inside WebFrameWidget. Page focus will not affect this value.

    2- Will not process IME events if RenderWidget::has_focus_ is false. This
    is achieved by adding a new method ShouldHandleImeEvents() which will
    return false when there is no WebFrameWidget or page focus.

This CL also adds a RenderWidgetTest to prevent future regressions of this kind.

BUG=629721

Review-Url: https://codereview.chromium.org/2608293002
Cr-Commit-Position: refs/heads/master@{#441788}
Committed:
https://chromium.googlesource.com/chromium/src/+/5aff1941d0f7a2a68e07fd000592...
==========

[commit-bot: I haz the power, 2017-01-06 01:27:19]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5aff1941d0f7a2a68e07fd000592...