Implement Pepper IME API.

Implement Pepper IME API.

BUG=59425
TEST=Build chrome and ppapi_example_ime,
Confirm "out/Release/chrome --register-pepper-plugins='out/Release/lib/libppapi_example_ime.so;application/x-ppapi-example' ppapi/examples/ime/ime.html" works.

This CL is the last part for adding the basic IME support for PPAPI,
preceded by the previous two changes
 codereview.chromium.org/7882004 (API declarations) and
 codereview.chromium.org/7978019 (thunk and proxy implementation).
This CL comes with the actual Chrome-side implementation of the API
with an example to show how to use IME in PPAPI.

Keep in mind the current implementation still not reached the point of "the complete" set of IME APIs yet.
- Advanced features in design doc (like surrounding text retrieval) is not available.
- DOM and PPAPI composition events are not converted each other.
Rather, it aims to provide basic set of functions just needed to implement inline composition in plugins.

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

[kinaba, 2011-09-30 11:56:24]

Hi, I would like you to review the patch. Thank you all in advance!

Takayoshi, please check it conforms to your design doc.

Brett and Yuzhu, I need your eyes again particularly for checking whether it
fits nicely in PPAPI.

James and Hironori, I'd like your advice especially from the view point of
experts of IMEs on various platforms.

To James, I hope I addressed your partial comments in other tentative CL, except
two:

> > if (!composition_text_.empty() && text.empty())
> > focused_plugin_->HandleCompositionEnd(text);
> should send composition_text_ along with composition end event? compositionend
> event defined in HTML DOM Event Level3 has such behavior.

I don't think so. This branch corresponds to the DOM Level 3 statement "may be
the empty string if the content has been deleted or if the composition process
has been canceled", and this is also the way how WebKit does.

and

