Test for Pepper IME events.

Test for Pepper IME events.

This patch adds a way to simulate IME composition events
inside the renderer process, and tests that IME events are
properly passed between the renderer and plugins.

ppapi/tests/test_ime_input_event.cc:
is the actual test case

ppapi/{api,c,cpp}/dev/*ime_input_event_dev*:
implements an API to create IME events from plugins for testing.

other files:
wire necessary stuff for simulating IME events.
Since Pepper IME events are not delivered through WebKit/DOM layer
but rather directly sent from renderer to plugins,
the simulation part also follows the similar code path.

BUG=126714
TEST=browser_tests PPAPITest.ImeInputEvent

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

[kinaba, 2012-05-14 08:51:41]

Sorry for taking long time for coming back to this, but I somehow implemented
the testing work for Pepper IME.

Brett and Yuzhu, could you kindly check the CL from Pepper's perspective?

Kochi-san, I'd like to you to check that the test cases are correctly verifying
what we want to verify. (Of course comments to other parts are welcome!:))

Thanks!

[yzshen1, 2012-05-15 18:03:47]

Only nits.

http://codereview.chromium.org/10391101/diff/11001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10391101/diff/11001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:436: if (render_view_)
nit: {} for multiple-line body.

http://codereview.chromium.org/10391101/diff/11001/ppapi/api/dev/ppb_ime_inpu...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

http://codereview.chromium.org/10391101/diff/11001/ppapi/api/dev/ppb_ime_inpu...
ppapi/api/dev/ppb_ime_input_event_dev.idl:28: * you will get a mouse event
passed through the
This is not a mouse event.

http://codereview.chromium.org/10391101/diff/11001/ppapi/api/dev/ppb_ime_inpu...
ppapi/api/dev/ppb_ime_input_event_dev.idl:35: * input event. The type must be
one of the ime event types.
nit: Please use 'IME' instead of 'ime' to be consistent. (and line 60).

http://codereview.chromium.org/10391101/diff/11001/ppapi/api/dev/ppb_ime_inpu...
ppapi/api/dev/ppb_ime_input_event_dev.idl:57: * @param[in] selection_start The
end index returned by
selection_start -> selection_end

http://codereview.chromium.org/10391101/diff/11001/ppapi/api/dev/ppb_ime_inpu...
ppapi/api/dev/ppb_ime_input_event_dev.idl:63: PP_Resource Create([in]
PP_Instance instance,
Nit: Usually Create() is placed at the top, before Is*().

http://codereview.chromium.org/10391101/diff/11001/ppapi/proxy/resource_creat...
File ppapi/proxy/resource_creation_proxy.cc (right):

http://codereview.chromium.org/10391101/diff/11001/ppapi/proxy/resource_creat...
ppapi/proxy/resource_creation_proxy.cc:107: &segment_offsets[segment_number+1]);
spaces around '+', please.

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
File ppapi/tests/test_ime_input_event.cc (right):

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:9: #include
"ppapi/c/dev/ppb_ime_input_event_dev.h"
sort this section, please.

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:31: void TestImeInputEvent::RunTests(const
std::string& filter) {
Keep the definitions in the same order as the declarations in the .h file.

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:42: ime_input_event_interface_(NULL) {
Please also init those boolean members.

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:49: js_code += "var plugin =
document.getElementById('plugin');"
Why do we need to do a '+=' here? (and line 81)

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:204: switch
(input_event_interface_->GetType(received)) {
received_type?

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:220: uint32_t receivedSegmentNumber =
name_variables_this_way (and some other places).

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:223:
ime_input_event_interface_->GetSegmentNumber(received);
expected?

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:253:
ime_input_event_interface_->GetTargetSegment(received);
expected? :)

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
File ppapi/tests/test_ime_input_event.h (right):

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.h:13: #include
"ppapi/c/dev/ppb_ime_input_event_dev.h"
sort, please.

http://codereview.chromium.org/10391101/diff/11001/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.h:26: // TestCase implementation.
line 22-24 also TestCase implementation.

http://codereview.chromium.org/10391101/diff/11001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10391101/diff/11001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1646: underline.endOffset =
offsets[i+1];
spaces around '+', please.

http://codereview.chromium.org/10391101/diff/11001/webkit/plugins/ppapi/resou...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/10391101/diff/11001/webkit/plugins/ppapi/resou...
webkit/plugins/ppapi/resource_creation_impl.cc:195: if (type !=
PP_INPUTEVENT_TYPE_IME_COMPOSITION_START &&
Please put the implementation in ppb_input_event_shared, so that you can use it
in here and resource_creation_proxy.cc.

[brettw, 2012-05-15 19:33:12]

I'll defer my review to Yuzhu. LGTM to make owners checks happy.

[brettw, 2012-05-15 19:33:32]

BTW thanks for following up with this!

[kinaba, 2012-05-16 10:13:57]

Thanks for the comments.

https://chromiumcodereview.appspot.com/10391101/diff/11001/content/renderer/p...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/content/renderer/p...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:436: if (render_view_)
On 2012/05/15 18:03:48, yzshen1 wrote:
> nit: {} for multiple-line body.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/api/dev/ppb_...
File ppapi/api/dev/ppb_ime_input_event_dev.idl (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:28: * you will get a mouse event
passed through the
On 2012/05/15 18:03:48, yzshen1 wrote:
> This is not a mouse event.

Done. Copy&pasting is always harmful..:(

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:35: * input event. The type must be
one of the ime event types.
On 2012/05/15 18:03:48, yzshen1 wrote:
> nit: Please use 'IME' instead of 'ime' to be consistent. (and line 60).

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:57: * @param[in] selection_start The
end index returned by
On 2012/05/15 18:03:48, yzshen1 wrote:
> selection_start -> selection_end

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/api/dev/ppb_...
ppapi/api/dev/ppb_ime_input_event_dev.idl:63: PP_Resource Create([in]
PP_Instance instance,
On 2012/05/15 18:03:48, yzshen1 wrote:
> Nit: Usually Create() is placed at the top, before Is*().

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/proxy/resour...
File ppapi/proxy/resource_creation_proxy.cc (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/proxy/resour...
ppapi/proxy/resource_creation_proxy.cc:107: &segment_offsets[segment_number+1]);
On 2012/05/15 18:03:48, yzshen1 wrote:
> spaces around '+', please.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
File ppapi/tests/test_ime_input_event.cc (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:9: #include
"ppapi/c/dev/ppb_ime_input_event_dev.h"
On 2012/05/15 18:03:48, yzshen1 wrote:
> sort this section, please.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:31: void TestImeInputEvent::RunTests(const
std::string& filter) {
On 2012/05/15 18:03:48, yzshen1 wrote:
> Keep the definitions in the same order as the declarations in the .h file.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:42: ime_input_event_interface_(NULL) {
On 2012/05/15 18:03:48, yzshen1 wrote:
> Please also init those boolean members.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:49: js_code += "var plugin =
document.getElementById('plugin');"
On 2012/05/15 18:03:48, yzshen1 wrote:
> Why do we need to do a '+=' here? (and line 81)

Done. We don't need it.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:204: switch
(input_event_interface_->GetType(received)) {
On 2012/05/15 18:03:48, yzshen1 wrote:
> received_type?

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:220: uint32_t receivedSegmentNumber =
On 2012/05/15 18:03:48, yzshen1 wrote:
> name_variables_this_way (and some other places).

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:223:
ime_input_event_interface_->GetSegmentNumber(received);
On 2012/05/15 18:03:48, yzshen1 wrote:
> expected?

Done. Good catch.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.cc:253:
ime_input_event_interface_->GetTargetSegment(received);
On 2012/05/15 18:03:48, yzshen1 wrote:
> expected? :)

Done. expected is expected.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
File ppapi/tests/test_ime_input_event.h (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.h:13: #include
"ppapi/c/dev/ppb_ime_input_event_dev.h"
On 2012/05/15 18:03:48, yzshen1 wrote:
> sort, please.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/ppapi/tests/test_i...
ppapi/tests/test_ime_input_event.h:26: // TestCase implementation.
On 2012/05/15 18:03:48, yzshen1 wrote:
> line 22-24 also TestCase implementation.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1646: underline.endOffset =
offsets[i+1];
On 2012/05/15 18:03:48, yzshen1 wrote:
> spaces around '+', please.

Done.

https://chromiumcodereview.appspot.com/10391101/diff/11001/webkit/plugins/ppa...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

https://chromiumcodereview.appspot.com/10391101/diff/11001/webkit/plugins/ppa...
webkit/plugins/ppapi/resource_creation_impl.cc:195: if (type !=
PP_INPUTEVENT_TYPE_IME_COMPOSITION_START &&
On 2012/05/15 18:03:48, yzshen1 wrote:
> Please put the implementation in ppb_input_event_shared, so that you can use
it
> in here and resource_creation_proxy.cc.

Done. I also did the refactoring for other input events than IME, but if there's
a particular reason not to do so I'll revert it.

[yzshen1, 2012-05-16 20:33:28]

LGTM

http://codereview.chromium.org/10391101/diff/11001/webkit/plugins/ppapi/resou...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/10391101/diff/11001/webkit/plugins/ppapi/resou...
webkit/plugins/ppapi/resource_creation_impl.cc:195: if (type !=
PP_INPUTEVENT_TYPE_IME_COMPOSITION_START &&
I don't know of any reason not to do that.
Thanks!
On 2012/05/16 10:13:57, kinaba wrote:
> On 2012/05/15 18:03:48, yzshen1 wrote:
> > Please put the implementation in ppb_input_event_shared, so that you can use
> it
> > in here and resource_creation_proxy.cc.
> 
> Done. I also did the refactoring for other input events than IME, but if
there's
> a particular reason not to do so I'll revert it.

[kochi, 2012-05-18 10:18:50]

LGTM with nits

Thanks for working on this and sorry for the review delay.
This test covers all the 4 event types for both IME aware/unaware plugins and
looks good.

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
File ppapi/tests/test_ime_input_event.cc (right):

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:21: const char *(kCompositionChar[]) = {
I don't think you don't need parentheses and you should write like
const char* kCompositionChar[] = {

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:22: "\xE6\x96\x87", "\xE5\xAD\x97",
"\xE5\x88\x97"
Can you add a comment that describes what is the string?

Maybe like this:
// Japanese Kanji letters meaning "a string" ("mojiretsu" in Kanji)

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:77: std::string js_code;
nit: why don't you just declare and substitute at once?

std::string js_code = "var plugin...

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:116: default:
Could you add a comment for ignoring other events?
like
// Don't care for any other input event types for this test.

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:278: uint32_t expected_selection_end;
Usually initialization should be done for local variable declaration.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables

Initializing '= 0' is fine here.

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:333: ASSERT_TRUE(
nit: new line unnecessary.

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:335: ASSERT_TRUE(
This line also should fit in one line.

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:363: ASSERT_TRUE(
same as line 333

http://chromiumcodereview.appspot.com/10391101/diff/16006/ppapi/tests/test_im...
ppapi/tests/test_ime_input_event.cc:365: ASSERT_TRUE(
same as line 335

[kinaba, 2012-05-21 02:40:15]

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
File ppapi/tests/test_ime_input_event.cc (right):

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:21: const char *(kCompositionChar[]) = {
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> I don't think you don't need parentheses and you should write like
> const char* kCompositionChar[] = {

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:22: "\xE6\x96\x87", "\xE5\xAD\x97",
"\xE5\x88\x97"
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> Can you add a comment that describes what is the string?
> 
> Maybe like this:
> // Japanese Kanji letters meaning "a string" ("mojiretsu" in Kanji)

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:77: std::string js_code;
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> nit: why don't you just declare and substitute at once?
> 
> std::string js_code = "var plugin...

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:116: default:
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> Could you add a comment for ignoring other events?
> like
> // Don't care for any other input event types for this test.

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:278: uint32_t expected_selection_end;
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> Usually initialization should be done for local variable declaration.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables
> 
> Initializing '= 0' is fine here.

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:333: ASSERT_TRUE(
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> nit: new line unnecessary.

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:335: ASSERT_TRUE(
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> This line also should fit in one line.

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:363: ASSERT_TRUE(
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> same as line 333

Done.

http://codereview.chromium.org/10391101/diff/16006/ppapi/tests/test_ime_input...
ppapi/tests/test_ime_input_event.cc:365: ASSERT_TRUE(
On 2012/05/18 10:18:50, Takayoshi Kochi wrote:
> same as line 335

Done. Thanks for catching them up!

[commit-bot: I haz the power, 2012-05-21 04:42:10]

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

[commit-bot: I haz the power, 2012-05-21 05:20:25]

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

[commit-bot: I haz the power, 2012-05-21 05:40:40]

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

[commit-bot: I haz the power, 2012-05-21 07:09:34]

Change committed as 138080