Implement Input Method related WebPlugin interface for browser plugin

Implement Input Method related WebPlugin interface for browser plugin

This implements input method-related interfaces defined in WebPlugin
https://codereview.chromium.org/102963005
to enable minimum support of IME for brower plugin (e.g. <webview> for
Chrome apps).

BUG=235573
TEST=content_browsertests --gtest_filter=BrowserPluginHostTest.InputMethod

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240836

[kochi, 2013-12-10 08:30:08]

Hi reviewers,

This is the chromium part of enabling Input Methods for
browser_plugin (e.g. <webview>).

nona@chromium.org: Please review if the coverage of
IME-related IPCs are good enough.

fsamuel@chromium.org: Please review changes in browser_plugin code.

sadrul@chromium.org: Please review changes in RWHV* code as OWNERS,
once this gets LGTM from nona & fsamuel.

cevans@chromium.org: Please review changes in 
browser_plugin_messages.h.

[Fady Samuel, 2013-12-10 17:55:32]

https://codereview.chromium.org/103403006/diff/30001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/103403006/diff/30001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:514:
IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_ImeConfirmComposition,
Alphabetical order.

https://codereview.chromium.org/103403006/diff/30001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:516:
IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_ImeSetComposition,
Alphabetical order.

https://codereview.chromium.org/103403006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/103403006/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_guest.cc:341:
guest_->GetEmbedderRenderWidgetHostView());
rwhv may be null.

https://codereview.chromium.org/103403006/diff/30001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/103403006/diff/30001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:1329: int selectionEnd) {
if (!HasGuestInstanceID())
  return false;

