The first step for enabling off-the-spot IME on Pepper on ChromeOS/Linux.

The first step for enabling off-the-spot IME on Pepper on ChromeOS/Linux.

BUG=83684
TEST=Go to http://weathernews.jp with pepper flash plug in enabled.
  Focus the text input field and type something with IME on.
  Verify that the candidate window is shown somewhere on the screen
  and commited text is entered in the text field.

This patch is the first step toward off-the-spot IME on Pepper plugins
hosted on RenderWidgetHostViewGtk. This patch:
- Makes it possible to turn on/off IME when a plugin is focused.
- Makes it possible for the plugin to receive committed results from IME.
What are NOT IN this patch (and what I hope to improve in forthcoming
separate patches) are:
- Proper placement of the candidate window.
- Showing preedit texts.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87215

[James Su, 2011-05-25 04:31:34]

Glad to know that you are working on it.

I'm wondering if it can be done completely inside webkit, as if a plugin is just
a node on the HTML DOM tree, similar than how IME is supported on normal HTML
nodes.

And IMHO, we need to add IME support to PPAPI directly, so that we can support
following things beside inline composition and over-the-spot candidate window:
1. Enable/disable/change the current input method according to the type of
focused control inside the plugin. E.g. we should not enable IME when a password
box is focused.
2. Reset input session when changing focus among different controls inside the
plugin.

[kochi, 2011-05-25 05:51:19]

Yes, we should add IME support in the PPAPI level, extending existing
keyboard/char events.
Please understand that this is an intermediate solution until we get there
(especially for forthcoming
M13 branch).

Those new events and communications between Chrome and the Flash player need
changes inside
Flash player, which is not under control of us (although we can ask them)
and take some time, and
we already started designing this.

Thanks for your input.

On Wed, May 25, 2011 at 1:31 PM, <suzhe@chromium.org> wrote:

> Glad to know that you are working on it.
>
> I'm wondering if it can be done completely inside webkit, as if a plugin is
> just
> a node on the HTML DOM tree, similar than how IME is supported on normal
> HTML
> nodes.
>
> And IMHO, we need to add IME support to PPAPI directly, so that we can
> support
> following things beside inline composition and over-the-spot candidate
> window:
> 1. Enable/disable/change the current input method according to the type of
> focused control inside the plugin. E.g. we should not enable IME when a
> password
> box is focused.
> 2. Reset input session when changing focus among different controls inside
> the
> plugin.
>
>
> http://codereview.chromium.org/6990072/
>



-- 
Takayoshi Kochi

[kochi, 2011-05-25 06:19:44]

