PPAPI: Move IMEInputEvent and TextInput to stable.

PPAPI: Move IMEInputEvent and TextInput to stable.

To be able to work with current ppapi Flash, this CL keeps ABI of "PPB_IMEInputEvent_Dev_0_2", "PPB_TextInput_Dev_0_2" and "PPP_TextInput_Dev_0_2".

BUG=None
TEST=Manually checked that ppapi_example_ime works and also works with current Flash.
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214878

[dmichael (off chromium), 2013-07-16 22:42:06]

I need to look in more depth. The big thing I'm concerned about is I don't see
how backwards compatibility works. I'm also not certain whether you need to keep
0.1 support.

https://codereview.chromium.org/18671004/diff/25001/ppapi/api/ppb_ime_input_e...
File ppapi/api/ppb_ime_input_event.idl (right):

https://codereview.chromium.org/18671004/diff/25001/ppapi/api/ppb_ime_input_e...
ppapi/api/ppb_ime_input_event.idl:85: * GetSegmentNumber() returns the number of
segments in the composition text.
I think GetSegmentCount might be a more descriptive name.

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
File ppapi/c/ppb_ime_input_event.h (left):

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
ppapi/c/ppb_ime_input_event.h:161: void (*GetSelection)(PP_Resource ime_event,
uint32_t* start, uint32_t* end);
You might need to check with yuzhu or raymes to see if it's okay to get rid of
the 0.1 version. It depends on whether a significant enough number of users
might have an older Flash that uses it.

https://codereview.chromium.org/18671004/diff/25001/ppapi/thunk/interfaces_pp...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://codereview.chromium.org/18671004/diff/25001/ppapi/thunk/interfaces_pp...
ppapi/thunk/interfaces_ppb_public_dev.h:28: PPB_IMEInputEvent_Dev_0_2)
It appears that you've deleted the file that defines this (as well as
PPB_TextInput_Dev_0_2). Am I missing something? Are you sure it works properly?

[Seigo Nonaka, 2013-07-17 06:10:39]

Thank you for your review.

Adding Yuzhu for getting advise for usage of 0.1 version.
IMO, the Flash has been using 0.2 for 1.5 years, so I think we can just ignore
the backward compatibility for 0.1.

cc-ing kinaba@ who is the author of Flash pepper IME support, just in case.

Thank you.

https://codereview.chromium.org/18671004/diff/25001/ppapi/api/ppb_ime_input_e...
File ppapi/api/ppb_ime_input_event.idl (right):

https://codereview.chromium.org/18671004/diff/25001/ppapi/api/ppb_ime_input_e...
ppapi/api/ppb_ime_input_event.idl:85: * GetSegmentNumber() returns the number of
segments in the composition text.
On 2013/07/16 22:42:07, dmichael wrote:
> I think GetSegmentCount might be a more descriptive name.

Done.

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
File ppapi/c/ppb_ime_input_event.h (left):

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
ppapi/c/ppb_ime_input_event.h:161: void (*GetSelection)(PP_Resource ime_event,
uint32_t* start, uint32_t* end);
Sure, adding yuzhu@ as the reviewer.

yuzhu: AFAIK, we only support specific version of Flash and Chrome for each
milestone. And Flash has been working with 0.2 version in 1.5 year. So seems
safe to drop 0.1 version.

On 2013/07/16 22:42:07, dmichael wrote:
> You might need to check with yuzhu or raymes to see if it's okay to get rid of
> the 0.1 version. It depends on whether a significant enough number of users
> might have an older Flash that uses it.

https://codereview.chromium.org/18671004/diff/25001/ppapi/thunk/interfaces_pp...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://codereview.chromium.org/18671004/diff/25001/ppapi/thunk/interfaces_pp...
ppapi/thunk/interfaces_ppb_public_dev.h:28: PPB_IMEInputEvent_Dev_0_2)
Yes, this works with current Flash properly because I leave
GetPPB_IMEInputEvent_Dev_0_2_Thunk function with same struct.
The struct was generated by IDL generator but now it is gone, so I introduce
Dev_0.2 struct by myself and fill 1.0 functions for compatibility.
This approach can be used so far as the function type signatures are kept
between Dev_0.2 and 1.0.

