Add functions to generate input events to PPB_Testing_Dev. These make

Add functions to generate input events to PPB_Testing_Dev. These make
it possible to test APIs that require user gestures and other input events.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110865

[bbudge, 2011-10-27 23:49:36]

Adding methods to PPB_Testing_Dev to generate input events for testing. No NaCl
proxy yet.

[dmichael (off chromium), 2011-10-28 16:02:21]

A test would really help. This should make it possible to make a pretty good
test of PPB_InputEvent, right?

http://codereview.chromium.org/8413021/diff/1/ppapi/c/dev/ppb_testing_dev.h
File ppapi/c/dev/ppb_testing_dev.h (right):

http://codereview.chromium.org/8413021/diff/1/ppapi/c/dev/ppb_testing_dev.h#n...
ppapi/c/dev/ppb_testing_dev.h:17: #define PPB_TESTING_DEV_INTERFACE_0_6
"PPB_Testing(Dev);0.6"
optional: It would be nice if you could get rid of 0.6 while you're in the
neighborhood :-)

http://codereview.chromium.org/8413021/diff/1/ppapi/c/dev/ppb_testing_dev.h#n...
ppapi/c/dev/ppb_testing_dev.h:80: // Generates a key up / down input event.
When you say 'Generate', you mean inject one so that it will be received by
PPP_InputEvent::HandleInputEvent? (Assuming the instance is registered)

http://codereview.chromium.org/8413021/diff/1/ppapi/c/dev/ppb_testing_dev.h#n...
ppapi/c/dev/ppb_testing_dev.h:84: const PP_InputEvent_Key& key_event);
Can you instead use 1 function that takes a PP_Resource representing the
InputEvent (whatever kind it is)? And create the input events using their Create
methods? (See ppb_input_event.h)

That would seem simpler & more future proof, I think.

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

http://codereview.chromium.org/8413021/diff/1/ppapi/proxy/ppapi_messages.h#ne...
ppapi/proxy/ppapi_messages.h:998: PP_InputEvent_Wheel /* wheel_event */)
What about sending the resource? Or if you want it to be a brand-new event, you
could use InputEventData.

[bbudge, 2011-11-04 23:17:29]

Added a test for PPB_InputEvents.

I had to abandon my original approach, where I was sending the simulated events
through the WebView (and WebKit). That caused  the plugin to receive other
events, and the original event was also slightly different (time stamp,
modifiers). Instead, I now route the event right back to the plugin. This is
actually better in a sense, as it gives the ability to control exactly which
events the plugin gets. I will need to do another CL to fake the user gesture in
plugin_module.cc, since it requires a WebKit CL to land.