https://codereview.chromium.org/103403006/diff/30001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:1347:
blink::WebWidget::ConfirmCompositionBehavior selectionBehavior) {
if (!HasGuestInstanceID())
  return false;

[kochi, 2013-12-11 01:35:20]

will add a browsertest.

https://codereview.chromium.org/103403006/diff/30001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/103403006/diff/30001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:514:
IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_ImeConfirmComposition,
On 2013/12/10 17:55:32, Fady Samuel wrote:
> Alphabetical order.

Done.

https://codereview.chromium.org/103403006/diff/30001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:516:
IPC_MESSAGE_HANDLER(BrowserPluginHostMsg_ImeSetComposition,
On 2013/12/10 17:55:32, Fady Samuel wrote:
> Alphabetical order.

Done.

https://codereview.chromium.org/103403006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/103403006/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_guest.cc:341:
guest_->GetEmbedderRenderWidgetHostView());
On 2013/12/10 17:55:32, Fady Samuel wrote:
> rwhv may be null.

Added a check.

https://codereview.chromium.org/103403006/diff/30001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/103403006/diff/30001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:1329: int selectionEnd) {
On 2013/12/10 17:55:32, Fady Samuel wrote:
> if (!HasGuestInstanceID())
>   return false;

Done.

Thanks for pointing out.

https://codereview.chromium.org/103403006/diff/30001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:1347:
blink::WebWidget::ConfirmCompositionBehavior selectionBehavior) {
On 2013/12/10 17:55:32, Fady Samuel wrote:
> if (!HasGuestInstanceID())
>   return false;

Done.

[Fady Samuel, 2013-12-11 01:38:06]

Will lg tm when you add a browser test. Thanks!

https://codereview.chromium.org/103403006/diff/70001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/103403006/diff/70001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_guest.cc:339: if (rwhv) {
Early exit preferred:

if (!rwhv)
  return;

[kochi, 2013-12-11 04:05:06]

I'll work on adding a browser test today, and ask you
another review.  Thanks!

https://codereview.chromium.org/103403006/diff/70001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/103403006/diff/70001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_guest.cc:339: if (rwhv) {
On 2013/12/11 01:38:06, Fady Samuel wrote:
> Early exit preferred:
> 
> if (!rwhv)
>   return;

Done.

[sadrul, 2013-12-11 07:00:59]

https://codereview.chromium.org/103403006/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/103403006/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:1788:
render_widget_host->ChangeTextInputType(type, input_mode, can_compose_inline);
If I am following the code correctly: this calls
RenderWidgetHostImpl::ChangeTextInputType ->
RenderWidgetHostViewGuest::TextInputTypeChanged -> RenderWidgetHostView* (of the
embedder)::TextInputTypeChanged. Is that right? If so, why do you need the
changes in RenderWidgetHostImpl? Why not directly call into
RWHVGuest::TextInputTypeChanged() from here?

[kochi, 2013-12-11 08:32:11]

Thanks for the review!

https://codereview.chromium.org/103403006/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/103403006/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:1788:
render_widget_host->ChangeTextInputType(type, input_mode, can_compose_inline);
On 2013/12/11 07:01:00, sadrul wrote:
> If I am following the code correctly: this calls
> RenderWidgetHostImpl::ChangeTextInputType ->
> RenderWidgetHostViewGuest::TextInputTypeChanged -> RenderWidgetHostView* (of
the
> embedder)::TextInputTypeChanged. Is that right? If so, why do you need the
> changes in RenderWidgetHostImpl? Why not directly call into
> RWHVGuest::TextInputTypeChanged() from here?

Yes your understanding is correct, and as your suggestion looks better, adopted
the idea.

[sadrul, 2013-12-11 08:35:09]

cool. The change in RenderWidgetHostViewGuest looks reasonable to me. I will
stamp after reviews from fsamuel@/nona@. In the meantime, is there a good way to
test this?

[kochi, 2013-12-11 08:57:01]

On 2013/12/11 08:35:09, sadrul wrote:
> cool. The change in RenderWidgetHostViewGuest looks reasonable to me. I will
> stamp after reviews from fsamuel@/nona@. In the meantime, is there a good way
to
> test this?

I'm working on adding a test in browser_plugin_host_browsertest.cc.
Basically I'm considering sending ViewMsg_ImeSetComposition etc. and check if
the messages are handled
in guest.

[kochi, 2013-12-11 11:19:09]

Hi Fady,

I added a browsertest, which fails yet.

I must be misunderstanding the layering of browser_plugin...
I expected that when I sent ImeSetCompositoin message to
embedder's RWH, it gets sent to the default renderer, then
redirected back to browser_plugin_guest, but it is not.

[Seigo Nonaka, 2013-12-12 04:19:55]

Following IPCs are used in Desktop IME implementation.

ViewHostMsg_ImeCancelComposition
ViewHostMsg_SelectionChanged,
ViewHostMsg_SelectionBoundsChanged,
ViewHostMsg_TextInputTypeChanged

ViewMsg_ImeSetComposition,
ViewMsg_ImeConfirmComposition,
ViewMsg_SetInputMethodActive,  // not sure this is alive or not. probably
yukawa@ know this.

I can not see SelectionBoundsChanged/SelectionChanged/SetInputMethodActive IPC
handling.
Are these already supported? or just not necessary for browser plugin?

Thank you.

[kochi, 2013-12-12 05:27:46]

Thanks for the information.

On 2013/12/12 04:19:55, Seigo Nonaka wrote:
> Following IPCs are used in Desktop IME implementation.
> 
> ViewHostMsg_ImeCancelComposition
> ViewHostMsg_SelectionChanged,
> ViewHostMsg_SelectionBoundsChanged,
> ViewHostMsg_TextInputTypeChanged
> 
> ViewMsg_ImeSetComposition,
> ViewMsg_ImeConfirmComposition,
> ViewMsg_SetInputMethodActive,  // not sure this is alive or not. probably
> yukawa@ know this.
> 
> I can not see SelectionBoundsChanged/SelectionChanged/SetInputMethodActive IPC
> handling.
> Are these already supported? or just not necessary for browser plugin?

The browser plugin sets SetInputMethodActive to true at initialization, and
it is always true.  So this is not a problem.

SelectionBoundsChanged is already handled in RenderWidgetHostViewGuest (just
forwards
the IPC to embedder).

For SelectionChanged, changed in patchset9 so that it is handled in the same
manner
as other IME related IPCs.

> Thank you.

[kochi, 2013-12-12 07:33:47]

On 2013/12/11 11:19:09, Takayoshi Kochi wrote:
> Hi Fady,
> 
> I added a browsertest, which fails yet.
> 
> I must be misunderstanding the layering of browser_plugin...
> I expected that when I sent ImeSetCompositoin message to
> embedder's RWH, it gets sent to the default renderer, then
> redirected back to browser_plugin_guest, but it is not.

This was because the <object id=plugin> element in browser_plugin_embedder.html
was not getting focus.  To make it work properly, I had to add
$('plugin').focus()
to the test, then it worked as expected.

I was assuming that if the browser_plugin content gets focus by mouse, the
embedding
HTML's <object> element is also focused, and WebViewImpl.cpp was written
assuming
this behavior.

I don't think scripted focus() on guest content must propagate its focus
to embedding element, but what is expected behavior of this case?

[Seigo Nonaka, 2013-12-12 09:00:50]

Sorry I forgot one thing.

ViewMsg_ExtendSelectionAndDelete should be also supported which is probably used
by Thai input.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Thank you.

[kochi, 2013-12-12 09:58:19]

On 2013/12/12 09:00:50, Seigo Nonaka wrote:
> Sorry I forgot one thing.
> 
> ViewMsg_ExtendSelectionAndDelete should be also supported which is probably
used
> by Thai input.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> Thank you.

Done.
Blink part is https://codereview.chromium.org/100193005/.

This CL should be landed after the blink counterpart lands.

[Fady Samuel, 2013-12-12 16:53:02]

browser_plugin lgtm

[kochi, 2013-12-13 01:19:44]

On 2013/12/12 16:53:02, Fady Samuel wrote:
> browser_plugin lgtm

Thanks,

nona-san,
could you take another look?

[kochi, 2013-12-13 01:21:36]

In case Chris is not available, added cdn@ for reviewing
browser_plugin_messages.h.

[Seigo Nonaka, 2013-12-13 03:08:58]

lgtm

[sadrul, 2013-12-13 03:10:27]

lgtm

[kochi, 2013-12-13 03:42:44]

Adding more reviewers for browser_plugin_messages.h.
Can anyone take a look?

Added 3 messages are basically identical to their corresponding ViewMsg_*, just
for redirecting the message
back to browser_plugin_guest.

[kochi, 2013-12-13 06:41:23]

https://codereview.chromium.org/103403006/diff/290001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/103403006/diff/290001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_guest.cc:333: }
Self-review: This part was missing.

As in SelectionBoundsChanged() below, any coordinates should
be translated for guest here.

[kochi, 2013-12-13 06:47:57]

Fady, could you review the bit I changed between patchset 14 and 15?
Thanks, and sorry for the last minute change.

[Tom Sepez, 2013-12-13 18:25:22]

browser_plugin_messages.h LGTM

[commit-bot: I haz the power, 2013-12-13 22:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/103403006/290001

[Fady Samuel, 2013-12-13 22:49:30]

lgtm patch set 15.

[commit-bot: I haz the power, 2013-12-14 01:31:38]

Change committed as 240836