> > if (!CanComposeInline()) {
> Is it possible that a plugin cannot handle inline composition but can handle
text input event? Or do we plan to support it?

Currently we don't support it. Should be useful for future plan, though...
But, yes, anyhow, using CanComposeInline() to check the ability of event
handling is logically skewed. I added a new name to make the intent clearer.

[James Su, 2011-09-30 12:56:19]

Hi,
  I'm currently on vacation until Oct 10. Will review your CL asap.

Best Regards
James Su

在 2011年9月30日 下午7:56， <kinaba@chromium.org>写道：

> Reviewers: James Su, hbono, yzshen1, brettw, Takayoshi Kochi,
>
> Message:
> Hi, I would like you to review the patch. Thank you all in advance!
>
> Takayoshi, please check it conforms to your design doc.
>
> Brett and Yuzhu, I need your eyes again particularly for checking whether
> it
> fits nicely in PPAPI.
>
> James and Hironori, I'd like your advice especially from the view point of
> experts of IMEs on various platforms.
>
> To James, I hope I addressed your partial comments in other tentative CL,
> except
> two:
>
>  > if (!composition_text_.empty() && text.empty())
>> > focused_plugin_->**HandleCompositionEnd(text);
>> should send composition_text_ along with composition end event?
>> compositionend
>> event defined in HTML DOM Event Level3 has such behavior.
>>
>
> I don't think so. This branch corresponds to the DOM Level 3 statement "may
> be
> the empty string if the content has been deleted or if the composition
> process
> has been canceled", and this is also the way how WebKit does.
>
> and
>
>  > if (!CanComposeInline()) {
>> Is it possible that a plugin cannot handle inline composition but can
>> handle
>>
> text input event? Or do we plan to support it?
>
> Currently we don't support it. Should be useful for future plan, though...
> But, yes, anyhow, using CanComposeInline() to check the ability of event
> handling is logically skewed. I added a new name to make the intent
> clearer.
>
> Description:
> Implement Pepper IME API.
>
> BUG=59425
> TEST=Build chrome and ppapi_example_ime,
> Confirm "out/Release/chrome
> --register-pepper-plugins='**out/Release/lib/libppapi_**
> example_ime.so;application/x-**ppapi-example'
> ppapi/examples/ime/ime.html" works.
>
> This CL is the last part for adding the basic IME support for PPAPI,
> preceded by the previous two changes
>  codereview.chromium.org/**7882004<http://codereview.chromium.org/7882004>(API
declarations) and
> 
codereview.chromium.org/**7978019<http://codereview.chromium.org/7978019>(thunk
and proxy implementation).
> This CL comes with the actual Chrome-side implementation of the API
> with an example to show how to use IME in PPAPI.
>
> Keep in mind the current implementation still not reached the point of "the
> complete" set of IME APIs yet.
> - Advanced features in design doc (like surrounding text retrieval) is not
> available.
> - DOM and PPAPI composition events are not converted each other.
> Rather, it aims to provide basic set of functions just needed to implement
> inline composition in plugins.
>
> Please review this at
http://codereview.chromium.**org/8073021/<http://codereview.chromium.org/8073...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M content/renderer/pepper_**plugin_delegate_impl.h
>  M content/renderer/pepper_**plugin_delegate_impl.cc
>  M content/renderer/render_view.h
>  M content/renderer/render_view.**cc
>  M content/renderer/render_**widget.h
>  M content/renderer/render_**widget.cc
>  A ppapi/examples/ime/ime.cc
>  A ppapi/examples/ime/ime.html
>  M ppapi/ppapi_tests.gypi
>  M webkit/glue/webkit_glue.gypi
>  M webkit/plugins/ppapi/DEPS
>  M webkit/plugins/ppapi/mock_**plugin_delegate.h
>  M webkit/plugins/ppapi/mock_**plugin_delegate.cc
>  M webkit/plugins/ppapi/plugin_**delegate.h
>  M webkit/plugins/ppapi/ppapi_**plugin_instance.h
>  M webkit/plugins/ppapi/ppapi_**plugin_instance.cc
>  M webkit/plugins/ppapi/ppb_text_**input_impl.cc
>
>
>


-- 
- James Su

[brettw, 2011-10-03 16:46:00]

LGTM, mostly style nits.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:932: return gfx::Rect(0,0,0,0);
Can you put spaces after the commas?

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.h:380: // Current text input
composition text. Empty if no composition is on process.
is on process -> is in progress (I think that's what you meant, anyway).

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:251: #if !defined(TOUCH_UI)
I think having these ifdefs and comments in the initializer list make it a bit
harder to follow. How about defining a constant kDefaultTextInputType (or
something) above, and have the ifdefs and comments for that. Then you can just
use that constant here to initialize the class member as one line.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:264: text_input_caret_(0,0,0,0),
Can you put spaces after the commas?

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:505: for (size_t i=0;
i<underlines.size(); ++i) {
Can you put spaces around the = and < (same in the loop below)

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:513:
event.composition_selection_start = (utf8_offsets[0]==std::string::npos ?
Spaces around the == (same below)

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:548: return
PP_ToBool(plugin_input_event_interface_->HandleInputEvent(
Can you make it always return true if the event isn't being filtered? It will be
this way in the proxied implementation (unfiltered means async events with
implicit return true), so forcing this will make the in-process implementation
work the same way.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppb_tex...
File webkit/plugins/ppapi/ppb_text_input_impl.cc (right):

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppb_tex...
webkit/plugins/ppapi/ppb_text_input_impl.cc:45: if (itype<0 ||
itype>ui::TEXT_INPUT_TYPE_URL)
Can you add spaces around the < and > here?

[yzshen1, 2011-10-03 18:28:09]

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:668: focused_plugin_(0),
Please use NULL.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:857: focused_plugin_ = 0;
Please use NULL.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:896:
focused_plugin_->HandleCompositionUpdate(text, underlines,
Nit: Might be better to use composition_text_ instead of text here, although
they are the same.

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

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:51: size_t utf8_prev(const std::string& str, size_t i)
{
Please use FunctionNameLikeThis. (Line 60 has the same issue.)

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:61: for (size_t step = 0; step < n; ++step) {
Would you please show me why this loop is necessary?

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:76: virtual void focusIn(const pp::Rect& caret, const
pp::Rect& bounding_box) {}
Please use MethodNameLikeThis. (There are quite a few places below that have the
same problem. Please fix them all.)

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:109: font_size_(height-2),
Spaces around '-', please. (There are quite a few places below have the same
problem, please fix them all.)

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:210: 
Please use sentence, with capital initial and '.' at the end. (Multiple
occurrences.)

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:427: bool onCompositionUpdate(const
pp::IMEInputEvent_Dev ev) {
May consider to use const & (and also line 446).

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:460: bool anyoneFocused = false;
Please use variable_name_like_this.

[kochi, 2011-10-04 08:52:45]

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.h:48: struct
WebCompositionUnderline;
Move after 'class WebMouseEvent;' line.

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

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:27: static const uint32_t TEXTFIELD_BG_COLOR =
0xffffffff;
I don't think you need 'static' for these consts.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:55: while ((str[i] & 0xC0) == 0x80 && i > 0);
minor nit: why uppercase 0xC0? (same for line 65)

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.h:54: struct WebCursorInfo;
Please move this line down after line 57.

(see content/renderer/render_view.h for example).

[kinaba, 2011-10-05 04:43:19]

I hope I addressed the issues commented so far.
Thank you for nice suggestions (and forgive my unpolished CL, I'll do it better
next time...)!

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:668: focused_plugin_(0),
On 2011/10/03 18:28:10, yzshen1 wrote:
> Please use NULL.

Done.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:857: focused_plugin_ = 0;
On 2011/10/03 18:28:10, yzshen1 wrote:
> Please use NULL.

Done.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:896:
focused_plugin_->HandleCompositionUpdate(text, underlines,
On 2011/10/03 18:28:10, yzshen1 wrote:
> Nit: Might be better to use composition_text_ instead of text here, although
> they are the same.

Done.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:932: return gfx::Rect(0,0,0,0);
On 2011/10/03 16:46:01, brettw wrote:
> Can you put spaces after the commas?

Done.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.h:48: struct
WebCompositionUnderline;
On 2011/10/04 08:52:45, Takayoshi Kochi wrote:
> Move after 'class WebMouseEvent;' line.

Done.

http://codereview.chromium.org/8073021/diff/5002/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.h:380: // Current text input
composition text. Empty if no composition is on process.
On 2011/10/03 16:46:01, brettw wrote:
> is on process -> is in progress (I think that's what you meant, anyway).

Done.

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

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:27: static const uint32_t TEXTFIELD_BG_COLOR =
0xffffffff;
On 2011/10/04 08:52:45, Takayoshi Kochi wrote:
> I don't think you need 'static' for these consts.

Done. Also changed the names to kThisWay.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:51: size_t utf8_prev(const std::string& str, size_t i)
{
On 2011/10/03 18:28:10, yzshen1 wrote:
> Please use FunctionNameLikeThis. (Line 60 has the same issue.)

Done. Thanks for pointing out these style nits. I hope I have cleaned them up...

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:55: while ((str[i] & 0xC0) == 0x80 && i > 0);
On 2011/10/04 08:52:45, Takayoshi Kochi wrote:
> minor nit: why uppercase 0xC0? (same for line 65)

Done.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:61: for (size_t step = 0; step < n; ++step) {
On 2011/10/03 18:28:10, yzshen1 wrote:
> Would you please show me why this loop is necessary?

Because I use it for moving n characters forward in RefocusByMouseClick().
To avoid confusion, I've updated here to split the function into Utf8Next (for
advancing 1 character) and Utf8NextN (n characters).

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:76: virtual void focusIn(const pp::Rect& caret, const
pp::Rect& bounding_box) {}
On 2011/10/03 18:28:10, yzshen1 wrote:
> Please use MethodNameLikeThis. (There are quite a few places below that have
the
> same problem. Please fix them all.)

Done.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:109: font_size_(height-2),
On 2011/10/03 18:28:10, yzshen1 wrote:
> Spaces around '-', please. (There are quite a few places below have the same
> problem, please fix them all.)

Done.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:210: 
On 2011/10/03 18:28:10, yzshen1 wrote:
> Please use sentence, with capital initial and '.' at the end. (Multiple
> occurrences.)

Done.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:427: bool onCompositionUpdate(const
pp::IMEInputEvent_Dev ev) {
On 2011/10/03 18:28:10, yzshen1 wrote:
> May consider to use const & (and also line 446).

Done.

http://codereview.chromium.org/8073021/diff/5002/ppapi/examples/ime/ime.cc#ne...
ppapi/examples/ime/ime.cc:460: bool anyoneFocused = false;
On 2011/10/03 18:28:10, yzshen1 wrote:
> Please use variable_name_like_this.

Done.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:251: #if !defined(TOUCH_UI)
On 2011/10/03 16:46:01, brettw wrote:
> I think having these ifdefs and comments in the initializer list make it a bit
> harder to follow. How about defining a constant kDefaultTextInputType (or
> something) above, and have the ifdefs and comments for that. Then you can just
> use that constant here to initialize the class member as one line.

Done. It now looks much better, thanks!

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:264: text_input_caret_(0,0,0,0),
On 2011/10/03 16:46:01, brettw wrote:
> Can you put spaces after the commas?

Done.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:505: for (size_t i=0;
i<underlines.size(); ++i) {
On 2011/10/03 16:46:01, brettw wrote:
> Can you put spaces around the = and < (same in the loop below)

Done.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:513:
event.composition_selection_start = (utf8_offsets[0]==std::string::npos ?
On 2011/10/03 16:46:01, brettw wrote:
> Spaces around the == (same below)

Done.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:548: return
PP_ToBool(plugin_input_event_interface_->HandleInputEvent(
On 2011/10/03 16:46:01, brettw wrote:
> Can you make it always return true if the event isn't being filtered? It will
be
> this way in the proxied implementation (unfiltered means async events with
> implicit return true), so forcing this will make the in-process implementation
> work the same way.

Done.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.h:54: struct WebCursorInfo;
On 2011/10/04 08:52:45, Takayoshi Kochi wrote:
> Please move this line down after line 57.
> 
> (see content/renderer/render_view.h for example).

Done.

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppb_tex...
File webkit/plugins/ppapi/ppb_text_input_impl.cc (right):

http://codereview.chromium.org/8073021/diff/5002/webkit/plugins/ppapi/ppb_tex...
webkit/plugins/ppapi/ppb_text_input_impl.cc:45: if (itype<0 ||
itype>ui::TEXT_INPUT_TYPE_URL)
On 2011/10/03 16:46:01, brettw wrote:
> Can you add spaces around the < and > here?

Done.

[yzshen1, 2011-10-05 17:19:50]

LGTM

Thanks!

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

http://codereview.chromium.org/8073021/diff/18018/ppapi/examples/ime/ime.cc#n...
ppapi/examples/ime/ime.cc:51: size_t Utf8Prev(const std::string& str, size_t i)
{
What about something like
size_t GetPrevCharOffsetUtf8(const std::string& str, size_t current_pos)?
(And please also consider the name of Utf8Next and Utf8NextN.)

[kinaba, 2011-10-06 07:05:00]

Updated.

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

http://codereview.chromium.org/8073021/diff/18018/ppapi/examples/ime/ime.cc#n...
ppapi/examples/ime/ime.cc:51: size_t Utf8Prev(const std::string& str, size_t i)
{
On 2011/10/05 17:19:50, yzshen1 wrote:
> What about something like
> size_t GetPrevCharOffsetUtf8(const std::string& str, size_t current_pos)?
> (And please also consider the name of Utf8Next and Utf8NextN.)

Done. For Utf8NextN, I also replaced it with a function to compute the n-th
offset from the beginning.

[kochi, 2011-10-07 08:51:04]

LGTM

On 2011/10/06 07:05:00, kinaba wrote:
> Updated.
> 
> http://codereview.chromium.org/8073021/diff/18018/ppapi/examples/ime/ime.cc
> File ppapi/examples/ime/ime.cc (right):
> 
>
http://codereview.chromium.org/8073021/diff/18018/ppapi/examples/ime/ime.cc#n...
> ppapi/examples/ime/ime.cc:51: size_t Utf8Prev(const std::string& str, size_t
i)
> {
> On 2011/10/05 17:19:50, yzshen1 wrote:
> > What about something like
> > size_t GetPrevCharOffsetUtf8(const std::string& str, size_t current_pos)?
> > (And please also consider the name of Utf8Next and Utf8NextN.)
> 
> Done. For Utf8NextN, I also replaced it with a function to compute the n-th
> offset from the beginning.

[James Su, 2011-10-11 04:13:03]

LGTM with some nits.

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:905: composition_text_ = text;
Looks like we don't need to copy text to composition_text_, as it'll always be
cleared at the end of this method.

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:183: int selection_end);
Perhaps we should use ui::CompositionText instead. But anyway, we can keep it as
is for now.

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:382: string16 composition_text_;
Looks like a boolean flag indicating if the current composition text is empty
should be enough.

[kinaba, 2011-10-11 09:12:21]

Thank you! I updated and replied the comments.

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:905: composition_text_ = text;
On 2011/10/11 04:13:04, James Su wrote:
> Looks like we don't need to copy text to composition_text_, as it'll always be
> cleared at the end of this method.

Done.
Also, added some comments on the text.empty() case.

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/8073021/diff/28002/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.h:382: string16 composition_text_;
On 2011/10/11 04:13:04, James Su wrote:
> Looks like a boolean flag indicating if the current composition text is empty
> should be enough.

We need to store the text (not only its emptiness) in order to respond to
RenderWidgetHost::ImeConfirmComposition(void) "// Finishes an ongoing
composition with the composition text set by last SetComposition() call." GTK
and Views host use the method on focus-out. In this case,
PepperPluginDelegateImpl::OnImeConfirmComposition is called with the argument
text="" and the stored composition_text_ is used.

[kinaba, 2011-10-11 09:45:47]

Bono-san,

Could you kindly take a look and comment? Thank you so much!

[commit-bot: I haz the power, 2011-10-12 10:13:59]

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

[commit-bot: I haz the power, 2011-10-12 11:38:33]

Change committed as 105056