I removed PPB_Testing_Dev 0.6 (at least I think I did. Since I added a method, I
don't think we need to rev the interface, right?)

[dmichael (off chromium), 2011-11-07 17:12:58]

http://codereview.chromium.org/8413021/diff/24009/ppapi/c/dev/ppb_testing_dev.h
File ppapi/c/dev/ppb_testing_dev.h (right):

http://codereview.chromium.org/8413021/diff/24009/ppapi/c/dev/ppb_testing_dev...
ppapi/c/dev/ppb_testing_dev.h:16: #define PPB_TESTING_DEV_INTERFACE
PPB_TESTING_DEV_INTERFACE_0_7
I think in general we still need to rev the interface. Imagine that a
plugin/nexe which wants to use the new function is run against an older version
of Chrome that doesn't have the function. Bad things will ensue, and the
plugin/nexe won't really have any recourse.

It *probably* doesn't matter since this is only used internally. But I think the
safest course is to rev the interface.

http://codereview.chromium.org/8413021/diff/24009/ppapi/tests/test_input_even...
File ppapi/tests/test_input_event.cc (right):

http://codereview.chromium.org/8413021/diff/24009/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:107: // HandleInputEvent() should have been
called.
This only works if it's dispatched synchronously, which won't happen
out-of-process (your try runs seem to bear this out).

I think you need to run the message loop until you get it (in a while loop).

http://codereview.chromium.org/8413021/diff/24009/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:165: pp::Var(pp::Var::DontManage(),
Please don't use DontManage. It's a dangerous optimization that we should
probably get rid of. In this case, it looks like you want the 'PassRef'
constructor (that's what input_event.cc does in GetCharacterText, at least).

[I admit, this scares me a bit...  I think it implies that anyone who uses the C
interface has to explicitly Release the CharacterText any time they receive a
CHAR input event. Further, C++ plugins/nexes have to call GetCharacterText else
leak a reference... this at least isn't well documented].

http://codereview.chromium.org/8413021/diff/24009/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/24009/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:245:
EnterResourceNoLock<PPB_InputEvent_API> enter(input_event, false);
We should probably be consistent and use EnterResource. In truth, neither of
them locks in the in-process implementation.

But if we ever do try to support multi-threading for in-process plugins, we
would probably want to lock here to protect against the input_event PP_Resource
disappearing out from under us.

[bbudge, 2011-11-08 00:05:05]

I had to come up with a workaround to RunMessageLoop for the tests where we
*don't* expect an event. In this case, I simulate both the unexpected event, and
another expected event, so the test won't hang.

Is there another way?

http://codereview.chromium.org/8413021/diff/24009/ppapi/c/dev/ppb_testing_dev.h
File ppapi/c/dev/ppb_testing_dev.h (right):

http://codereview.chromium.org/8413021/diff/24009/ppapi/c/dev/ppb_testing_dev...
ppapi/c/dev/ppb_testing_dev.h:16: #define PPB_TESTING_DEV_INTERFACE
PPB_TESTING_DEV_INTERFACE_0_7
On 2011/11/07 17:12:58, dmichael wrote:
> I think in general we still need to rev the interface. Imagine that a
> plugin/nexe which wants to use the new function is run against an older
version
> of Chrome that doesn't have the function. Bad things will ensue, and the
> plugin/nexe won't really have any recourse.
> 
> It *probably* doesn't matter since this is only used internally. But I think
the
> safest course is to rev the interface.

Done.

http://codereview.chromium.org/8413021/diff/24009/ppapi/tests/test_input_even...
File ppapi/tests/test_input_event.cc (right):

http://codereview.chromium.org/8413021/diff/24009/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:165: pp::Var(pp::Var::DontManage(),
On 2011/11/07 17:12:58, dmichael wrote:
> Please don't use DontManage. It's a dangerous optimization that we should
> probably get rid of. In this case, it looks like you want the 'PassRef'
> constructor (that's what input_event.cc does in GetCharacterText, at least).
> 
> [I admit, this scares me a bit...  I think it implies that anyone who uses the
C
> interface has to explicitly Release the CharacterText any time they receive a
> CHAR input event. Further, C++ plugins/nexes have to call GetCharacterText
else
> leak a reference... this at least isn't well documented].

Done. I admit, I find this confusing.

http://codereview.chromium.org/8413021/diff/24009/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/24009/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:95: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
Most of these aren't needed anymore.

http://codereview.chromium.org/8413021/diff/24009/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:245:
EnterResourceNoLock<PPB_InputEvent_API> enter(input_event, false);
On 2011/11/07 17:12:58, dmichael wrote:
> We should probably be consistent and use EnterResource. In truth, neither of
> them locks in the in-process implementation.
> 
> But if we ever do try to support multi-threading for in-process plugins, we
> would probably want to lock here to protect against the input_event
PP_Resource
> disappearing out from under us.

Done.

[dmichael (off chromium), 2011-11-09 21:51:03]

Are you still thinking of doing it through WebKit as well?

Anyway, this is better than what we have, and you can iterate on the WebKit
stuff.

LGTM

[bbudge, 2011-11-17 18:59:32]

OK David, this is ready for another look. Things I'm not sure about and would
appreciate a close look:
1) How I handled removing version 0.6 and adding 0.8.
2) Use of Enter<> locks and general proxy hygiene.

Note, I moved the bulk of event translation code from plugin_module.cc to
ppapi_plugin_instance.cc.

Note, the tests aren't very comprehensive, but it's a good start I think.

[dmichael (off chromium), 2011-11-17 19:27:57]

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:342: switches::kEnablePepperTesting)) {
nit: 4-space indent

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:282: void GetKeyCode(const
std::string& char_text,
Can you put all this stuff somewhere else, like event_conversions?
ppapi_plugin_instance is already too big :-)

[bbudge, 2011-11-17 21:58:24]

PTAL, awaiting a new set of trys.

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:342: switches::kEnablePepperTesting)) {
On 2011/11/17 19:27:57, dmichael wrote:
> nit: 4-space indent
Isn't that standard when the line doesn't fit?

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:282: void GetKeyCode(const
std::string& char_text,
On 2011/11/17 19:27:57, dmichael wrote:
> Can you put all this stuff somewhere else, like event_conversions?
> ppapi_plugin_instance is already too big :-)

Done.

[bbudge, 2011-11-17 22:01:13]

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/57041/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:342: switches::kEnablePepperTesting)) {
On 2011/11/17 19:27:57, dmichael wrote:
> nit: 4-space indent

Oh wait, I see it now. Good eye. Fixed.

[dmichael (off chromium), 2011-11-18 03:44:45]

You should probably re-run your windows try; it failed to compile (though it
looks like a flake). Otherwise, just nits.

LGTM

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/event_...
File webkit/plugins/ppapi/event_conversion.cc (right):

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/event_...
webkit/plugins/ppapi/event_conversion.cc:471: key_down_event->unmodifiedText[0]
= text;
nit: looks like WebKit spacing here (and the if below)

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:342: switches::kEnablePepperTesting)) {
Ah, nice catch. I meant the line above, though :-)