On 2013/07/16 22:42:07, dmichael wrote:
> It appears that you've deleted the file that defines this (as well as
> PPB_TextInput_Dev_0_2). Am I missing something? Are you sure it works
properly?

[dmichael (off chromium), 2013-07-18 03:50:20]

https://codereview.chromium.org/18671004/diff/34001/ppapi/api/ppb_text_input.idl
File ppapi/api/ppb_text_input.idl (right):

https://codereview.chromium.org/18671004/diff/34001/ppapi/api/ppb_text_input....
ppapi/api/ppb_text_input.idl:107: void SelectionChanged([in] PP_Instance
instance);
As Darin mentioned, it would be better to use a PP_CompletionCallback for this.
You can also add an out-param if necessary.

https://codereview.chromium.org/18671004/diff/34001/ppapi/thunk/ppb_input_eve...
File ppapi/thunk/ppb_input_event_thunk.cc (right):

https://codereview.chromium.org/18671004/diff/34001/ppapi/thunk/ppb_input_eve...
ppapi/thunk/ppb_input_event_thunk.cc:14: struct PPB_IMEInputEvent_Dev_0_2 {
I'd really prefer if we just kept the old IDL around and used this there. Is
there a reason not to do it the same we we do all our other backwards
compatibility?

(It's also possible that the non-dev version will change in the course of taking
this out of Dev, so doing it the usual way is more resilient to that)

[yzshen1, 2013-07-22 18:56:33]

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
File ppapi/c/ppb_ime_input_event.h (left):

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
ppapi/c/ppb_ime_input_event.h:161: void (*GetSelection)(PP_Resource ime_event,
uint32_t* start, uint32_t* end);
Sorry, I didn't realize there is a question for me earlier.
(I was on vacation for quite a while so my inbox is full.)

Yes, we ship Pepper Flash along with Chrome, so it is unlikely that users need
to run a recent Chrome build with some really old Pepper Flash.

If we haven't use v0.1 for 1.5 years, it should be safe to remove.
However, we should still keep v0.2 as long as Flash needs it.


On 2013/07/17 06:10:39, Seigo Nonaka wrote:
> Sure, adding yuzhu@ as the reviewer.
> 
> yuzhu: AFAIK, we only support specific version of Flash and Chrome for each
> milestone. And Flash has been working with 0.2 version in 1.5 year. So seems
> safe to drop 0.1 version.
> 
> On 2013/07/16 22:42:07, dmichael wrote:
> > You might need to check with yuzhu or raymes to see if it's okay to get rid
of
> > the 0.1 version. It depends on whether a significant enough number of users
> > might have an older Flash that uses it.
>

[Seigo Nonaka, 2013-07-23 11:49:54]

Thank you for your review!
I updated the CL to meet new interfaces.
Please take an another look?

Thank you.

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
File ppapi/c/ppb_ime_input_event.h (left):

https://codereview.chromium.org/18671004/diff/25001/ppapi/c/ppb_ime_input_eve...
ppapi/c/ppb_ime_input_event.h:161: void (*GetSelection)(PP_Resource ime_event,
uint32_t* start, uint32_t* end);
Sure, at this moment I keep old IDL including both 0.2 and 0.1.
After this change, I will request (or I will do) update Flash to use 1.0
version.
Then let's remove old interface after the Flash works well on 1.0.

On 2013/07/22 18:56:33, yzshen1 wrote:
> Sorry, I didn't realize there is a question for me earlier.
> (I was on vacation for quite a while so my inbox is full.)
> 
> Yes, we ship Pepper Flash along with Chrome, so it is unlikely that users need
> to run a recent Chrome build with some really old Pepper Flash.
> 
> If we haven't use v0.1 for 1.5 years, it should be safe to remove.
> However, we should still keep v0.2 as long as Flash needs it.
> 
> 
> On 2013/07/17 06:10:39, Seigo Nonaka wrote:
> > Sure, adding yuzhu@ as the reviewer.
> > 
> > yuzhu: AFAIK, we only support specific version of Flash and Chrome for each
> > milestone. And Flash has been working with 0.2 version in 1.5 year. So seems
> > safe to drop 0.1 version.
> > 
> > On 2013/07/16 22:42:07, dmichael wrote:
> > > You might need to check with yuzhu or raymes to see if it's okay to get
rid
> of
> > > the 0.1 version. It depends on whether a significant enough number of
users
> > > might have an older Flash that uses it.
> > 
>

https://codereview.chromium.org/18671004/diff/34001/ppapi/api/ppb_text_input.idl
File ppapi/api/ppb_text_input.idl (right):

https://codereview.chromium.org/18671004/diff/34001/ppapi/api/ppb_text_input....
ppapi/api/ppb_text_input.idl:107: void SelectionChanged([in] PP_Instance
instance);
I looked into the details of this function and it turned out that this function
is no longer necessary.
So just removing for stable is fine.
Thank you for your comment :)

