Show composition text on IME panel when Pepper plugin is focused (Linux).

Show composition text on IME panel when Pepper plugin is focused (Linux).

BUG=83684
TEST=1. On Linux Chrome, load some PPAPI plugin (e.g., ppapi/example)
2. Focus the plugin element
3. Turn on IME and type some text
4. Verify composition text is drawn on the IME candidate panel.

This is the second step for enabling off-the-spot IME on Pepper on ChromeOS/Linux,
continuing from the first step r87215. This patch includes

[many places]
  Add a boolean flag to indicate inline composition is supported or not.
  (WebKit should also be updated for supporting this kind
   of information for the final complete version of Pepper
   IME API, but not in this patch)
[chrome/browser/renderer_host/render_widget_host_view_gtk.cc]
  Control IME context to show composition text on candidate window.
  (In this patch, only RenderWidgetHostViewGtk, i.e., Linux and ChromeOS
   is considered. On Windows and Mac, the behavior stays unchanged.)
[content/renderer/render_view.cc]
  Turn the flag off when Pepper is focused.

and not yet include

- Enhancement of ChromeOS candidate window to have composition text area.
- Proper placement of the candidate window.

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

[kinaba, 2011-06-08 07:23:46]

This is the second step for enabling off-the-spot Pepper IME continuing from
http://codereview.chromium.org/6990072.
Contrary to the previous cl that was meant to be a temporal change for
short-term fix,
I suppose this one to be enduring (even after full-fledged support Pepper IME
API is established, some plugins might not want to bother rendering preedit
texts by themselves and be able to throw the task to browser).

[kochi, 2011-06-09 06:17:33]

LGTM

How about adding Oshima-san as a reviewer as well?

[James Su, 2011-06-09 07:50:21]

Instead of adding a new text input type, I'd prefer to use a separate flag to
indicate if the renderer support inline composition or not. So that when we add
more text input types, we don't need to add two variants for each one.

And we need to improve the code in views/ime/* to support it as well.

[kinaba, 2011-06-10 13:03:07]

On 2011/06/09 07:50:21, James Su wrote:
> Instead of adding a new text input type, I'd prefer to use a separate flag to
> indicate if the renderer support inline composition or not. So that when we
add
> more text input types, we don't need to add two variants for each one.

Done. Thanks for nice suggestion.

> And we need to improve the code in views/ime/* to support it as well.

Definitely.
However, I'd like to improve there in near future, not now in this (already
rather large) patch. I added a TODO comment in RenderWidgetHostViewViews,
the point where views/ime/* has a chance to be used in
PREEDIT_ON_CANDIDATE_WINDOW mode.

[kinaba, 2011-06-10 13:04:06]

On 2011/06/09 06:17:33, Takayoshi Kochi wrote:
> LGTM
> 
> How about adding Oshima-san as a reviewer as well?

Added Oshima-san as a reviewer. Thanks.

[oshima, 2011-06-10 17:41:49]

I'm probably not qualified to review this change as i'm not familiar with ime
code for renderer/plugins. style looks good though.

[James Su, 2011-06-13 01:31:10]

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

http://codereview.chromium.org/7041003/diff/4001/content/common/view_messages...
content/common/view_messages.h:1701: ui::TextInputPreeditType, /* preedit_type
*/
How about to use a bool flag instead?

http://codereview.chromium.org/7041003/diff/4001/ui/base/ime/text_input_type.h
File ui/base/ime/text_input_type.h (right):