switches::... should be indented 4 more spaces:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

[bbudge, 2011-11-18 19:00:06]

Running another set of trys.

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/event_...
File webkit/plugins/ppapi/event_conversion.cc (right):

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/event_...
webkit/plugins/ppapi/event_conversion.cc:471: key_down_event->unmodifiedText[0]
= text;
On 2011/11/18 03:44:45, dmichael wrote:
> nit: looks like WebKit spacing here (and the if below)

Done.

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/56025/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:342: switches::kEnablePepperTesting)) {
On 2011/11/18 03:44:45, dmichael wrote:
> Ah, nice catch. I meant the line above, though :-)
> 
> switches::... should be indented 4 more spaces:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

Done.

[bbudge, 2011-11-18 19:06:21]

Fix one more indentation snafu.

[commit-bot: I haz the power, 2011-11-18 21:14:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/8413021/72005

[commit-bot: I haz the power, 2011-11-18 21:15:31]

Presubmit check for 8413021-72005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Missing matching PPAPI definition:

***************
  ppapi/api/dev/ppb_testing_dev.idl
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/plugins/ppapi/ppapi_plugin_instance.cc,webkit/plugins/ppapi/event_conversion.cc,webkit/plugins/ppapi/plugin_module.cc,webkit/plugins/ppapi/ppapi_plugin_instance.h,webkit/plugins/ppapi/event_conversion.h

Presubmit checks took 3.8s to calculate.

[bbudge, 2011-11-19 05:36:14]

Patch got out of date, added IDL method.

[bbudge, 2011-11-19 21:20:36]

This now passes all tests. Need an LGTM from webkit/plugins OWNERS trung or
piman.

[piman, 2011-11-19 22:31:00]

http://codereview.chromium.org/8413021/diff/82006/ppapi/api/dev/ppb_testing_d...
File ppapi/api/dev/ppb_testing_dev.idl (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/api/dev/ppb_testing_d...
ppapi/api/dev/ppb_testing_dev.idl:83: * Passes the input event to the renderer,
which sends it back to the
nit: s/renderer/browser/ (here and below)
While it's true that in chrome the renderer process takes care of all this, it's
an implementation detail. From the plugin's point of view, there's only "the
plugin side" and "the browser side", browser being taken in the broad sense -
that's why the interfaces are named PPP and PPB respectively.

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
File ppapi/proxy/ppb_testing_proxy.cc (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
ppapi/proxy/ppb_testing_proxy.cc:83: NOTREACHED();
Not NOTREACHED() here. It is reachable by an incorrect use from the plugin. I'd
simply return here (and maybe DLOG).

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
ppapi/proxy/ppb_testing_proxy.cc:173: input_event));
So what you're doing here is unexpected.
The plugin creates an event. That round-trips to the browser, which creates a
resource for it. Now when you call SimulateInputEvent, the proxy extracts the
data out of the resource, sends it over to the browser, which then re-creates a
resource for it, with the same data. That seems very inefficient and
unnecessarily complex...

How about simply sending the host_resource in the message and passing that along
to the browser side's SimulateInputEvent?

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
File ppapi/tests/test_input_event.cc (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:63: !!input_event_interface_ &&
nit: no need for !!, pointers implicitly cast to bools when used in boolean
operators.

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:136: while (!received_expected_event_) {
So, the problem with this, is that you only have 2 alternatives: succeed or
don't terminate. If something breaks, it's going to be very painful for
everyone, because tests will hang.
How about using a separate mechanism to detect that we're done? E.g.
PostMessage, or synchronous scripting?

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:144: // the requested one is received by
HandleInputEvent. We can't (reliably test
typo: remove (.

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_event.h
File ppapi/tests/test_input_event.h (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.h:36: PP_InputEvent_MouseButton buttons);
Can we avoid overloading? It's disallowed by the style guide.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/event_...
File webkit/plugins/ppapi/event_conversion.cc (right):

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/event_...
webkit/plugins/ppapi/event_conversion.cc:277: WebUChar& code,
Please replace non-const reference parameters by pointers, as mandated by the
style guide.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/event_...
webkit/plugins/ppapi/event_conversion.cc:450: WebUChar code, text;
Initialize variables.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:233: NOTREACHED();
This is not NOTREACHED - it is reachable, e.g. by an incorrect use from the
plugin, but also if DidDestroyPlugin and SimulateInputEvent messages cross each
other when out-of-process.
In general, if we have an error return value, we'd return PP_ERROR_FAILED or
something. If we don't really care here, you can simply DLOG.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:238: NOTREACHED();
same here.

[bbudge, 2011-11-20 02:53:56]

Hey, thanks for the weekend reviewing!

http://codereview.chromium.org/8413021/diff/82006/ppapi/api/dev/ppb_testing_d...
File ppapi/api/dev/ppb_testing_dev.idl (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/api/dev/ppb_testing_d...
ppapi/api/dev/ppb_testing_dev.idl:83: * Passes the input event to the renderer,
which sends it back to the
On 2011/11/19 22:31:00, piman wrote:
> nit: s/renderer/browser/ (here and below)
> While it's true that in chrome the renderer process takes care of all this,
it's
> an implementation detail. From the plugin's point of view, there's only "the
> plugin side" and "the browser side", browser being taken in the broad sense -
> that's why the interfaces are named PPP and PPB respectively.

Done.

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
File ppapi/proxy/ppb_testing_proxy.cc (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
ppapi/proxy/ppb_testing_proxy.cc:83: NOTREACHED();
On 2011/11/19 22:31:00, piman wrote:
> Not NOTREACHED() here. It is reachable by an incorrect use from the plugin.
I'd
> simply return here (and maybe DLOG).

Done. This code idiom was copied from ppp_input_event_proxy.cc:29; is it also
incorrect there?

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
ppapi/proxy/ppb_testing_proxy.cc:173: input_event));
On 2011/11/19 22:31:00, piman wrote:
> So what you're doing here is unexpected.
> The plugin creates an event. That round-trips to the browser, which creates a
> resource for it. Now when you call SimulateInputEvent, the proxy extracts the
> data out of the resource, sends it over to the browser, which then re-creates
a
> resource for it, with the same data. That seems very inefficient and
> unnecessarily complex...
> 
> How about simply sending the host_resource in the message and passing that
along
> to the browser side's SimulateInputEvent?

I tried that but couldn't get it to work (CLs before #9). Apparently the input
event resources are created on the plugin side.

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
File ppapi/tests/test_input_event.cc (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:63: !!input_event_interface_ &&
On 2011/11/19 22:31:00, piman wrote:
> nit: no need for !!, pointers implicitly cast to bools when used in boolean
> operators.

Done.

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:136: while (!received_expected_event_) {
Redid this using PostMessage. An excellent suggestion. Thanks.

On 2011/11/19 22:31:00, piman wrote:
> So, the problem with this, is that you only have 2 alternatives: succeed or
> don't terminate. If something breaks, it's going to be very painful for
> everyone, because tests will hang.
> How about using a separate mechanism to detect that we're done? E.g.
> PostMessage, or synchronous scripting?

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.cc:144: // the requested one is received by
HandleInputEvent. We can't (reliably test
On 2011/11/19 22:31:00, piman wrote:
> typo: remove (.

Done.

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_event.h
File ppapi/tests/test_input_event.h (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/tests/test_input_even...
ppapi/tests/test_input_event.h:36: PP_InputEvent_MouseButton buttons);
On 2011/11/19 22:31:00, piman wrote:
> Can we avoid overloading? It's disallowed by the style guide.

Done.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/event_...
File webkit/plugins/ppapi/event_conversion.cc (right):

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/event_...
webkit/plugins/ppapi/event_conversion.cc:277: WebUChar& code,
On 2011/11/19 22:31:00, piman wrote:
> Please replace non-const reference parameters by pointers, as mandated by the
> style guide.

Done.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/event_...
webkit/plugins/ppapi/event_conversion.cc:450: WebUChar code, text;
On 2011/11/19 22:31:00, piman wrote:
> Initialize variables.

Done.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:233: NOTREACHED();
On 2011/11/19 22:31:00, piman wrote:
> This is not NOTREACHED - it is reachable, e.g. by an incorrect use from the
> plugin, but also if DidDestroyPlugin and SimulateInputEvent messages cross
each
> other when out-of-process.
> In general, if we have an error return value, we'd return PP_ERROR_FAILED or
> something. If we don't really care here, you can simply DLOG.

Done.

http://codereview.chromium.org/8413021/diff/82006/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:238: NOTREACHED();
On 2011/11/19 22:31:00, piman wrote:
> same here.

Done.

[piman, 2011-11-20 04:36:49]

lgtm

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
File ppapi/proxy/ppb_testing_proxy.cc (right):

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
ppapi/proxy/ppb_testing_proxy.cc:83: NOTREACHED();
On 2011/11/20 02:53:57, bbudge1 wrote:
> On 2011/11/19 22:31:00, piman wrote:
> > Not NOTREACHED() here. It is reachable by an incorrect use from the plugin.
> I'd
> > simply return here (and maybe DLOG).
> 
> Done. This code idiom was copied from ppp_input_event_proxy.cc:29; is it also
> incorrect there?

I think over there it's ok to have: the call is in the same process as the
renderer,  so there's no trust domain issue, and it can ensure there's no race.

http://codereview.chromium.org/8413021/diff/82006/ppapi/proxy/ppb_testing_pro...
ppapi/proxy/ppb_testing_proxy.cc:173: input_event));
On 2011/11/20 02:53:57, bbudge1 wrote:
> On 2011/11/19 22:31:00, piman wrote:
> > So what you're doing here is unexpected.
> > The plugin creates an event. That round-trips to the browser, which creates
a
> > resource for it. Now when you call SimulateInputEvent, the proxy extracts
the
> > data out of the resource, sends it over to the browser, which then
re-creates
> a
> > resource for it, with the same data. That seems very inefficient and
> > unnecessarily complex...
> > 
> > How about simply sending the host_resource in the message and passing that
> along
> > to the browser side's SimulateInputEvent?
> 
> I tried that but couldn't get it to work (CLs before #9). Apparently the input
> event resources are created on the plugin side.

I see, ok, that's fine then.

[bbudge, 2011-11-20 16:11:24]

Added plugin_ppb.cc to fix the NaCl proxy.

[bbudge, 2011-11-20 17:03:44]

Add destructor to remove Javascript injected into test page.

[piman, 2011-11-20 20:07:26]

lgtm