Implement RenderWidgetHostViewBase::ImeCancelComposition for all Views (Aura Only)

Implement RenderWidgetHostViewBase::ImeCancelComposition for all Views (Aura Only)

Currently, the RenderWidgetHostViewChildFrame's do not implement this method.
This CL implements the method for RenderWidgetHostViewBase where the view will
forward the call to the TextInputManager. The TextInputManager will then call
its observers about the change. The tab's view in aura is an observer of the
TextInputManager which will in turn cancel any ongoing composition.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/50ee203f1d50521adeeb01faaa380e3c41c8b7a4
Cr-Commit-Position: refs/heads/master@{#402663}

[EhsanK, 2016-06-27 19:55:17]

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

[commit-bot: I haz the power, 2016-06-27 19:56:02]

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

[EhsanK, 2016-06-27 20:06:20]

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

[EhsanK, 2016-06-27 20:06:21]

Hello Ken,

Sorry for the load of reviews. Please take a look.

This is another small CL for renderer->browser communication.

Thanks,

Ehsan

[commit-bot: I haz the power, 2016-06-27 21:33:39]

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

[commit-bot: I haz the power, 2016-06-27 21:33:40]

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

[kenrb, 2016-06-28 15:12:41]

Mostly looks fine, but don't you need to remove the ImeCancelComposition
override in RenderWidgetHostViewChildFrame?

https://codereview.chromium.org/2098353005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2098353005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:416:
GetTextInputManager()->ImeCancelComposition(this);
I notice the method above this one has a USE_AURA guard, but that's not
necessary, right? GetTextInputManager() should return nullptr on non-Aura
platforms until it is implemented on them.

[EhsanK, 2016-06-28 16:38:26]

Description was changed from

==========
Implement RenderWidgetHostViewBase::ImeCancelComposition for all Views (Aura
Only)

Currently, the RenderWidgetHostViewChildFrame's do not implement this method.
This CL implements the method for RenderWidgetHostViewBase where the view will
forward the call to the TextInputManager. The TextInputManager will then call
its observers about the change. The tab's view in aura is an observer of the
TextInputManager which will in turn cancel any ongoing composition.

BUG=578168
==========

to

==========
Implement RenderWidgetHostViewBase::ImeCancelComposition for all Views (Aura
Only)

Currently, the RenderWidgetHostViewChildFrame's do not implement this method.
This CL implements the method for RenderWidgetHostViewBase where the view will
forward the call to the TextInputManager. The TextInputManager will then call
its observers about the change. The tab's view in aura is an observer of the
TextInputManager which will in turn cancel any ongoing composition.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[EhsanK, 2016-06-28 16:41:19]

PTAL.

Thanks Ken for reminding me to remove the override. I found it hard to find a
way to test this change.

https://codereview.chromium.org/2098353005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2098353005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.cc:416:
GetTextInputManager()->ImeCancelComposition(this);
On 2016/06/28 15:12:41, kenrb wrote:
> I notice the method above this one has a USE_AURA guard, but that's not
> necessary, right? GetTextInputManager() should return nullptr on non-Aura
> platforms until it is implemented on them.
We always create TextInputManager now which is on all platforms. Therefore this
will never be null.

But that being said, there is no reason to have this code path in other
platforms now since TextInputManager has no observers (tab's view) in other
platforms.

I will enclose it in #ifdefs.

[kenrb, 2016-06-28 16:52:58]

What are the problems with testing? Have you been able to test it manually with
OOPIFs?

[EhsanK, 2016-06-28 17:26:30]

On 2016/06/28 16:52:58, kenrb wrote:
> What are the problems with testing? Have you been able to test it manually
with
> OOPIFs?

I believe I did that a month ago and it was working fine. But I had the whole
IME
change in place with all the IPC from browser->renderer and back.

[kenrb, 2016-06-28 19:21:07]

lgtm, although it would be nice to have a test at some point to spot
regressions.

[EhsanK, 2016-06-28 19:38:07]

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

[EhsanK, 2016-06-28 19:38:07]

Thanks Ken!

I will keep looking to see if there is a way to send a
'InputHostMsg_ImeCancelComposition' from the renderer in an interactive test.

Adding creis@ as a reviewer.

Charlie, could you please take a look?

[Charlie Reis, 2016-06-28 21:33:46]

LGTM

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

https://codereview.chromium.org/2098353005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:415: #if
defined(USE_AURA)
Same TODO as above?

https://codereview.chromium.org/2098353005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2098353005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:261: virtual void
ImeCancelComposition();
Same TODO as TextInputStateChanged?

[EhsanK, 2016-06-28 22:04:34]

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

[EhsanK, 2016-06-28 22:05:01]

Thanks for the reviews. I will CQ after the dry-run.

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

https://codereview.chromium.org/2098353005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:415: #if
defined(USE_AURA)
On 2016/06/28 21:33:46, Charlie Reis wrote:
> Same TODO as above?

Acknowledged.

https://codereview.chromium.org/2098353005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2098353005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:261: virtual void
ImeCancelComposition();
On 2016/06/28 21:33:46, Charlie Reis wrote:
> Same TODO as TextInputStateChanged?

Acknowledged.

[commit-bot: I haz the power, 2016-06-28 22:06:19]

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-06-29 00:09:12]

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

[commit-bot: I haz the power, 2016-06-29 00:09:13]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-06-29 01:00:23]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-06-29 01:00:23]

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/2098353005/#ps60001
(title: "Addressing creis@'s comments")

[commit-bot: I haz the power, 2016-06-29 01:02:51]

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-06-29 02:18:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 02:19:57]

Description was changed from

==========
Implement RenderWidgetHostViewBase::ImeCancelComposition for all Views (Aura
Only)

Currently, the RenderWidgetHostViewChildFrame's do not implement this method.
This CL implements the method for RenderWidgetHostViewBase where the view will
forward the call to the TextInputManager. The TextInputManager will then call
its observers about the change. The tab's view in aura is an observer of the
TextInputManager which will in turn cancel any ongoing composition.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement RenderWidgetHostViewBase::ImeCancelComposition for all Views (Aura
Only)

Currently, the RenderWidgetHostViewChildFrame's do not implement this method.
This CL implements the method for RenderWidgetHostViewBase where the view will
forward the call to the TextInputManager. The TextInputManager will then call
its observers about the change. The tab's view in aura is an observer of the
TextInputManager which will in turn cancel any ongoing composition.

BUG=578168
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/50ee203f1d50521adeeb01faaa380e3c41c8b7a4
Cr-Commit-Position: refs/heads/master@{#402663}
==========

[commit-bot: I haz the power, 2016-06-29 02:19:58]

Patchset 4 (id:??) landed as
https://crrev.com/50ee203f1d50521adeeb01faaa380e3c41c8b7a4
Cr-Commit-Position: refs/heads/master@{#402663}