Pepper IME API for surrounding text retrieval.

Pepper IME API for surrounding text retrieval.

IME benefits from knowing what portion of text is selected inside a text editing plugin.
This change is to implement a Pepper API for doing it. It consists of three API functions:

1. Browser asks plugins by PPP_TextInput_Dev::RequestSurroundingText() to send such info.
2. Plugin must reply the query by PPB_TextInput_Dev::UpdateSurroundingText().
3. Additionally, plugin notifies the browser by PPB_TextInput_Dev::SelectionChanged() that the selection is changed. Typically triggers the steps 1->2.

Intention of the API design is (1) to avoid synchronous IPC, and (2) to keep the room to implement it in an optimal and right way, that is, expensive send of selection text happens only when needed (= "IME requiring the info is on" + "selection indeed changed in the plugin"), though the current impl in the patch is not necessary like that (for sake of simplicity).

The changes in the API is in:
* ppapi/c/dev/ppb_text_input_dev.h
* ppapi/c/dev/ppp_text_input_dev.h

The browser side implementation mostly resides in:
* content/renderer/render_view_impl.cc
* content/renderer/pepper/pepper_plugin_delegate_impl.{h,cc}
* webkit/plugins/ppapi/ppapi_plugin_instance.{h,cc}

The other files are for wiring necessary cables.

BUG=101101
TEST=Manual: make ppapi_example_ime and run ./your/chrome --register-pepper-plugins=\ 
"/path/to/ppapi_example_ime.plugin;application/x-ppapi-example-ime" \
--ppapi-out-of-process \
file:///path/to/ppapi/examples/ime/ime.html 
Try some IME that supports reconversion (e.g., Google Japanese Input on Windows).

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

[kinaba, 2012-03-01 13:25:48]