http://codereview.chromium.org/7041003/diff/4001/ui/base/ime/text_input_type....
ui/base/ime/text_input_type.h:29: enum TextInputPreeditType {
We use term "composition" instead of "preedit" in chromium.
And I think a boolean flag is enough for indicating if inline composition is
supported or not.

[kinaba, 2011-06-13 08:08:05]

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

http://codereview.chromium.org/7041003/diff/4001/content/common/view_messages...
content/common/view_messages.h:1701: ui::TextInputPreeditType, /* preedit_type
*/
On 2011/06/13 01:31:10, James Su wrote:
> How about to use a bool flag instead?

Done.

http://codereview.chromium.org/7041003/diff/4001/ui/base/ime/text_input_type.h
File ui/base/ime/text_input_type.h (right):

http://codereview.chromium.org/7041003/diff/4001/ui/base/ime/text_input_type....
ui/base/ime/text_input_type.h:29: enum TextInputPreeditType {
On 2011/06/13 01:31:10, James Su wrote:
> We use term "composition" instead of "preedit" in chromium.
> And I think a boolean flag is enough for indicating if inline composition is
> supported or not.

Done (for all the other parts of this patch).

[James Su, 2011-06-13 09:58:20]

LGTM.

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

http://codereview.chromium.org/7041003/diff/4003/content/renderer/render_widg...
content/renderer/render_widget.h:284: // preedit text.
nit: // composition text.

[kinaba, 2011-06-13 11:01:31]

Thank you, James. I missed to check comments.

Brett, could you please take a look as an owner of the directory?

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

http://codereview.chromium.org/7041003/diff/4003/content/renderer/render_widg...
content/renderer/render_widget.h:284: // preedit text.
On 2011/06/13 09:58:20, James Su wrote:
> nit: // composition text.

Done.

[brettw, 2011-06-13 14:52:36]

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_vie...
content/renderer/render_view.cc:3995: } else {
Normally we prefer no "else" after a return statement, so you can just remove
the else (I realize you probably just copied the code above).

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_wid...
content/renderer/render_widget.cc:1302: return
static_cast<ui::TextInputType>(webwidget_->textInputType());
I'm worried about these things getting out-of-sync. We usually like to have
compiler asserts that the Chrome type is the same as the WebKit type. It would
be best to have this in the ui code, but it looks like that can't depend on
WebKit. So maybe we can just put them here?

[kinaba, 2011-06-13 17:48:16]

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_vie...
content/renderer/render_view.cc:3995: } else {
On 2011/06/13 14:52:36, brettw wrote:
> Normally we prefer no "else" after a return statement, so you can just remove
> the else (I realize you probably just copied the code above).

Done.

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_wid...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/7041003/diff/10004/content/renderer/render_wid...
content/renderer/render_widget.cc:1302: return
static_cast<ui::TextInputType>(webwidget_->textInputType());
On 2011/06/13 14:52:36, brettw wrote:
> I'm worried about these things getting out-of-sync. We usually like to have
> compiler asserts that the Chrome type is the same as the WebKit type. It would
> be best to have this in the ui code, but it looks like that can't depend on
> WebKit. So maybe we can just put them here?

Added a DCHECK for that we receive a known (to the ui:: code) input
type. But I'd like to know if there is a better way to express this.

[brettw, 2011-06-13 17:50:25]

http://codereview.chromium.org/7041003/diff/8004/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/7041003/diff/8004/content/renderer/render_widg...
content/renderer/render_widget.cc:1304: DCHECK(type <=
ui::TEXT_INPUT_TYPE_PASSWORD) <<
I was thinking of something more like the code at the bottom of this file:
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_pa...

[kinaba, 2011-06-13 18:08:03]

http://codereview.chromium.org/7041003/diff/8004/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/7041003/diff/8004/content/renderer/render_widg...
content/renderer/render_widget.cc:1304: DCHECK(type <=
ui::TEXT_INPUT_TYPE_PASSWORD) <<
On 2011/06/13 17:50:25, brettw wrote:
> I was thinking of something more like the code at the bottom of this file:
>
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_pa...

Done. Thank you, I didn't know this macro!

[kinaba, 2011-06-15 02:31:51]

Brett, do you think it is ok to commit now?

On 2011/06/13 18:08:03, kinaba wrote:
>
http://codereview.chromium.org/7041003/diff/8004/content/renderer/render_widg...
> File content/renderer/render_widget.cc (right):
> 
>
http://codereview.chromium.org/7041003/diff/8004/content/renderer/render_widg...
> content/renderer/render_widget.cc:1304: DCHECK(type <=
> ui::TEXT_INPUT_TYPE_PASSWORD) <<
> On 2011/06/13 17:50:25, brettw wrote:
> > I was thinking of something more like the code at the bottom of this file:
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/webkit/fileapi/file_system_pa...
> 
> Done. Thank you, I didn't know this macro!

[brettw, 2011-06-15 02:39:32]

The asserts look fine, I didn't check the rest.

[kochi, 2011-06-16 10:26:16]

On 2011/06/15 02:39:32, brettw wrote:
> The asserts look fine, I didn't check the rest.

Brett, then does this look good to you?
The OWNERS file is blocking from this to be committed.

[brettw, 2011-06-16 17:39:43]

LGTM

http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view...
content/renderer/render_view.cc:3992: // consider all the parts of PPAPI plugins
are accepting text inputs.
I don't quite follow this comment. To me, considering all the parts of PPAPI
plugins as accepting text inputs implies that inline composition would be
available. Can you reword this?

[kochi, 2011-06-17 03:47:21]

http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view...
content/renderer/render_view.cc:3992: // consider all the parts of PPAPI plugins
are accepting text inputs.
Maybe like this:

Until PPAPI has an interface for handling IME events, there is no way for the
browser to know whether the plugin is capable of drawing composition text.  We
assume plugins are incapable and let the browser handle composition display for
now.

[brettw, 2011-06-17 04:01:52]

On Thu, Jun 16, 2011 at 8:47 PM,  <kochi@chromium.org> wrote:
>
>
http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view.cc
> File content/renderer/render_view.cc (right):
>
>
http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view...
> content/renderer/render_view.cc:3992: // consider all the parts of PPAPI
> plugins are accepting text inputs.
> Maybe like this:
>
> Until PPAPI has an interface for handling IME events, there is no way
> for the browser to know whether the plugin is capable of drawing
> composition text.  We assume plugins are incapable and let the browser
> handle composition display for now.

Sounds great, thanks for putting that in.

Brett

[kinaba, 2011-06-17 04:12:25]

http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7041003/diff/3008/content/renderer/render_view...
content/renderer/render_view.cc:3992: // consider all the parts of PPAPI plugins
are accepting text inputs.
On 2011/06/17 03:47:21, Takayoshi Kochi wrote:
> Maybe like this:
> 
> Until PPAPI has an interface for handling IME events, there is no way for the
> browser to know whether the plugin is capable of drawing composition text.  We
> assume plugins are incapable and let the browser handle composition display
for
> now.

Done.

[commit-bot: I haz the power, 2011-06-17 07:59:48]

Change committed as 89451