On 2013/07/18 03:50:20, dmichael wrote:
> As Darin mentioned, it would be better to use a PP_CompletionCallback for
this.
> You can also add an out-param if necessary.

https://codereview.chromium.org/18671004/diff/34001/ppapi/thunk/ppb_input_eve...
File ppapi/thunk/ppb_input_event_thunk.cc (right):

https://codereview.chromium.org/18671004/diff/34001/ppapi/thunk/ppb_input_eve...
ppapi/thunk/ppb_input_event_thunk.cc:14: struct PPB_IMEInputEvent_Dev_0_2 {
Sure, I revert dev IDLs.

On 2013/07/18 03:50:20, dmichael wrote:
> I'd really prefer if we just kept the old IDL around and used this there. Is
> there a reason not to do it the same we we do all our other backwards
> compatibility?
> 
> (It's also possible that the non-dev version will change in the course of
taking
> this out of Dev, so doing it the usual way is more resilient to that)

[yzshen1, 2013-07-23 16:48:19]

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi/library.dsc (right):

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi/library.dsc:106:
'ppb_text_input_controller.h',
Is it intended to put it in dev?
I thought you are going to put it in the stable folder?

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_cpp/library.dsc (right):

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_cpp/library.dsc:71:
'text_input_controller.cc',
If it is intended to put it in dev, the file/class name need to have _dev
suffix.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/input_event.h
File ppapi/cpp/input_event.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/input_event.h#n...
ppapi/cpp/input_event.h:379: PP_InputEvent_Type type,
wrong indent.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
File ppapi/cpp/text_input_controller.cc (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:7: #include
"ppapi/c/ppb_text_input_controller.h"
it is not needed because the .h file has included it.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:8: #include "ppapi/cpp/instance.h"
It isn't needed once we change the constructor to take InstanceHandle as
parameter.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:9: #include "ppapi/cpp/instance_handle.h"
ditto.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:39: const Rect& bounding_box) {
wrong indent.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:54: uint32_t caret,
wrong indent.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
File ppapi/cpp/text_input_controller.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.h:18: // This class allows you to associate the
PPP_TextInputController and
There isn't PPP_TextInputController, right?
I think we can remove all comments. It shouldn't be used as a base class
anymore.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.h:43: explicit TextInputController(Instance*
instance);
I think usually cpp class constructors take const InstanceHandle&

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.h:44: virtual ~TextInputController();
Please remove virtual.

https://codereview.chromium.org/18671004/diff/43001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/ppapi_sources.gypi#...
ppapi/ppapi_sources.gypi:209: 'cpp/text_input_controller.cc',
alphabetically, please.

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/interfaces_pp...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/interfaces_pp...
ppapi/thunk/interfaces_ppb_public_stable.h:63: PROXIED_IFACE(NoAPIName,
PPB_IME_INPUT_EVENT_INTERFACE_1_0,
Please order the list based on the second parameter, alphabetically.

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/ppb_instance_...
File ppapi/thunk/ppb_instance_api.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/ppb_instance_...
ppapi/thunk/ppb_instance_api.h:20: #include
"ppapi/c/ppb_text_input_controller.h"
nit: I think this is not needed.

[Seigo Nonaka, 2013-07-24 09:05:36]

Thank you for your review!
PTAL?

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi/library.dsc (right):

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi/library.dsc:106:
'ppb_text_input_controller.h',
On 2013/07/23 16:48:20, yzshen1 wrote:
> Is it intended to put it in dev?
> I thought you are going to put it in the stable folder?

Yes, thanks!
Done.

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi_cpp/library.dsc (right):

https://codereview.chromium.org/18671004/diff/43001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi_cpp/library.dsc:71:
'text_input_controller.cc',
On 2013/07/23 16:48:20, yzshen1 wrote:
> If it is intended to put it in dev, the file/class name need to have _dev
> suffix.

Sorry I'd like to ship as stable. Moving to stable location.
Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/input_event.h
File ppapi/cpp/input_event.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/input_event.h#n...
ppapi/cpp/input_event.h:379: PP_InputEvent_Type type,
On 2013/07/23 16:48:20, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
File ppapi/cpp/text_input_controller.cc (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:7: #include
"ppapi/c/ppb_text_input_controller.h"
On 2013/07/23 16:48:20, yzshen1 wrote:
> it is not needed because the .h file has included it.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:8: #include "ppapi/cpp/instance.h"
On 2013/07/23 16:48:20, yzshen1 wrote:
> It isn't needed once we change the constructor to take InstanceHandle as
> parameter.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:9: #include "ppapi/cpp/instance_handle.h"
On 2013/07/23 16:48:20, yzshen1 wrote:
> ditto.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:39: const Rect& bounding_box) {
On 2013/07/23 16:48:20, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.cc:54: uint32_t caret,
On 2013/07/23 16:48:20, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
File ppapi/cpp/text_input_controller.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.h:18: // This class allows you to associate the
PPP_TextInputController and
On 2013/07/23 16:48:20, yzshen1 wrote:
> There isn't PPP_TextInputController, right?
> I think we can remove all comments. It shouldn't be used as a base class
> anymore.

Yes! Thanks.
Removed.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.h:43: explicit TextInputController(Instance*
instance);
On 2013/07/23 16:48:20, yzshen1 wrote:
> I think usually cpp class constructors take const InstanceHandle&

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/cpp/text_input_cont...
ppapi/cpp/text_input_controller.h:44: virtual ~TextInputController();
On 2013/07/23 16:48:20, yzshen1 wrote:
> Please remove virtual.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/ppapi_sources.gypi#...
ppapi/ppapi_sources.gypi:209: 'cpp/text_input_controller.cc',
On 2013/07/23 16:48:20, yzshen1 wrote:
> alphabetically, please.

Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/interfaces_pp...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/interfaces_pp...
ppapi/thunk/interfaces_ppb_public_stable.h:63: PROXIED_IFACE(NoAPIName,
PPB_IME_INPUT_EVENT_INTERFACE_1_0,
On 2013/07/23 16:48:20, yzshen1 wrote:
> Please order the list based on the second parameter, alphabetically.

Sure, Done.

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/ppb_instance_...
File ppapi/thunk/ppb_instance_api.h (right):

https://codereview.chromium.org/18671004/diff/43001/ppapi/thunk/ppb_instance_...
ppapi/thunk/ppb_instance_api.h:20: #include
"ppapi/c/ppb_text_input_controller.h"
On 2013/07/23 16:48:20, yzshen1 wrote:
> nit: I think this is not needed.

Yes! removed.

[yzshen1, 2013-07-24 16:49:16]

lgtm

https://codereview.chromium.org/18671004/diff/69001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi/library.dsc (right):

https://codereview.chromium.org/18671004/diff/69001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi/library.dsc:77:
'ppb_text_input_controller.h',
alphabetically, please.

[Seigo Nonaka, 2013-07-25 04:36:55]

yzshen: Thank you for your review!

Dave, PTAL?

And also adding Darin as the reviewer.

Hi Darin,

I addressed your comments in
https://groups.google.com/a/chromium.org/forum/#!topic/pepper-dev/3nIJ40qwZf8
(Did nothing for PP_CompletionCallback because it turned out that the function
can be removed)

Could you check and take an another look?

Thank you.

https://codereview.chromium.org/18671004/diff/69001/native_client_sdk/src/lib...
File native_client_sdk/src/libraries/ppapi/library.dsc (right):

https://codereview.chromium.org/18671004/diff/69001/native_client_sdk/src/lib...
native_client_sdk/src/libraries/ppapi/library.dsc:77:
'ppb_text_input_controller.h',
On 2013/07/24 16:49:16, yzshen1 wrote:
> alphabetically, please.

Done.

[darin (slow to review), 2013-07-25 05:03:20]

On 2013/07/25 04:36:55, Seigo Nonaka wrote:
> yzshen: Thank you for your review!
> 
> Dave, PTAL?
> 
> And also adding Darin as the reviewer.
> 
> Hi Darin,
> 
> I addressed your comments in
>
https://groups.google.com/a/chromium.org/forum/#%21topic/pepper-dev/3nIJ40qwZf8
> (Did nothing for PP_CompletionCallback because it turned out that the function
> can be removed)

Sounds great, thanks for exploring that!


> 
> Could you check and take an another look?
> 
> Thank you.
> 
>
https://codereview.chromium.org/18671004/diff/69001/native_client_sdk/src/lib...
> File native_client_sdk/src/libraries/ppapi/library.dsc (right):
> 
>
https://codereview.chromium.org/18671004/diff/69001/native_client_sdk/src/lib...
> native_client_sdk/src/libraries/ppapi/library.dsc:77:
> 'ppb_text_input_controller.h',
> On 2013/07/24 16:49:16, yzshen1 wrote:
> > alphabetically, please.
> 
> Done.

[Seigo Nonaka, 2013-07-25 16:22:52]

Hi reviewers,

thank you for your reviews.

I'd like to update one small things.

I heard from kinaba@ about http://crbug.com/133472: there is an used argument in
current Dev spec.
It is *boundary_rect* in UpdateCaretPosition.
This argument was initially introduced for supporting 2nd argument of
ViewHostMsg_ImeCompositionRangeChanged.
However current spec is not correct or not enough to support this IPC.

This argument is eventually used for nicer looking of input method but not a
mandatory features.
So I think we should drop it for stable version now and it is nice to revisit
later when it is necessary.

Thank you.

[darin (slow to review), 2013-07-25 16:46:35]

On Thu, Jul 25, 2013 at 9:22 AM, <nona@chromium.org> wrote:

> Hi reviewers,
>
> thank you for your reviews.
>
> I'd like to update one small things.
>
> I heard from kinaba@ about http://crbug.com/133472: there is an used
> argument in
> current Dev spec.
> It is *boundary_rect* in UpdateCaretPosition.
> This argument was initially introduced for supporting 2nd argument of
> ViewHostMsg_**ImeCompositionRangeChanged.
> However current spec is not correct or not enough to support this IPC.
>
> This argument is eventually used for nicer looking of input method but not
> a
> mandatory features.
> So I think we should drop it for stable version now and it is nice to
> revisit
> later when it is necessary.
>
>
Sounds good.



> Thank you.
>
>
https://codereview.chromium.**org/18671004/<https://codereview.chromium.org/1...
>

[dmichael (off chromium), 2013-07-25 22:08:14]

https://codereview.chromium.org/18671004/diff/127001/ppapi/api/ppb_text_input...
File ppapi/api/ppb_text_input_controller.idl (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/api/ppb_text_input...
ppapi/api/ppb_text_input_controller.idl:92: [in] str_t text,
This should probably be PP_Var to ensure that it is UTF-8 validated. That's what
we usually use for UTF-8 string params.

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.cc
File ppapi/cpp/input_event.cc (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.cc...
ppapi/cpp/input_event.cc:296: event.pp_resource())) {
nit: I think this should be indented 4 more spaces

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.cc...
ppapi/cpp/input_event.cc:300: if (is_ime_event) {
Why do this instead of just putting the AddRefResource and
PassRefFromConstructor in the above if?

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.h
File ppapi/cpp/input_event.h (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.h#...
ppapi/cpp/input_event.h:426: std::pair<uint32_t, uint32_t> GetSelection() const;
We don't return pair anywhere else, and it forces the caller to use a pair even
if they don't want to. I'd rather just use two out-params like the C API.

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
File ppapi/cpp/text_input_controller.h (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
nit: 2013 on new files

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:18: class TextInputController {
Please document this class with appropriate doxygen style (possibly in a follow
up CL)

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:27: uint32_t caret, uint32_t anchor);
nit: in declarations, put all the arguments on their own line if they don't fit
in 1

[Seigo Nonaka, 2013-07-26 17:40:41]

Thank you for your review.
PTAL?

https://codereview.chromium.org/18671004/diff/127001/ppapi/api/ppb_text_input...
File ppapi/api/ppb_text_input_controller.idl (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/api/ppb_text_input...
ppapi/api/ppb_text_input_controller.idl:92: [in] str_t text,
On 2013/07/25 22:08:14, dmichael wrote:
> This should probably be PP_Var to ensure that it is UTF-8 validated. That's
what
> we usually use for UTF-8 string params.

Done.

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.cc
File ppapi/cpp/input_event.cc (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.cc...
ppapi/cpp/input_event.cc:296: event.pp_resource())) {
On 2013/07/25 22:08:14, dmichael wrote:
> nit: I think this should be indented 4 more spaces

Done.

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.cc...
ppapi/cpp/input_event.cc:300: if (is_ime_event) {
Sorry this weird code was introduced when I move from dev code.
Done.
On 2013/07/25 22:08:14, dmichael wrote:
> Why do this instead of just putting the AddRefResource and
> PassRefFromConstructor in the above if?

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.h
File ppapi/cpp/input_event.h (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/input_event.h#...
ppapi/cpp/input_event.h:426: std::pair<uint32_t, uint32_t> GetSelection() const;
On 2013/07/25 22:08:14, dmichael wrote:
> We don't return pair anywhere else, and it forces the caller to use a pair
even
> if they don't want to. I'd rather just use two out-params like the C API.

Done.

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
File ppapi/cpp/text_input_controller.h (right):

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/07/25 22:08:14, dmichael wrote:
> nit: 2013 on new files

Done.

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:18: class TextInputController {
Sure, roughly added in this CL and will follow up in next cl with some example
codes.
On 2013/07/25 22:08:14, dmichael wrote:
> Please document this class with appropriate doxygen style (possibly in a
follow
> up CL)

https://codereview.chromium.org/18671004/diff/127001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:27: uint32_t caret, uint32_t anchor);
On 2013/07/25 22:08:14, dmichael wrote:
> nit: in declarations, put all the arguments on their own line if they don't
fit
> in 1

Done.

[dmichael (off chromium), 2013-07-26 17:59:37]

lgtm with the C++ change I noted. Thanks!

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
File ppapi/cpp/text_input_controller.h (right):

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:25: /// A constructor for creating a
<code>TextInputController</code>
nit: period to end the sentence

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:46: /// CancelCompositionText() informs the
browser about the current composition
s/about/that

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:47: /// text is cancelled by plugins.
s/by plugins./by the plugin.

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:62: void UpdateSurroundingText(const
std::string& text,
We're trying to keep the C++ interfaces as very simple wrappers with minimal
conversion (at least going forward). So please just use "const Var&" here.

https://codereview.chromium.org/18671004/diff/137001/ppapi/thunk/ppb_text_inp...
File ppapi/thunk/ppb_text_input_thunk.cc (right):

https://codereview.chromium.org/18671004/diff/137001/ppapi/thunk/ppb_text_inp...
ppapi/thunk/ppb_text_input_thunk.cc:51: void UpdateSurroundingText(PP_Instance
instance, PP_Var text,
nit: I think it would be clearest to go ahead and add the version here as well.
(...Text_1_0)

[Seigo Nonaka, 2013-07-31 01:57:45]

Thank you for your reviews!

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
File ppapi/cpp/text_input_controller.h (right):

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:25: /// A constructor for creating a
<code>TextInputController</code>
On 2013/07/26 17:59:37, dmichael wrote:
> nit: period to end the sentence

Done.

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:46: /// CancelCompositionText() informs the
browser about the current composition
On 2013/07/26 17:59:37, dmichael wrote:
> s/about/that

Done.

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:47: /// text is cancelled by plugins.
On 2013/07/26 17:59:37, dmichael wrote:
> s/by plugins./by the plugin.

Done.

https://codereview.chromium.org/18671004/diff/137001/ppapi/cpp/text_input_con...
ppapi/cpp/text_input_controller.h:62: void UpdateSurroundingText(const
std::string& text,
On 2013/07/26 17:59:37, dmichael wrote:
> We're trying to keep the C++ interfaces as very simple wrappers with minimal
> conversion (at least going forward). So please just use "const Var&" here.

Done.

[commit-bot: I haz the power, 2013-07-31 01:59:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/18671004/158001

[commit-bot: I haz the power, 2013-07-31 03:26:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Seigo Nonaka, 2013-07-31 04:33:01]

Adding native_client_sdk owners.

Hi Bradley, could you take an owner review?

Thank you.

[bradn, 2013-07-31 16:49:28]

lgtm

[Seigo Nonaka, 2013-07-31 18:05:32]

I forgot addressing one comment.
Thank you for all reviewers.

I will submit after try bot will go green.

Thank you.

https://codereview.chromium.org/18671004/diff/137001/ppapi/thunk/ppb_text_inp...
File ppapi/thunk/ppb_text_input_thunk.cc (right):

https://codereview.chromium.org/18671004/diff/137001/ppapi/thunk/ppb_text_inp...
ppapi/thunk/ppb_text_input_thunk.cc:51: void UpdateSurroundingText(PP_Instance
instance, PP_Var text,
On 2013/07/26 17:59:37, dmichael wrote:
> nit: I think it would be clearest to go ahead and add the version here as
well.
> (...Text_1_0)

Done.

[commit-bot: I haz the power, 2013-07-31 20:13:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/18671004/173001

[commit-bot: I haz the power, 2013-07-31 20:39:13]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Seigo Nonaka, 2013-08-01 00:12:49]

Going to submit with bypassing presubmits because there are non essential
errors...

> Missing LGTM from an OWNER for these files:
>     native_client_sdk/src/libraries/ppapi/library.dsc
>     native_client_sdk/src/libraries/ppapi_cpp/library.dsc

I already got lgtm from bradnelson but seems CQ deos not recognize aliases.

> TODOs found in stable public PPAPI files:
> 
> ***************
> ppapi/cpp/input_event.h
> ppapi/c/ppb_input_event.h
> ppapi/api/ppb_input_event.idl
> ***************

TODOs are not introduced with this CL and seems still available.

[commit-bot: I haz the power, 2013-08-01 00:17:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/18671004/173001

[commit-bot: I haz the power, 2013-08-01 00:22:56]

Change committed as 214878

[noelallen1, 2013-08-06 17:06:05]

Running ./generators.py today causes several files to regenerate incorrectly.

I've traced down part of the problem to this CL which is breaking the generator.

It appears that both:
  ppb_text_input_controller.idl and
  dev/ppb_text_input_dev.idl

define:
  enum PP_TextInput_Type

[Seigo Nonaka, 2013-08-06 17:20:46]

On 2013/08/06 17:06:05, noelallen1 wrote:
> Running ./generators.py today causes several files to regenerate incorrectly.
> 
> I've traced down part of the problem to this CL which is breaking the
generator.
> 
> It appears that both:
>   ppb_text_input_controller.idl and
>   dev/ppb_text_input_dev.idl
> 
> define:
>   enum PP_TextInput_Type

Sorry for breaking script...
I ran generators.py and actually broken.

I think I can fix with renaming /PP_TextInput_Type/PP_TextInput_Type_Dev/ in
dev/ppb_text_input_dev.idl

I will make a CL to fix.