Additional update on Pepper IME API and boilerplate thunk/proxy implementation.

Additional update on Pepper IME API and boilerplate thunk/proxy implementation.

BUG=59425
TEST=Check that ppapi_tests compile.

This CL is the second (out of three) part for adding IME support for PPAPI.
It reflects comments from James Su to the previous CL:
http://codereview.chromium.org/7882004.
- Renamed ..._COMPOSTION_START to _IME_COMPOSITON_START.
- Changed to assure GetSegment to return a strictly increasing sequence of
  segmentation points from 0 to the length.
and,
- Added the mostly boilerplate code for interfacing with
  in-process & out-of-process plugins.
The actual implementation of the IME support will come as
the next and the last part of this series of patches.

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

[kinaba, 2011-09-21 04:33:35]

Just for illustrating purpose, I uploaded the unsplit full patch at
http://codereview.chromium.org/7977017/.

James, I included your suggestions on the previous CL to this one. Could you
review if the design is good enough?

Brett and Yuzhu, again, I'd like you in particular to check I'm not doing things
in a way different from PPAPI's way.

Thank you very much!

[kochi, 2011-09-22 10:48:47]

LGTM for my part (API itself).

[yzshen1, 2011-09-22 19:24:00]

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

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppapi_messages.h#ne...
ppapi/proxy/ppapi_messages.h:803: // PPB_TextInput.
nit: this section should be moved downwards (after PPB_Testing).

Is it necessary that these messages are sync messages?
I have a quick look at the CL "Never submit: tentative Pepper IME" and haven't
found the reason why they need to be sync (at least the first two). Would you
please explain why?

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppb_text_input_proxy.h
File ppapi/proxy/ppb_text_input_proxy.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppb_text_input_prox...
ppapi/proxy/ppb_text_input_proxy.h:8: #include
"ppapi/c/pp_completion_callback.h"
Please only include those that are necessary.
There are quite a few that can be removed.

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppb_text_input_prox...
ppapi/proxy/ppb_text_input_proxy.h:39: const PP_Rect& boundingBox) OVERRIDE;
Use bounding_box, please. (line 53 has the same problem.)

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
File ppapi/shared_impl/input_event_impl.cc (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
ppapi/shared_impl/input_event_impl.cc:28: composition_segments(),
nit: You don't need this.

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
File ppapi/shared_impl/input_event_impl.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
ppapi/shared_impl/input_event_impl.h:46: std::vector<uint32_t>
composition_segments;
Maybe something like composition_segment_offsets is more clear.

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_input_event_api.h
File ppapi/thunk/ppb_input_event_api.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_input_event_api...
ppapi/thunk/ppb_input_event_api.h:9: #include
"ppapi/c/dev/ppb_ime_input_event_dev.h"
Please order the includes.

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_text_input_api.h
File ppapi/thunk/ppb_text_input_api.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_text_input_api....
ppapi/thunk/ppb_text_input_api.h:22: const PP_Rect& boundingBox) = 0;
bounding_box

http://codereview.chromium.org/7978019/diff/1/webkit/plugins/ppapi/ppb_text_i...
File webkit/plugins/ppapi/ppb_text_input_impl.cc (right):

http://codereview.chromium.org/7978019/diff/1/webkit/plugins/ppapi/ppb_text_i...
webkit/plugins/ppapi/ppb_text_input_impl.cc:8: #include
"webkit/plugins/ppapi/ppapi_plugin_instance.h"
Please order the includes.

http://codereview.chromium.org/7978019/diff/1/webkit/plugins/ppapi/ppb_text_i...
webkit/plugins/ppapi/ppb_text_input_impl.cc:24: int(PP_TEXTINPUT_TYPE_NONE),
mismatching_enums);
4 space indent

[James Su, 2011-09-23 01:53:57]

Ahh, I gave my comments to http://codereview.chromium.org/7977017/.
If this CL same as that one?

[kochi, 2011-09-23 02:53:25]

On 2011/09/23 01:53:57, James Su wrote:
> Ahh, I gave my comments to http://codereview.chromium.org/7977017/.
> If this CL same as that one?

James, the one you commented is FYI CL, the complete one.
This CL is subset of the 7977017, so your comments are still valid.

[kinaba, 2011-09-27 02:49:27]

Updated. Thank you for comments.

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

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppapi_messages.h#ne...
ppapi/proxy/ppapi_messages.h:803: // PPB_TextInput.
On 2011/09/22 19:24:00, yzshen1 wrote:
> nit: this section should be moved downwards (after PPB_Testing).

Done.

> Is it necessary that these messages are sync messages?
> I have a quick look at the CL "Never submit: tentative Pepper IME" and haven't
> found the reason why they need to be sync (at least the first two). Would you
> please explain why?

Oh, yes, there's no reason for them to be sync.
Changed to async. Thanks.

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppb_text_input_proxy.h
File ppapi/proxy/ppb_text_input_proxy.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppb_text_input_prox...
ppapi/proxy/ppb_text_input_proxy.h:8: #include
"ppapi/c/pp_completion_callback.h"
On 2011/09/22 19:24:00, yzshen1 wrote:
> Please only include those that are necessary.
> There are quite a few that can be removed.

Done.

http://codereview.chromium.org/7978019/diff/1/ppapi/proxy/ppb_text_input_prox...
ppapi/proxy/ppb_text_input_proxy.h:39: const PP_Rect& boundingBox) OVERRIDE;
On 2011/09/22 19:24:00, yzshen1 wrote:
> Use bounding_box, please. (line 53 has the same problem.)

