Proxy PPB_Input_Event, PPP_Input_Event, and associated IFs.

Proxy PPB_Input_Event, PPP_Input_Event, and associated IFs.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2035
TEST=ppapi_examples_event.cc
(mball is helping with an automated test right now)

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=6135

[dmichael (off chromium), 2011-07-18 22:31:06]

This is mostly working now, so I'd like to start the review since time is short.
Key events aren't working yet for some reason. There's a slim chance that the
problem is actually in the chrome implementation. I'll work on figuring that out
tonight or tomorrow.

Note that I hacked some dead code out of plugin_resource and
plugin_resource_tracker, because it caused me some confusion and would do the
same to the next person to touch it.

[dmichael (off chromium), 2011-07-19 14:29:59]

Last night I figured out the keyboard event problem. Unfiltered events in the
chrome implementation of the new API were defaulting to 'not handled', so the
plugin was not getting focus.  I just checked in this fix:
http://codereview.chromium.org/7426001
as Chrome r93017

Apologies for the automated test not being here yet. mball is helping out with
it. If he gets too busy, I'll take it over to make sure we have one.

Otherwise, I think this CL is ready for review, if you haven't started yet.
Trybots are green.

[dmichael (off chromium), 2011-07-19 14:47:40]

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource.h (left):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource.h:59: PP_Resource GetReference();
Note to reviewers: resource_id_ was never set, so was always zero. So this
function (which was never used) didn't work.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource.h:74: };
This class was never used and wouldn't have worked (because GetReference didn't
work).

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource.h:99: PP_Resource resource_id_;
resource_id_ was always 0. The resource id only actually shows up as the key in
the PluginResourceTracker. So I deleted it to save confusion for future
maintainers.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource.h:104: void StoppedTracking();
StoppedTracking only sets resource_id_ to 0, so I deleted it and references to
it since it had no effect. (oops, comment and friendship are no longer needed;
deleting in the next patch).

[sehr (please use chromium), 2011-07-20 00:33:40]

LGTM with a few nits and/or minor coding convention issues.  Sorry for the
delay.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
File src/shared/ppapi_proxy/browser_ppp_input_event.cc (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
src/shared/ppapi_proxy/browser_ppp_input_event.cc:91: //  char* event_data =
const_cast<char*>(reinterpret_cast<const char*>(data));
Why is this commented out?  Remove it.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
File src/shared/ppapi_proxy/browser_ppp_input_event.h (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
src/shared/ppapi_proxy/browser_ppp_input_event.h:39: // Allows BrowserInstance
to remove and instance from our map on deletion of
s/and/an/

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
src/shared/ppapi_proxy/browser_ppp_input_event.h:63: static
InstanceRegisteredTypesMap map;
The same thread safety issue we were talking about in my CL is here, right?  Or
is this only going to run on the main thread?

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_ppb_input_event.cc (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_ppb_input_event.cc:1: // Copyright (c) 2010 The
Native Client Authors. All rights reserved.
2011

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_ppb_input_event.cc:199: &::RequestInputEvents,
I think in the rest of the proxy we've not used the &::.  Please remove it.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.h (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.h:79: explicit
ResourceAndRefCounts(PluginResource* r, size_t browser_refcount);
Nit. explicit isn't strictly necessary for a binary constructor.

[dmichael (off chromium), 2011-07-20 03:50:22]

Fixed up style issues and deleted the whole static map thing, since it should be
the browser's responsibility to ignore the return value of unfiltered events
anyway. Running the trybots again. PTAL if you can.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
File src/shared/ppapi_proxy/browser_ppp_input_event.cc (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
src/shared/ppapi_proxy/browser_ppp_input_event.cc:91: //  char* event_data =
const_cast<char*>(reinterpret_cast<const char*>(data));
On 2011/07/20 00:33:41, sehr wrote:
> Why is this commented out?  Remove it.
Oops, done.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
File src/shared/ppapi_proxy/browser_ppp_input_event.h (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
src/shared/ppapi_proxy/browser_ppp_input_event.h:39: // Allows BrowserInstance
to remove and instance from our map on deletion of
On 2011/07/20 00:33:41, sehr wrote:
> s/and/an/

Done.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/brow...
src/shared/ppapi_proxy/browser_ppp_input_event.h:63: static
InstanceRegisteredTypesMap map;
On 2011/07/20 00:33:41, sehr wrote:
> The same thread safety issue we were talking about in my CL is here, right? 
Or
> is this only going to run on the main thread?
Input events always come in on the main thread. The RequestInputEvents,
RequestFilteringInputEvents, and ClearInputEvents, however, I have no control
over. Nice catch...  I guess we might as well start trying to put thread safety
in from the get-go, because it's hard to bandaid it on.

In this case, I realized this whole map was not necessary. It did help me catch
a bug in chrome, since I was always returning false for unfiltered events. But
in reality, Chrome should ignore our return value for us in the unfiltered case,
and we shouldn't have to worry about it in the proxy. I'll double check that
Brett's okay with this. It saves a bunch of error-prone code.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_ppb_input_event.cc (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_ppb_input_event.cc:199: &::RequestInputEvents,
On 2011/07/20 00:33:41, sehr wrote:
> I think in the rest of the proxy we've not used the &::.  Please remove it.
Good point. The & is definitely unnecessary. The latter 3 need the :: to select
the ones at file scope instead of the functions with the same name at class
scope, which otherwise win out by overloading rules. If you'd rather I rename
functions or something, let me know.

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
File src/shared/ppapi_proxy/plugin_resource_tracker.h (right):

http://codereview.chromium.org/7395005/diff/12001/src/shared/ppapi_proxy/plug...
src/shared/ppapi_proxy/plugin_resource_tracker.h:79: explicit
ResourceAndRefCounts(PluginResource* r, size_t browser_refcount);
On 2011/07/20 00:33:41, sehr wrote:
> Nit. explicit isn't strictly necessary for a binary constructor.

Done.