Declarations for Pepper IME API.

Declarations for Pepper IME API.

BUG=59425
TEST=Check that ppapi_tests compile.

This change list is the first part for adding IME support for PPAPI. For effectiveness of reviewing, I'll split the rather large change into three parts:
(*1) Header files declaring IME API.
(2) Boilerplate code for proxy & thunk stuff.
(3) Actual implementation in webkit/plugin/ppapi/* and content/renderer/*.

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

[kinaba, 2011-09-13 04:12:48]

Takayoshi, could you check the API is properly following your design doc?
Brett and Yuzhu, could you review the patch, in particular, if it is fitting
well in the framework of PPAPI?

Just for better understanding, uploaded the whole unsplit patch set at
http://codereview.chromium.org/7621010/.

Thank you!

[yzshen1, 2011-09-13 06:28:34]

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:7: * Implementation of the TextInput
interface.
It is not an "implementation". :)

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:16: * text input. Intentionally kept sync
with WebKit::WebTextInputType defined in:
Minor: kept sync with ->kept in sync with?

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:55: [in] PP_Rect boundingBox);
boundingBox -> bounding_box.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:57: * Force to commit the current
composition text (if any).
Minor: Force -> Forces.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:62: * Cancel the current composition (if
any).
Minor: Cancel -> Cancels.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl
File ppapi/api/ppb_input_event.idl (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:740: interface PPB_CompositionInputEvent {
It seems to me that the naming convention is:
PP_INPUTEVENT_CLASS_<some_class> <--> PPB_<some_class>InputEvent.
(For example, PP_INPUTEVENT_CLASS_MOUSE <--> PPB_MouseInputEvent.)
It might be good to follow this convention.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:766: * segments[2*i+1]. For events other than
COMPOSITION_UPDATE, the function
It might be good to add more comments about the segments array. According to the
design doc, it satisfies "startx < endx, and startx < startx+1, and all startx
and endx are between 0 and length of composition text. "

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:772: * @param[out] segments A pointer to an array
containing segment information.
If we decide to use a void* memory block like this, it is good to add comments
about : who is responsible for releasing the memory/how to release it, what is
the type of each element in the memory block (is it uint32_t?).

Moreover, it is probably more common to use void** instead of void* as an out
parameter for pointers.

To me, it seems better to have two methods instead of this GetSegments:
uint32_t GetSegmentNumber();
and
PP_Bool GetSegmentAt([in] uint32_t index, [out] uint32_t start, [out] uint32_t
end);

All these calls will be handled within the plugin process, so there won't be
much overhead.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:781: * GetTargetSegment() returns the number of
the current target segment of
Minor issue: number -> index?

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

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/dev/text_input_dev.h#...
ppapi/cpp/dev/text_input_dev.h:11: /// This file defines the API for controlling
event delivery of IME.
It seems this comment is not very accurate.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/dev/text_input_dev.h#...
ppapi/cpp/dev/text_input_dev.h:23: void UpdateCaretPosition(const Rect& caret,
const Rect& boundingBox);
boundingBox -> bounding_box.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.cc
File ppapi/cpp/input_event.cc (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.cc#newcod...
ppapi/cpp/input_event.cc:231: : InputEvent() {
Wrong indent.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.cc#newcod...
ppapi/cpp/input_event.cc:236: event.pp_resource())) {
Minor: I think it should be 4 spaces indent from "if".

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.h
File ppapi/cpp/input_event.h (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.h#newcode308
ppapi/cpp/input_event.h:308: /// Constructs an is_null() keyboard input event
object.
This is not a "keyboard" input event object.

http://codereview.chromium.org/7882004/diff/1/ppapi/ppapi_cpp.gypi
File ppapi/ppapi_cpp.gypi (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/ppapi_cpp.gypi#newcode70
ppapi/ppapi_cpp.gypi:70: 'c/dev/ppb_text_input_dev.h',
Please arrange the files alphabetically.

[kinaba, 2011-09-13 09:29:01]

Updated. Thank you for various comments!

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:7: * Implementation of the TextInput
interface.
On 2011/09/13 06:28:34, yzshen1 wrote:
> It is not an "implementation". :)

Done. :-)

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:16: * text input. Intentionally kept sync
with WebKit::WebTextInputType defined in:
On 2011/09/13 06:28:34, yzshen1 wrote:
> Minor: kept sync with ->kept in sync with?

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:55: [in] PP_Rect boundingBox);
On 2011/09/13 06:28:34, yzshen1 wrote:
> boundingBox -> bounding_box.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:57: * Force to commit the current
composition text (if any).
On 2011/09/13 06:28:34, yzshen1 wrote:
> Minor: Force -> Forces.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/dev/ppb_text_input_de...
ppapi/api/dev/ppb_text_input_dev.idl:62: * Cancel the current composition (if
any).
On 2011/09/13 06:28:34, yzshen1 wrote:
> Minor: Cancel -> Cancels.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl
File ppapi/api/ppb_input_event.idl (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:740: interface PPB_CompositionInputEvent {
On 2011/09/13 06:28:34, yzshen1 wrote:
> It seems to me that the naming convention is:
> PP_INPUTEVENT_CLASS_<some_class> <--> PPB_<some_class>InputEvent.
> (For example, PP_INPUTEVENT_CLASS_MOUSE <--> PPB_MouseInputEvent.)
> It might be good to follow this convention. 

Done. Changed the class name to PP_INPUTEVENT_CLASS_COMPOSITION.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:766: * segments[2*i+1]. For events other than
COMPOSITION_UPDATE, the function
On 2011/09/13 06:28:34, yzshen1 wrote:
> It might be good to add more comments about the segments array. According to
the
> design doc, it satisfies "startx < endx, and startx < startx+1, and all startx
> and endx are between 0 and length of composition text. "

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:772: * @param[out] segments A pointer to an array
containing segment information.
On 2011/09/13 06:28:34, yzshen1 wrote:
> uint32_t GetSegmentNumber();
> and
> PP_Bool GetSegmentAt([in] uint32_t index, [out] uint32_t start, [out] uint32_t
> end);

Done. Thanks, it looks much better!

http://codereview.chromium.org/7882004/diff/1/ppapi/api/ppb_input_event.idl#n...
ppapi/api/ppb_input_event.idl:781: * GetTargetSegment() returns the number of
the current target segment of
On 2011/09/13 06:28:34, yzshen1 wrote:
> Minor issue: number -> index?

Done.

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

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/dev/text_input_dev.h#...
ppapi/cpp/dev/text_input_dev.h:11: /// This file defines the API for controlling
event delivery of IME.
On 2011/09/13 06:28:34, yzshen1 wrote:
> It seems this comment is not very accurate.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/dev/text_input_dev.h#...
ppapi/cpp/dev/text_input_dev.h:23: void UpdateCaretPosition(const Rect& caret,
const Rect& boundingBox);
On 2011/09/13 06:28:34, yzshen1 wrote:
> boundingBox -> bounding_box.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.cc
File ppapi/cpp/input_event.cc (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.cc#newcod...
ppapi/cpp/input_event.cc:231: : InputEvent() {
On 2011/09/13 06:28:34, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.cc#newcod...
ppapi/cpp/input_event.cc:236: event.pp_resource())) {
On 2011/09/13 06:28:34, yzshen1 wrote:
> Minor: I think it should be 4 spaces indent from "if".

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.h
File ppapi/cpp/input_event.h (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/cpp/input_event.h#newcode308
ppapi/cpp/input_event.h:308: /// Constructs an is_null() keyboard input event
object.
On 2011/09/13 06:28:34, yzshen1 wrote:
> This is not a "keyboard" input event object.

Done.

http://codereview.chromium.org/7882004/diff/1/ppapi/ppapi_cpp.gypi
File ppapi/ppapi_cpp.gypi (right):

http://codereview.chromium.org/7882004/diff/1/ppapi/ppapi_cpp.gypi#newcode70
ppapi/ppapi_cpp.gypi:70: 'c/dev/ppb_text_input_dev.h',
On 2011/09/13 06:28:34, yzshen1 wrote:
> Please arrange the files alphabetically.

Done.

[brettw, 2011-09-13 22:36:04]

This looks like a great start. Some higher-level comments:

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:16: * text input. Intentionally kept in
sync with WebKit::WebTextInputType in:
We shouldn't be talking about the sync with webkit here. Once we publish this
API, we can't change it, and WebKit could diverge, so we can't stay up-to-date.

Instead, I just wouldn't comment here, and then in the webkit/plugins/ppapi
implementation, have a conversion function with compiler asserts that they're
the same values. Then if they diverge, we have one place to change to keep the
conversion up-to-date. See the webkit/plugins/ppapi/webkit_forwarding_impl.cc

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:41: interface PPB_TextInput_Dev {
I'm worried the very generic name "TextInput" might be confusing. This wold only
be used for IME, right? I can imagine seeing this and being confused that I need
to implement this for all types of text input.

Does it make sense to have "IME" in the name, like "IMETextInput"?

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.idl
File ppapi/api/ppb_input_event.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.id...
ppapi/api/ppb_input_event.idl:225: * Identifies composition input events.
Is IME not appropriate here? Sorry if I don't totally understand all the terms
here. I just want to be sure that things are named so that if a developer knyows
he doesn't care about IME, he knows to skip these things.

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.id...
ppapi/api/ppb_input_event.idl:740: interface PPB_CompositionInputEvent {
Can we put the APIs in the Dev directory (and name them accordingly) to start
off with? I imagine we'll be tweaking them a lot and it would be nice to not
have to worry about backwards compat as much.

You can keep the input event types in the enum above, I think.

[kochi, 2011-09-14 02:07:04]

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:41: interface PPB_TextInput_Dev {
IME generally accepted as CJK input methods, but e.g.
setting text field type is beyond the scope of IME, hence
we use TextInput as prefix for this.

On 2011/09/13 22:36:04, brettw wrote:
> I'm worried the very generic name "TextInput" might be confusing. This wold
only
> be used for IME, right? I can imagine seeing this and being confused that I
need
> to implement this for all types of text input.
> 
> Does it make sense to have "IME" in the name, like "IMETextInput"?

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.idl
File ppapi/api/ppb_input_event.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.id...
ppapi/api/ppb_input_event.idl:225: * Identifies composition input events.
I thinks this is in response to Yuzhu's comment about
naming convention (CLASS_* <=> *InputEvent)

I think IME is fine here, so what about changing the
event interface name to have 'IME' prefix?  This still
breaks the convention, but slightly better than not following
the convention at all.

On 2011/09/13 22:36:04, brettw wrote:
> Is IME not appropriate here? Sorry if I don't totally understand all the terms
> here. I just want to be sure that things are named so that if a developer
knyows
> he doesn't care about IME, he knows to skip these things.

[kinaba, 2011-09-14 08:54:04]

Updated. Thank you for useful feedbacks!

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:16: * text input. Intentionally kept in
sync with WebKit::WebTextInputType in:
On 2011/09/13 22:36:04, brettw wrote:
> We shouldn't be talking about the sync with webkit here. Once we publish this
> API, we can't change it, and WebKit could diverge, so we can't stay
up-to-date.
> 
> Instead, I just wouldn't comment here, and then in the webkit/plugins/ppapi
> implementation, have a conversion function with compiler asserts that they're
> the same values. Then if they diverge, we have one place to change to keep the
> conversion up-to-date. See the webkit/plugins/ppapi/webkit_forwarding_impl.cc

Done. In fact, the static asserts are already set up in my implementation and
hence simply removed the comment here.

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:41: interface PPB_TextInput_Dev {
On 2011/09/13 22:36:04, brettw wrote:
> I'm worried the very generic name "TextInput" might be confusing. This wold
only
> be used for IME, right? I can imagine seeing this and being confused that I
need
> to implement this for all types of text input.
> 
> Does it make sense to have "IME" in the name, like "IMETextInput"?

It is not limited to IME. For instance, touch UI relies on the text input type
and caret position to decide virtual keyboard pop-up. As opposed to the
IMEInputEvent that can somehow be ignored, "TextInput" is recommendable (though
not mandatory) for all types of text inputs.

I added several comments to make the purpose of this interface clearer so as not
to bring confusion...

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.idl
File ppapi/api/ppb_input_event.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.id...
ppapi/api/ppb_input_event.idl:225: * Identifies composition input events.
On 2011/09/14 02:07:05, Takayoshi Kochi wrote:
> I thinks this is in response to Yuzhu's comment about
> naming convention (CLASS_* <=> *InputEvent)
> 
> I think IME is fine here, so what about changing the
> event interface name to have 'IME' prefix?  This still
> breaks the convention, but slightly better than not following
> the convention at all.
> 
> On 2011/09/13 22:36:04, brettw wrote:
> > Is IME not appropriate here? Sorry if I don't totally understand all the
terms
> > here. I just want to be sure that things are named so that if a developer
> knyows
> > he doesn't care about IME, he knows to skip these things.
> 

I see. I turned the names to _CLASS_IME and IMEInputEvent.

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/ppb_input_event.id...
ppapi/api/ppb_input_event.idl:740: interface PPB_CompositionInputEvent {
On 2011/09/13 22:36:04, brettw wrote:
> Can we put the APIs in the Dev directory (and name them accordingly) to start
> off with? I imagine we'll be tweaking them a lot and it would be nice to not
> have to worry about backwards compat as much.
> 
> You can keep the input event types in the enum above, I think.

Done.

[yzshen1, 2011-09-14 17:32:56]

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:41: interface PPB_TextInput_Dev {
Is it true that Confirm/CancelCompositionText are IME-specific and supposed to
work with PP_INPUTEVENT_CLASS_IME; while other functions can be used by other
text inputs such as touch UI?

On 2011/09/14 08:54:04, kinaba wrote:
> On 2011/09/13 22:36:04, brettw wrote:
> > I'm worried the very generic name "TextInput" might be confusing. This wold
> only
> > be used for IME, right? I can imagine seeing this and being confused that I
> need
> > to implement this for all types of text input.
> > 
> > Does it make sense to have "IME" in the name, like "IMETextInput"?
> 
> It is not limited to IME. For instance, touch UI relies on the text input type
> and caret position to decide virtual keyboard pop-up. As opposed to the
> IMEInputEvent that can somehow be ignored, "TextInput" is recommendable
(though
> not mandatory) for all types of text inputs.
> 
> I added several comments to make the purpose of this interface clearer so as
not
> to bring confusion...

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:50: * segment in the composition text.
The positions are given by byte-indices of
byte -> char?

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:51: * the string GetText(). They
always satisfy 0 <= start < end <= (Length of
Just to double check: according to "end <= (Length of GetText())", the segment
is defined as [start, end) instead of [start, end], right?
It is better to comment about that explicitly.

I am not sure usually what the segments look like, but according to the
definition, overlapping is allowed:
[0, 3), [1, 3)
However, overlapping with the same starting position is not allowed:
Not allowed: [0, 2) [0, 3)
Is this intended?

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:53: * COMPOSITION_UPDATE or index >=
GetSegmentNumber(), nothing happens.
Is it better to return a PP_Bool?

http://codereview.chromium.org/7882004/diff/7010/ppapi/c/dev/ppb_ime_input_ev...
File ppapi/c/dev/ppb_ime_input_event_dev.h (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/c/dev/ppb_ime_input_ev...
ppapi/c/dev/ppb_ime_input_event_dev.h:17: #define
PPB_IME_INPUT_EVENT_INTERFACE_0_1 "PPB_IMEInputEvent(Dev);0.1"
You should have 'DEV' in the definition:
PPB_IME_INPUT_EVENT_DEV_INTERFACE_0_1
and
PPB_IME_INPUT_EVENT_DEV_INTERFACE
(Please also change the idl.)

[brettw, 2011-09-14 23:00:06]

LGTM from me at a high level, I'll let Yuzhu finish the review.

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:50: * segment in the composition text.
The positions are given by byte-indices of
On 2011/09/14 17:32:56, yzshen1 wrote:
> byte -> char?

We should be careful how this is defined. Is it Unicode characters or bytes?
Either way there should probably be some extra comment here explaining that it's
not the "other one."

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:45: interface PPB_TextInput_Dev {
Looks like you didn't regenerate this from the idl or you forgot to update the
CL with the resulting new file.

[kochi, 2011-09-15 07:27:18]

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:51: * the string GetText(). They
always satisfy 0 <= start < end <= (Length of
I just discussed this with Kazuhiro and removed the
'startx < startx+1' constraint from the doc.

The background is that information from IME is not
guaranteed and some IME may send
{ [2, 3), [0, 3), [1, 3), [1, 2) }
for example.

The browser may filter out such overlapping case, but
some IME *might* invent useful usage of overlapping
segments and we cannot prevent from it to happen.

So we defer the validation of the data to plugins.


On 2011/09/14 17:32:56, yzshen1 wrote:
> Just to double check: according to "end <= (Length of GetText())", the segment
> is defined as [start, end) instead of [start, end], right?
> It is better to comment about that explicitly.
> 
> I am not sure usually what the segments look like, but according to the
> definition, overlapping is allowed:
> [0, 3), [1, 3)
> However, overlapping with the same starting position is not allowed:
> Not allowed: [0, 2) [0, 3)
> Is this intended?

[kinaba, 2011-09-15 08:53:49]

Updated.

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:41: interface PPB_TextInput_Dev {
On 2011/09/14 17:32:56, yzshen1 wrote:
> Is it true that Confirm/CancelCompositionText are IME-specific and supposed to
> work with PP_INPUTEVENT_CLASS_IME;

Yes--they are IME-specific and very closely related to
PP_INPUTEVENT_CLASS_IME--but, no, they can be used without registering
PP_INPUTEVENT_CLASS_IME. A hypothetical example is a plugin that doesn't want to
bother managing composition text by itself (and hence don't register for
PP_INPUTEVENT_CLASS_IME), but wants to make sure the composition is reset at the
timing of, say, focus-change.

> while other functions can be used by other text inputs such as touch UI?

True.

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:50: * segment in the composition text.
The positions are given by byte-indices of
On 2011/09/14 23:00:06, brettw wrote:
> On 2011/09/14 17:32:56, yzshen1 wrote:
> > byte -> char?
> 
> We should be careful how this is defined. Is it Unicode characters or bytes?
> Either way there should probably be some extra comment here explaining that
it's
> not the "other one."

It is bytes, not Unicode characters. Improved the comment for clarification.
Thank you for good suggestion.

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:51: * the string GetText(). They
always satisfy 0 <= start < end <= (Length of
On 2011/09/15 07:27:19, Takayoshi Kochi wrote:
> I just discussed this with Kazuhiro and removed the
> 'startx < startx+1' constraint from the doc.
> 
> The background is that information from IME is not
> guaranteed and some IME may send
> { [2, 3), [0, 3), [1, 3), [1, 2) }
> for example.
> 
> The browser may filter out such overlapping case, but
> some IME *might* invent useful usage of overlapping
> segments and we cannot prevent from it to happen.
> 
> So we defer the validation of the data to plugins.
> 
> 
> On 2011/09/14 17:32:56, yzshen1 wrote:
> > Just to double check: according to "end <= (Length of GetText())", the
segment
> > is defined as [start, end) instead of [start, end], right?
> > It is better to comment about that explicitly.
> > 
> > I am not sure usually what the segments look like, but according to the
> > definition, overlapping is allowed:
> > [0, 3), [1, 3)
> > However, overlapping with the same starting position is not allowed:
> > Not allowed: [0, 2) [0, 3)
> > Is this intended?
> 

Added a comment saying that [start, end) is the one, instead of [start, end]. As
explained in Takayoshi's reply, we've dropped further constraints.

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:53: * COMPOSITION_UPDATE or index >=
GetSegmentNumber(), nothing happens.
On 2011/09/14 17:32:56, yzshen1 wrote:
> Is it better to return a PP_Bool?

Done.

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_text_input...
File ppapi/api/dev/ppb_text_input_dev.idl (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_text_input...
ppapi/api/dev/ppb_text_input_dev.idl:45: interface PPB_TextInput_Dev {
On 2011/09/14 23:00:06, brettw wrote:
> Looks like you didn't regenerate this from the idl or you forgot to update the
> CL with the resulting new file.

I'm sorry I didn't get the point. This is the latest IDL I intend to submit.
Please tell me if you notice that I forgot to update something.

http://codereview.chromium.org/7882004/diff/7010/ppapi/c/dev/ppb_ime_input_ev...
File ppapi/c/dev/ppb_ime_input_event_dev.h (right):

http://codereview.chromium.org/7882004/diff/7010/ppapi/c/dev/ppb_ime_input_ev...
ppapi/c/dev/ppb_ime_input_event_dev.h:17: #define
PPB_IME_INPUT_EVENT_INTERFACE_0_1 "PPB_IMEInputEvent(Dev);0.1"
On 2011/09/14 17:32:56, yzshen1 wrote:
> You should have 'DEV' in the definition:
> PPB_IME_INPUT_EVENT_DEV_INTERFACE_0_1
> and
> PPB_IME_INPUT_EVENT_DEV_INTERFACE
> (Please also change the idl.)

Done.

[yzshen1, 2011-09-15 16:59:54]

LGTM.

Thanks!

On 2011/09/15 08:53:49, kinaba wrote:
> Updated.
> 
>
http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
> File ppapi/api/dev/ppb_text_input_dev.idl (right):
> 
>
http://codereview.chromium.org/7882004/diff/9002/ppapi/api/dev/ppb_text_input...
> ppapi/api/dev/ppb_text_input_dev.idl:41: interface PPB_TextInput_Dev {
> On 2011/09/14 17:32:56, yzshen1 wrote:
> > Is it true that Confirm/CancelCompositionText are IME-specific and supposed
to
> > work with PP_INPUTEVENT_CLASS_IME;
> 
> Yes--they are IME-specific and very closely related to
> PP_INPUTEVENT_CLASS_IME--but, no, they can be used without registering
> PP_INPUTEVENT_CLASS_IME. A hypothetical example is a plugin that doesn't want
to
> bother managing composition text by itself (and hence don't register for
> PP_INPUTEVENT_CLASS_IME), but wants to make sure the composition is reset at
the
> timing of, say, focus-change.
> 
> > while other functions can be used by other text inputs such as touch UI?
> 
> True.
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
> File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
> ppapi/api/dev/ppb_ime_input_event_dev.idl:50: * segment in the composition
text.
> The positions are given by byte-indices of
> On 2011/09/14 23:00:06, brettw wrote:
> > On 2011/09/14 17:32:56, yzshen1 wrote:
> > > byte -> char?
> > 
> > We should be careful how this is defined. Is it Unicode characters or bytes?
> > Either way there should probably be some extra comment here explaining that
> it's
> > not the "other one."
> 
> It is bytes, not Unicode characters. Improved the comment for clarification.
> Thank you for good suggestion.
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
> ppapi/api/dev/ppb_ime_input_event_dev.idl:51: * the string GetText(). They
> always satisfy 0 <= start < end <= (Length of
> On 2011/09/15 07:27:19, Takayoshi Kochi wrote:
> > I just discussed this with Kazuhiro and removed the
> > 'startx < startx+1' constraint from the doc.
> > 
> > The background is that information from IME is not
> > guaranteed and some IME may send
> > { [2, 3), [0, 3), [1, 3), [1, 2) }
> > for example.
> > 
> > The browser may filter out such overlapping case, but
> > some IME *might* invent useful usage of overlapping
> > segments and we cannot prevent from it to happen.
> > 
> > So we defer the validation of the data to plugins.
> > 
> > 
> > On 2011/09/14 17:32:56, yzshen1 wrote:
> > > Just to double check: according to "end <= (Length of GetText())", the
> segment
> > > is defined as [start, end) instead of [start, end], right?
> > > It is better to comment about that explicitly.
> > > 
> > > I am not sure usually what the segments look like, but according to the
> > > definition, overlapping is allowed:
> > > [0, 3), [1, 3)
> > > However, overlapping with the same starting position is not allowed:
> > > Not allowed: [0, 2) [0, 3)
> > > Is this intended?
> > 
> 
> Added a comment saying that [start, end) is the one, instead of [start, end].
As
> explained in Takayoshi's reply, we've dropped further constraints.
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_ime_input_...
> ppapi/api/dev/ppb_ime_input_event_dev.idl:53: * COMPOSITION_UPDATE or index >=
> GetSegmentNumber(), nothing happens.
> On 2011/09/14 17:32:56, yzshen1 wrote:
> > Is it better to return a PP_Bool?
> 
> Done.
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_text_input...
> File ppapi/api/dev/ppb_text_input_dev.idl (right):
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/api/dev/ppb_text_input...
> ppapi/api/dev/ppb_text_input_dev.idl:45: interface PPB_TextInput_Dev {
> On 2011/09/14 23:00:06, brettw wrote:
> > Looks like you didn't regenerate this from the idl or you forgot to update
the
> > CL with the resulting new file.
> 
> I'm sorry I didn't get the point. This is the latest IDL I intend to submit.
> Please tell me if you notice that I forgot to update something.
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/c/dev/ppb_ime_input_ev...
> File ppapi/c/dev/ppb_ime_input_event_dev.h (right):
> 
>
http://codereview.chromium.org/7882004/diff/7010/ppapi/c/dev/ppb_ime_input_ev...
> ppapi/c/dev/ppb_ime_input_event_dev.h:17: #define
> PPB_IME_INPUT_EVENT_INTERFACE_0_1 "PPB_IMEInputEvent(Dev);0.1"
> On 2011/09/14 17:32:56, yzshen1 wrote:
> > You should have 'DEV' in the definition:
> > PPB_IME_INPUT_EVENT_DEV_INTERFACE_0_1
> > and
> > PPB_IME_INPUT_EVENT_DEV_INTERFACE
> > (Please also change the idl.)
> 
> Done.

[kochi, 2011-09-16 02:10:59]

LGTM

[James Su, 2011-09-16 06:39:53]

http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
ppapi/api/dev/ppb_ime_input_event_dev.idl:15: interface PPB_IMEInputEvent_Dev {
Is this event only for composition text or for both composition text and result
text?
If it's only for composition text, I'd suggest to have term Composition in the
name. Maybe IMECompositionEvent (if you'd prefer to keep term IME).

HTML DOM event specification has different events for composition and result
text, so it's probably good for us to  have different events as well.

http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
ppapi/api/dev/ppb_ime_input_event_dev.idl:72: [out] uint32_t end);
The segmentation information should only have a set of indexes, indicating the
segmentation boundary in the text. And the first and the last index should be 0
and text length.
Both windows and mac define segmentation information in this way.

Using [start,end) pairs is ok for underline information, but  semantically, it's
not suitable for describing segmentation information.

[kinaba, 2011-09-16 08:19:08]

On 2011/09/16 06:39:53, James Su wrote:
>
http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
> File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):
> 
>
http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
> ppapi/api/dev/ppb_ime_input_event_dev.idl:15: interface PPB_IMEInputEvent_Dev
{
> Is this event only for composition text or for both composition text and
result
> text?
> If it's only for composition text, I'd suggest to have term Composition in the
> name. Maybe IMECompositionEvent (if you'd prefer to keep term IME).
> 
> HTML DOM event specification has different events for composition and result
> text, so it's probably good for us to  have different events as well.
> 
>
http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
> ppapi/api/dev/ppb_ime_input_event_dev.idl:72: [out] uint32_t end);
> The segmentation information should only have a set of indexes, indicating the
> segmentation boundary in the text. And the first and the last index should be
0
> and text length.
> Both windows and mac define segmentation information in this way.
> 
> Using [start,end) pairs is ok for underline information, but  semantically,
it's
> not suitable for describing segmentation information.

Thank you for comments, James.
I'd like to catch it up next week by setting up another CL (since unfortunately
I've already closed this one).

As a quick response,
- I'm still inclined to keep the composition and result text events in the same
class. The reason is that, although the DOM textInput event is neutral to input
methods and used for all kinds of text inputs, our PP_INPUTEVENT_TYPE_IME_TEXT
is somewhat specific to IME, not to break the compatibility of existing
PP_INPUTEVENT_CHAR events.

- Thanks for letting me know that Windows and Mac have that sort of segmentation
info. Then it is reasonable to provide the segmentation information by a set of
indexes. I'll update the API in that direction.

[James Su, 2011-09-16 08:44:55]

On 2011/09/16 08:19:08, kinaba wrote:
> On 2011/09/16 06:39:53, James Su wrote:
> >
>
http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
> > File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):
> > 
> >
>
http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
> > ppapi/api/dev/ppb_ime_input_event_dev.idl:15: interface
PPB_IMEInputEvent_Dev
> {
> > Is this event only for composition text or for both composition text and
> result
> > text?
> > If it's only for composition text, I'd suggest to have term Composition in
the
> > name. Maybe IMECompositionEvent (if you'd prefer to keep term IME).
> > 
> > HTML DOM event specification has different events for composition and result
> > text, so it's probably good for us to  have different events as well.
> > 
> >
>
http://codereview.chromium.org/7882004/diff/10002/ppapi/api/dev/ppb_ime_input...
> > ppapi/api/dev/ppb_ime_input_event_dev.idl:72: [out] uint32_t end);
> > The segmentation information should only have a set of indexes, indicating
the
> > segmentation boundary in the text. And the first and the last index should
be
> 0
> > and text length.
> > Both windows and mac define segmentation information in this way.
> > 
> > Using [start,end) pairs is ok for underline information, but  semantically,
> it's
> > not suitable for describing segmentation information.
> 
> Thank you for comments, James.
> I'd like to catch it up next week by setting up another CL (since
unfortunately
> I've already closed this one).
> 
> As a quick response,
> - I'm still inclined to keep the composition and result text events in the
same
> class. The reason is that, although the DOM textInput event is neutral to
input
> methods and used for all kinds of text inputs, our PP_INPUTEVENT_TYPE_IME_TEXT
> is somewhat specific to IME, not to break the compatibility of existing
> PP_INPUTEVENT_CHAR events.
Sounds ok to me.

> 
> - Thanks for letting me know that Windows and Mac have that sort of
segmentation
> info. Then it is reasonable to provide the segmentation information by a set
of
> indexes. I'll update the API in that direction.
Thanks.

[James Su, 2011-09-16 08:45:28]

One more comment.

http://codereview.chromium.org/7882004/diff/10002/ppapi/api/ppb_input_event.idl
File ppapi/api/ppb_input_event.idl (right):

http://codereview.chromium.org/7882004/diff/10002/ppapi/api/ppb_input_event.i...
ppapi/api/ppb_input_event.idl:110: PP_INPUTEVENT_TYPE_COMPOSITION_START = 11,
I'm wondering if PP_INPUTEVENT_TYPE_IME_COMPOSITION_START is better? Considering
you are going to use the same event class for both composition and text events.