Done. Sorry for repeating the same mistake...

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
File ppapi/shared_impl/input_event_impl.cc (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
ppapi/shared_impl/input_event_impl.cc:28: composition_segments(),
On 2011/09/22 19:24:00, yzshen1 wrote:
> nit: You don't need this.

Done.

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
File ppapi/shared_impl/input_event_impl.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/shared_impl/input_event_i...
ppapi/shared_impl/input_event_impl.h:46: std::vector<uint32_t>
composition_segments;
On 2011/09/22 19:24:00, yzshen1 wrote:
> Maybe something like composition_segment_offsets is more clear.

Done.

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_input_event_api.h
File ppapi/thunk/ppb_input_event_api.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_input_event_api...
ppapi/thunk/ppb_input_event_api.h:9: #include
"ppapi/c/dev/ppb_ime_input_event_dev.h"
On 2011/09/22 19:24:00, yzshen1 wrote:
> Please order the includes.

Done.

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_text_input_api.h
File ppapi/thunk/ppb_text_input_api.h (right):

http://codereview.chromium.org/7978019/diff/1/ppapi/thunk/ppb_text_input_api....
ppapi/thunk/ppb_text_input_api.h:22: const PP_Rect& boundingBox) = 0;
On 2011/09/22 19:24:00, yzshen1 wrote:
> bounding_box

Done.

http://codereview.chromium.org/7978019/diff/1/webkit/plugins/ppapi/ppb_text_i...
File webkit/plugins/ppapi/ppb_text_input_impl.cc (right):

http://codereview.chromium.org/7978019/diff/1/webkit/plugins/ppapi/ppb_text_i...
webkit/plugins/ppapi/ppb_text_input_impl.cc:8: #include
"webkit/plugins/ppapi/ppapi_plugin_instance.h"
On 2011/09/22 19:24:00, yzshen1 wrote:
> Please order the includes.

Done.

http://codereview.chromium.org/7978019/diff/1/webkit/plugins/ppapi/ppb_text_i...
webkit/plugins/ppapi/ppb_text_input_impl.cc:24: int(PP_TEXTINPUT_TYPE_NONE),
mismatching_enums);
On 2011/09/22 19:24:00, yzshen1 wrote:
> 4 space indent
> 
> 

Done.

[James Su, 2011-09-27 04:23:59]

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

http://codereview.chromium.org/7978019/diff/9001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:896: PP_Instance /* instance */)
I'm now wondering if ConfirmCompositionText is feasible or not. AFAIK, only
Windows (IMM32) supports such operation, neither Mac nor Linux supports it.
On Windows IMM32, when an application sends such a request to the input method,
the input method should handle it and send back the result text to the
application synchronously.
Because an application usually sends this request before doing some operation
against the text content, so the content needs to be finalized before the
operation. If it happens asynchronously, then it'll be much complex for the
application to support it.

So in order to support it in pepper plugins, it should be a sync call from the
plugin process all the way up to the browser process, and a message containing
the result text must be sent back before the ACK message. It's apparently not
good for chrome's architecture.

Another solution is to fake a sync call by asking the browser to cancel the
composition asynchronously while confirming the current composition text as the
result text synchronously inside the plugin's process. So that we can avoid sync
RPC call completely. But then, I'd rather prefer to just remove this message and
let the plugin confirm the current composition text by itself and send an async
CancelCompositionText message explicitly.

[brettw, 2011-09-27 04:28:42]

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

http://codereview.chromium.org/7978019/diff/9001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:896: PP_Instance /* instance */)
If we can remove some complexity by not supporting this function, and still have
a good product, then let's do it. I don't really know how important this is to
the user experience. We can always add stuff later if we determine that it's
important.

[yzshen1, 2011-09-27 05:00:43]

LGTM
Thanks!
(Please make sure you get LGTMs from other reviewers before checking in the CL.)

[James Su, 2011-09-27 05:06:39]

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

http://codereview.chromium.org/7978019/diff/9001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:896: PP_Instance /* instance */)
On 2011/09/27 04:28:42, brettw wrote:
> If we can remove some complexity by not supporting this function, and still
have
> a good product, then let's do it. I don't really know how important this is to
> the user experience. We can always add stuff later if we determine that it's
> important.

I think this message is not that important. Chrome does not support such feature
on all platforms, no user complaint so far.

[kinaba, 2011-09-27 05:18:00]

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

http://codereview.chromium.org/7978019/diff/9001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:896: PP_Instance /* instance */)
I see. Let us drop it from the API (I've also discussed with Takayoshi offline
and got agreed). Thanks for the detailed explanation.

I'll upload the change soon.

On 2011/09/27 05:06:39, James Su wrote:
> On 2011/09/27 04:28:42, brettw wrote:
> > If we can remove some complexity by not supporting this function, and still
> have
> > a good product, then let's do it. I don't really know how important this is
to
> > the user experience. We can always add stuff later if we determine that it's
> > important.
> 
> I think this message is not that important. Chrome does not support such
feature
> on all platforms, no user complaint so far.

[brettw, 2011-09-27 05:22:28]

LGTM, looks great.

[James Su, 2011-09-27 05:55:33]

LGTM.

[kinaba, 2011-09-27 05:55:36]

Done. Removed the interface of composition confirmation.

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

http://codereview.chromium.org/7978019/diff/9001/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:896: PP_Instance /* instance */)
On 2011/09/27 05:06:39, James Su wrote:
> On 2011/09/27 04:28:42, brettw wrote:
> > If we can remove some complexity by not supporting this function, and still
> have
> > a good product, then let's do it. I don't really know how important this is
to
> > the user experience. We can always add stuff later if we determine that it's
> > important.
> 
> I think this message is not that important. Chrome does not support such
feature
> on all platforms, no user complaint so far.

Done.