Hello folks. This patch is the continuation of Pepper IME API work (e.g.,
http://codereview.chromium.org/8073021/). Could you kindly review it? Thank you
very much!

[kinaba, 2012-03-07 05:42:12]

On 2012/03/01 13:25:48, kinaba wrote:
> Hello folks. This patch is the continuation of Pepper IME API work (e.g.,
> http://codereview.chromium.org/8073021/). Could you kindly review it? Thank
you
> very much!

Ping: reviewers.

If you think the patch too big to review I can separate it to API definition
part and Chrome-side implementation part (though in fact the most of the changes
in the patchset is almost boilerplate code and hence the essential change is not
so large).

[kochi, 2012-03-07 08:43:02]

new interfaces definition LGTM.

http://codereview.chromium.org/8769003/diff/37001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

http://codereview.chromium.org/8769003/diff/37001/ppapi/ppapi_sources.gypi#ne...
ppapi/ppapi_sources.gypi:73: 'c/dev/ppb_text_input_dev.h',
nit: swap line 73 & 74 (sort)

[kinaba, 2012-03-07 09:24:03]

http://codereview.chromium.org/8769003/diff/37001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

http://codereview.chromium.org/8769003/diff/37001/ppapi/ppapi_sources.gypi#ne...
ppapi/ppapi_sources.gypi:73: 'c/dev/ppb_text_input_dev.h',
On 2012/03/07 08:43:02, Takayoshi Kochi wrote:
> nit: swap line 73 & 74 (sort)

Good catch. Done.

[yzshen1, 2012-03-07 18:22:55]

Only a few nits for the interface definition.

Will review and comment on the other files.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:74: * text. <code>text</code> is an UTF-8
string that contains the current range
nit: an -> a

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:82: * features of IME that puts back the
already commited text into the
nit: committed.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:85: * looks to be on the beggining of a
sentense, suggest capital letters in a
beggining -> begining

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:88: * When the focus is not on text, or
when the plugin do not want to send the
do -> does

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppp_text_inpu...
File ppapi/api/dev/ppp_text_input_dev.idl (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppp_text_inpu...
ppapi/api/dev/ppp_text_input_dev.idl:16: * plugin have to implement to provide
hints for text input system (IME).
nit: have -> has

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppp_text_inpu...
ppapi/api/dev/ppp_text_input_dev.idl:20: * Request the plugin to send back the
text arround the current caret or
nit: Request -> Requests

[yzshen1, 2012-03-08 07:51:43]

http://codereview.chromium.org/8769003/diff/36043/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8769003/diff/36043/content/renderer/render_vie...
content/renderer/render_view_impl.h:273: // Informs the render view that a PPAPI
plugin has changed selction.
selction -> selection

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
File ppapi/cpp/dev/text_input_dev.cc (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.cc:20: uint32_t desired_number_of_chanracetes) {
Is it "characters"? :)

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.cc:33: template <> const char*
interface_name<PPB_TextInput_Dev>() {
You should consider backward compatibility.

Using this cpp wrapper class, the plugin should be able to work with browsers
that only support the v0.1 interface. (We could remove the support later, after
we update all our users with v0.2 support.)

Please see net_address_private.cc as an example.

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_dev.h
File ppapi/cpp/dev/text_input_dev.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.h:8: #include <string>
A blank line below it, please.

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.h:24: //     class MyInstance() :
pp::TextInput_Dev(this) {
class should be removed.

http://codereview.chromium.org/8769003/diff/36043/ppapi/examples/ime/ime.cc
File ppapi/examples/ime/ime.cc (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/examples/ime/ime.cc#n...
ppapi/examples/ime/ime.cc:100: explicit
TextFieldStatusNotifyingHanlder(pp::Instance* instance)
Hanlder -> Handler, please.

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:909:
IPC_MESSAGE_ROUTED2(PpapiMsg_PPPTextInput_RequestSurroundingText,
Should be moved to the PpapiMsg* section.

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppb_text_input_...
File ppapi/proxy/ppb_text_input_proxy.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppb_text_input_...
ppapi/proxy/ppb_text_input_proxy.h:8: #include <string>
a blank line below it, please.

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppp_text_input_...
File ppapi/proxy/ppp_text_input_proxy.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppp_text_input_...
ppapi/proxy/ppp_text_input_proxy.h:9: #include
"ppapi/c/dev/ppp_text_input_dev.h"
Sort the list, please.

[James Su, 2012-03-08 17:27:21]

Sorry for late reply. Overall the CL looks good to me. Just one question needs
clarification: Seems that there is no code dealing with text input type, e.g. do
we need to avoid returning any surrounding text if the text input type is
password? Or is it the responsibility of the plugin itself?

[kinaba, 2012-03-13 15:29:27]

On 2012/03/08 17:27:21, James Su wrote:
> Sorry for late reply. Overall the CL looks good to me. Just one question needs
> clarification: Seems that there is no code dealing with text input type, e.g.
do
> we need to avoid returning any surrounding text if the text input type is
> password? Or is it the responsibility of the plugin itself?

Good point, I was not clear about that.
My plan is to make it the responsibility of the plugin.
(After all, plugins are free to hide surrounding text in other places than
password field if it is more appropriate, so the plugin should anyhow take care
what to send,
and the browser must accept such plugin-side choice anyhow.)

I'll add documentation about that. Thank you for confirming it.

[kinaba, 2012-03-14 04:28:53]

Updated.

http://codereview.chromium.org/8769003/diff/36043/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8769003/diff/36043/content/renderer/render_vie...
content/renderer/render_view_impl.h:273: // Informs the render view that a PPAPI
plugin has changed selction.
On 2012/03/08 07:51:43, yzshen1 wrote:
> selction -> selection

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:74: * text. <code>text</code> is an UTF-8
string that contains the current range
On 2012/03/07 18:22:56, yzshen1 wrote:
> nit: an -> a

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:82: * features of IME that puts back the
already commited text into the
On 2012/03/07 18:22:56, yzshen1 wrote:
> nit: committed.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:85: * looks to be on the beggining of a
sentense, suggest capital letters in a
On 2012/03/07 18:22:56, yzshen1 wrote:
> beggining -> begining

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppb_text_inpu...
ppapi/api/dev/ppb_text_input_dev.idl:88: * When the focus is not on text, or
when the plugin do not want to send the
On 2012/03/07 18:22:56, yzshen1 wrote:
> do -> does

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppp_text_inpu...
File ppapi/api/dev/ppp_text_input_dev.idl (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppp_text_inpu...
ppapi/api/dev/ppp_text_input_dev.idl:16: * plugin have to implement to provide
hints for text input system (IME).
On 2012/03/07 18:22:56, yzshen1 wrote:
> nit: have -> has

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/api/dev/ppp_text_inpu...
ppapi/api/dev/ppp_text_input_dev.idl:20: * Request the plugin to send back the
text arround the current caret or
On 2012/03/07 18:22:56, yzshen1 wrote:
> nit: Request -> Requests

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
File ppapi/cpp/dev/text_input_dev.cc (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.cc:20: uint32_t desired_number_of_chanracetes) {
On 2012/03/08 07:51:43, yzshen1 wrote:
> Is it "characters"? :)

Yes... what's happened to my keyboard?!!

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.cc:33: template <> const char*
interface_name<PPB_TextInput_Dev>() {
On 2012/03/08 07:51:43, yzshen1 wrote:
> You should consider backward compatibility.
> 
> Using this cpp wrapper class, the plugin should be able to work with browsers
> that only support the v0.1 interface. (We could remove the support later,
after
> we update all our users with v0.2 support.)
> 
> Please see net_address_private.cc as an example.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_dev.h
File ppapi/cpp/dev/text_input_dev.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.h:8: #include <string>
On 2012/03/08 07:51:43, yzshen1 wrote:
> A blank line below it, please.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.h:24: //     class MyInstance() :
pp::TextInput_Dev(this) {
On 2012/03/08 07:51:43, yzshen1 wrote:
> class should be removed.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/examples/ime/ime.cc
File ppapi/examples/ime/ime.cc (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/examples/ime/ime.cc#n...
ppapi/examples/ime/ime.cc:100: explicit
TextFieldStatusNotifyingHanlder(pp::Instance* instance)
On 2012/03/08 07:51:43, yzshen1 wrote:
> Hanlder -> Handler, please.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:909:
IPC_MESSAGE_ROUTED2(PpapiMsg_PPPTextInput_RequestSurroundingText,
On 2012/03/08 07:51:43, yzshen1 wrote:
> Should be moved to the PpapiMsg* section.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppb_text_input_...
File ppapi/proxy/ppb_text_input_proxy.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppb_text_input_...
ppapi/proxy/ppb_text_input_proxy.h:8: #include <string>
On 2012/03/08 07:51:43, yzshen1 wrote:
> a blank line below it, please.

Done.

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppp_text_input_...
File ppapi/proxy/ppp_text_input_proxy.h (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/proxy/ppp_text_input_...
ppapi/proxy/ppp_text_input_proxy.h:9: #include
"ppapi/c/dev/ppp_text_input_dev.h"
On 2012/03/08 07:51:43, yzshen1 wrote:
> Sort the list, please.

Done.

[James Su, 2012-03-14 04:44:42]

LGTM, as long as you are aware of this issue.

On 2012/03/13 15:29:27, kinaba wrote:
> On 2012/03/08 17:27:21, James Su wrote:
> > Sorry for late reply. Overall the CL looks good to me. Just one question
needs
> > clarification: Seems that there is no code dealing with text input type,
e.g.
> do
> > we need to avoid returning any surrounding text if the text input type is
> > password? Or is it the responsibility of the plugin itself?
> 
> Good point, I was not clear about that.
> My plan is to make it the responsibility of the plugin.
> (After all, plugins are free to hide surrounding text in other places than
> password field if it is more appropriate, so the plugin should anyhow take
care
> what to send,
> and the browser must accept such plugin-side choice anyhow.)
> 
> I'll add documentation about that. Thank you for confirming it.

[yzshen1, 2012-03-14 06:48:08]

LGTM

Thanks!

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
File ppapi/cpp/dev/text_input_dev.cc (right):

http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
ppapi/cpp/dev/text_input_dev.cc:20: uint32_t desired_number_of_chanracetes) {
I am pretty sure that we have the same kind of keyboard. :)

On 2012/03/14 04:28:54, kinaba wrote:
> On 2012/03/08 07:51:43, yzshen1 wrote:
> > Is it "characters"? :)
> 
> Yes... what's happened to my keyboard?!!

[kinaba, 2012-03-14 18:10:44]

Brett, could you take a look?

On 2012/03/14 06:48:08, yzshen1 wrote:
> LGTM
> 
> Thanks!
> 
>
http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
> File ppapi/cpp/dev/text_input_dev.cc (right):
> 
>
http://codereview.chromium.org/8769003/diff/36043/ppapi/cpp/dev/text_input_de...
> ppapi/cpp/dev/text_input_dev.cc:20: uint32_t desired_number_of_chanracetes) {
> I am pretty sure that we have the same kind of keyboard. :)
> 
> On 2012/03/14 04:28:54, kinaba wrote:
> > On 2012/03/08 07:51:43, yzshen1 wrote:
> > > Is it "characters"? :)
> > 
> > Yes... what's happened to my keyboard?!!

[brettw, 2012-03-14 19:07:24]

lgtm

[commit-bot: I haz the power, 2012-03-14 19:16:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/8769003/59025

[commit-bot: I haz the power, 2012-03-14 22:16:52]

Try job failure for 8769003-59025 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-14 22:27:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/8769003/59025

[commit-bot: I haz the power, 2012-03-15 00:37:27]

Try job failure for 8769003-59025 (retry) (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-15 02:11:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/8769003/59025

[commit-bot: I haz the power, 2012-03-15 04:19:15]

Try job failure for 8769003-59025 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-03-15 06:08:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/8769003/59025

[commit-bot: I haz the power, 2012-03-15 07:34:54]

Change committed as 126862