First round of comments.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
File chrome/browser/renderer_host/gtk_im_context_wrapper.cc (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:245: ||
is_ppapi_plugin_focused_;
Put || in the end of the previous line, rather than this line.

See
http://dev.chromium.org/developers/coding-style
"Code formatting"

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:475: // For now, if a
PPAPI plugin is focused, we don't send ImeSetCompostion.
"For now" sounds ambiguous for any future reference on this
comment.

TODO(kinaba): (what you want to do in the future)

is preferred.

That said, as the similar comments appear in the following lines, how about one
TODO() comment on line 243 to cover why we want to special case PPAPI only?

For this instance, "if a PPAPI plugin is focused, we don't send
ImeSetComposition" says exactly what code does and adds not much information
than the code itself.  Please describe why we have to exclude PPAPI here.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:526: // events, instead
of sending renderer an IME event.
For now -> TODO().

E.g. TODO(kinaba): Until PPAPI has an interface for sending an IME event, we
send character events.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:527: for (size_t i = 0;
i<commit_text_.size(); ++i) {
Style nit: spaces around "<".

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:579: // For now, if a
PPAPI plugin is focused, we don't send ImeSetComposition.
Ditto.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:663: void
RenderWidgetHostViewMac::PpapiPluginFocusChanged(bool focused) {
TODO(kinaba): Implement this.
is fine for now.

If there is no need to do anything on Mac, please write  a comment about the
reason here.

BTW, do you have an environment to test Mac?

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.h:86: virtual void
PpapiPluginFocusChanged(bool focused) {}
Why didn't you add empty function in .cc file as you did
for mac/win?

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_win.cc:503: void
RenderWidgetHostViewWin::PpapiPluginFocusChanged(bool focused) {
Add TODO() or a comment why no implementation here.

http://codereview.chromium.org/6990072/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/6990072/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:1764: // Informs the browser that a plugin has
gained or lost focus.
Please be specific that this is "a ppapi plugin".

[James Su, 2011-05-25 07:04:24]

On 2011/05/25 05:51:19, Takayoshi Kochi wrote:
> Yes, we should add IME support in the PPAPI level, extending existing
> keyboard/char events.
> Please understand that this is an intermediate solution until we get there
> (especially for forthcoming
> M13 branch).
Yes I understand, but I still think it could be done inside webkit (at most
inside renderer), so that we don't need to touch so many files in browser side.
A possible approach for such an intermediate solution would be: modify
RenderWidget::UpdateInputMethod() code to send back WebKit::WebTextInputTypeText
when a ppapi plugin is focused, and modify
RenderWidget::OnImeConfirmComposition() to convert composition text into char
events. RenderWidget::OnImeSetComposition() may also need to be modified to
discard composition text in such case.
Another better approach is to do it insde
third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp, so that we don't
need to touch chrome code at all.

> 
> Those new events and communications between Chrome and the Flash player need
> changes inside
> Flash player, which is not under control of us (although we can ask them)
> and take some time, and
> we already started designing this.
Great to know that you already started working on it. Is there any design doc
that I can have a look?

> 
> Thanks for your input.
> 
> On Wed, May 25, 2011 at 1:31 PM, <mailto:suzhe@chromium.org> wrote:
> 
> > Glad to know that you are working on it.
> >
> > I'm wondering if it can be done completely inside webkit, as if a plugin is
> > just
> > a node on the HTML DOM tree, similar than how IME is supported on normal
> > HTML
> > nodes.
> >
> > And IMHO, we need to add IME support to PPAPI directly, so that we can
> > support
> > following things beside inline composition and over-the-spot candidate
> > window:
> > 1. Enable/disable/change the current input method according to the type of
> > focused control inside the plugin. E.g. we should not enable IME when a
> > password
> > box is focused.
> > 2. Reset input session when changing focus among different controls inside
> > the
> > plugin.
> >
> >
> > http://codereview.chromium.org/6990072/
> >
> 
> 
> 
> -- 
> Takayoshi Kochi

[kochi, 2011-05-25 11:21:14]

On Wed, May 25, 2011 at 4:04 PM, <suzhe@chromium.org> wrote:

> On 2011/05/25 05:51:19, Takayoshi Kochi wrote:
>
>> Yes, we should add IME support in the PPAPI level, extending existing
>> keyboard/char events.
>> Please understand that this is an intermediate solution until we get there
>> (especially for forthcoming
>> M13 branch).
>>
> Yes I understand, but I still think it could be done inside webkit (at most
> inside renderer), so that we don't need to touch so many files in browser
> side.
> A possible approach for such an intermediate solution would be: modify
> RenderWidget::UpdateInputMethod() code to send back
> WebKit::WebTextInputTypeText
> when a ppapi plugin is focused, and modify
> RenderWidget::OnImeConfirmComposition() to convert composition text into
> char
> events. RenderWidget::OnImeSetComposition() may also need to be modified to
> discard composition text in such case.
> Another better approach is to do it insde
> third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp, so that we
> don't
> need to touch chrome code at all.
>
>
Sorry for my naive question, but doesn't it break Win/Mac?
We would like to minimize the effect of the change within ChromeOS for now.

But for more generic solution, your insights sounds correct.


>  Those new events and communications between Chrome and the Flash player
>> need
>> changes inside
>> Flash player, which is not under control of us (although we can ask them)
>> and take some time, and
>> we already started designing this.
>>
> Great to know that you already started working on it. Is there any design
> doc
> that I can have a look?
>
>
Yes, I'll let you know when it's ready and ask you for your reviews ;)
I'll try to loop you in as early as possible.


>  Thanks for your input.
>>
>
>  On Wed, May 25, 2011 at 1:31 PM, <mailto:suzhe@chromium.org> wrote:
>>
>
>  > Glad to know that you are working on it.
>> >
>> > I'm wondering if it can be done completely inside webkit, as if a plugin
>> is
>> > just
>> > a node on the HTML DOM tree, similar than how IME is supported on normal
>> > HTML
>> > nodes.
>> >
>> > And IMHO, we need to add IME support to PPAPI directly, so that we can
>> > support
>> > following things beside inline composition and over-the-spot candidate
>> > window:
>> > 1. Enable/disable/change the current input method according to the type
>> of
>> > focused control inside the plugin. E.g. we should not enable IME when a
>> > password
>> > box is focused.
>> > 2. Reset input session when changing focus among different controls
>> inside
>> > the
>> > plugin.
>> >
>> >
>> > http://codereview.chromium.org/6990072/
>> >
>>
>
>
>
>  --
>> Takayoshi Kochi
>>
>
>
>
> http://codereview.chromium.org/6990072/
>



-- 
Takayoshi Kochi

[oshima, 2011-05-25 17:18:18]

just style nits

http://codereview.chromium.org/6990072/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_render_widget_host_view.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_render_widget_host_view.h:62: 
remove empty line

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.h:57: const gfx::Rect&
pos);
can you add OVERRIDE while you're at it?

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.h:96: 
We should remove this line as well, unless they're from different interface.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.h:99: 
ditto

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.h:222: 
ditto (line, OVERRIDE)

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.h:85: 
ditto

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.h:86: virtual void
PpapiPluginFocusChanged(bool focused) {}
agreed. OVERRIDE as well.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_win.h:169: 
ditto (line, OVERRIDE)

http://codereview.chromium.org/6990072/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/6990072/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.h:165: virtual void
PluginFocusChanged(bool focused);
OVERRIDE

[kinaba, 2011-05-25 20:10:55]

On 2011/05/25 07:04:24, James Su wrote:
> A possible approach for such an intermediate solution would be: modify
> RenderWidget::UpdateInputMethod() code to send back
WebKit::WebTextInputTypeText
> when a ppapi plugin is focused, and modify
> RenderWidget::OnImeConfirmComposition() to convert composition text into char
> events. RenderWidget::OnImeSetComposition() may also need to be modified to
> discard composition text in such case.

This way of keeping modifications inside the renderer sounds good
to me. I will reorganize the patch right now along this direction.
Thank you for nice suggestion.

On 2011/05/25 11:21:14, Takayoshi Kochi wrote:
> Sorry for my naive question, but doesn't it break Win/Mac?
> We would like to minimize the effect of the change within ChromeOS for now.

It surely affects Win/Mac. But if I understand correctly, the Pepper-IME
issue is common to all platforms and is even desirable if it could be
dealt with in such a platform independent manner.

If my understanding is wrong, I think we still can #ifdef CHROMEOS
out the code just as we already have many #ifdef MACOS stuff for
NPAPI+IME handling in RenderView.

On 2011/05/25 07:04:24, James Su wrote:
> Another better approach is to do it insde
> third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp, so that we
don't
> need to touch chrome code at all.

I agree this is another possible way.
I chose Chrome-side fix rather than WebKit-side fix because it
should be quicker for landing till M13 branch.

[kinaba, 2011-05-25 22:23:53]

I've reorganized the patch so that the core of the modification goes into the
renderer rather than the browser.

For missing OVERRIDE's in RenderWidgetHostViewXXX (comments by oshima),
since this patch no more touches these classes, I
will take care of it in a separate patch.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_render_widget_host_view.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_render_widget_host_view.h:62: 
On 2011/05/25 17:18:18, oshima wrote:
> remove empty line

The whole change here is removed from the patch set 2.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
File chrome/browser/renderer_host/gtk_im_context_wrapper.cc (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:245: ||
is_ppapi_plugin_focused_;
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> Put || in the end of the previous line, rather than this line.
> 
> See
> http://dev.chromium.org/developers/coding-style
> "Code formatting"

The whole change here is removed from the patch set 2.
Thank you for the pointer. I'll read it carefully.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:475: // For now, if a
PPAPI plugin is focused, we don't send ImeSetCompostion.
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> "For now" sounds ambiguous for any future reference on this
> comment.
> 
> TODO(kinaba): (what you want to do in the future)
> 
> is preferred.
> 
> That said, as the similar comments appear in the following lines, how about
one
> TODO() comment on line 243 to cover why we want to special case PPAPI only?
> 
> For this instance, "if a PPAPI plugin is focused, we don't send
> ImeSetComposition" says exactly what code does and adds not much information
> than the code itself.  Please describe why we have to exclude PPAPI here.

Done (the code is moved to content/renderer/render_view.cc:3951).

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:526: // events, instead
of sending renderer an IME event.
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> For now -> TODO().
> 
> E.g. TODO(kinaba): Until PPAPI has an interface for sending an IME event, we
> send character events.

Done (the code is moved to content/renderer/render_view.cc:3967).

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:527: for (size_t i = 0;
i<commit_text_.size(); ++i) {
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> Style nit: spaces around "<".

Done (the code is moved to content/renderer/render_view.cc:3969).

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/gt...
chrome/browser/renderer_host/gtk_im_context_wrapper.cc:579: // For now, if a
PPAPI plugin is focused, we don't send ImeSetComposition.
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> Ditto.

Done (content/renderer/render_view.cc:3951).

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:663: void
RenderWidgetHostViewMac::PpapiPluginFocusChanged(bool focused) {
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> TODO(kinaba): Implement this.
> is fine for now.
> 
> If there is no need to do anything on Mac, please write  a comment about the
> reason here.
> 
> BTW, do you have an environment to test Mac?

Removed from the patch set 2.
I have, but it is a slow laptop, and I don't have for Win at all.

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_views.h:86: virtual void
PpapiPluginFocusChanged(bool focused) {}
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> Why didn't you add empty function in .cc file as you did
> for mac/win?

(Removed from the patch set 2.)

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6990072/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_win.cc:503: void
RenderWidgetHostViewWin::PpapiPluginFocusChanged(bool focused) {
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> Add TODO() or a comment why no implementation here.

(Removed from the patch set 2.)

http://codereview.chromium.org/6990072/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/6990072/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:1764: // Informs the browser that a plugin has
gained or lost focus.
On 2011/05/25 06:19:44, Takayoshi Kochi wrote:
> Please be specific that this is "a ppapi plugin".

(Removed from the patch set 2.)

http://codereview.chromium.org/6990072/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/6990072/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.h:165: virtual void
PluginFocusChanged(bool focused);
On 2011/05/25 17:18:18, oshima wrote:
> OVERRIDE

Done.

[James Su, 2011-05-26 01:29:55]

This CL looks much simpler now :)

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_widg...
File content/renderer/render_widget.h (right):

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_widg...
content/renderer/render_widget.h:278: WebKit::WebRect* caret_bounds);
How about just return the text input type? We don't need to care about the caret
bounds for now.

[brettw, 2011-05-26 02:57:26]

LGTM for a high level (so you don't have to wait for another pass from me in MTV
time, please be sure the other reviewers are happy with the details).

Just one comment:

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_view...
content/renderer/render_view.h:1065: bool is_ppapi_plugin_focused_;
Can this state be tracked on the PepperPluginDelegateImpl? RenderView can call
into that to query state as needed. But it's nice to keep RenderView as small as
possible and keep the Pepper stuff in one spot.

[kochi, 2011-05-26 03:44:48]

LGTM once James and Brett's comments are resolved.

It looks much safer than the previous to land and further
cleanup.

[oshima, 2011-05-26 18:39:46]

LGTM on my bits

[kinaba, 2011-05-30 03:07:11]

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_view...
content/renderer/render_view.h:1065: bool is_ppapi_plugin_focused_;
On 2011/05/26 02:57:26, brettw wrote:
> Can this state be tracked on the PepperPluginDelegateImpl? RenderView can call
> into that to query state as needed. But it's nice to keep RenderView as small
as
> possible and keep the Pepper stuff in one spot.

Done.
(The variable is moved to PepperPluginDelegateImpl, but I still kept
the notification function PpapiPluginFocusChanged for invoking
UpdateInputMethod() at that timing.)

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_widg...
File content/renderer/render_widget.h (right):

http://codereview.chromium.org/6990072/diff/7003/content/renderer/render_widg...
content/renderer/render_widget.h:278: WebKit::WebRect* caret_bounds);
On 2011/05/26 01:29:56, James Su wrote:
> How about just return the text input type? We don't need to care about the
caret
> bounds for now.

Done.

[James Su, 2011-05-30 06:09:49]

LGTM.