Use macros to define pepper interfaces

This patch tries to remove most of the manual registration for Pepper interfaces, and replaces it with a list of macros. When files want to know which Pepper interface names and structs there are, they define what they want to do with the macros, and then include the relevant files for the classes of interfaces they want (stable, private, dev).

This does not convert all the dev interfaces. I just did a few to keep the patch smaller. So there is still a lot of manual registration.

This fixes the previous design problem where we assumed one *_Proxy object == one interface. We have been hacking around this lately with duplicate GetInfo calls, but this doesn't work for PPP interfaces.

Now, a _Proxy object is just there to help keep things organized. One proxy can handle zero, one, or many interfaces, and this mapping is controlled by just one line in the interfaces file.

So for example, to add a new function to a new version of an interface with backward compatibility, you would add that function to the _api.h file, and write a thunk for the new interface. Then you only need to add one line to the interfaces_ppb_public_stable.h file and that will be hooked up with the proxy and the implementation.

This removes some _proxy objects/files that were used only to declare that the interfaces existed, since they're no longer necessary.

I folded Console into the Instance API which removed a bunch of code.

I removed FileChooser 0.4. I think everybody has converted to the new one, and I think parts of it weren't even hooked up properly anymore.

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

[brettw, 2011-08-25 21:24:52]

This is what I had in mind for the autogenerated interface lists.

The first section would be the "common name" for the interfaces, regardless of
version. This would typically correspond to one proxy or resource object.

The section section lists the common name the interface string name, and the
struct name.

[noelallen_use_chromium, 2011-09-07 21:16:57]

First round of feedback, up to but not including ppb_file_chooser.*

http://codereview.chromium.org/7740038/diff/5001/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (left):

http://codereview.chromium.org/7740038/diff/5001/ppapi/ppapi_proxy.gypi#oldco...
ppapi/ppapi_proxy.gypi:118: 'proxy/ppb_url_request_info_proxy.cc',
Why is URL request removed?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/dispatcher.cc
File ppapi/proxy/dispatcher.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/dispatcher.cc#ne...
ppapi/proxy/dispatcher.cc:69: if (msg.routing_id() <= 0 || msg.routing_id() >=
INTERFACE_ID_COUNT) {
What is guaranteeing that Chrome and the Plugin this proxy is servicing match id
values?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/host_dispatcher.h
File ppapi/proxy/host_dispatcher.h (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/host_dispatcher....
ppapi/proxy/host_dispatcher.h:80: void set_allow_plugin_reentrancy() {
I understand this is just a moved block, but isn't this transitory?  How does
this get cleared?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_id.h
File ppapi/proxy/interface_id.h (left):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_id.h#o...
ppapi/proxy/interface_id.h:50: INTERFACE_ID_PPB_URL_RESPONSE_INFO,
These IDs do not match the include guard, interface name or enum's it defines.
see ppb_url_response_info.h:
#define PPB_URLRESPONSEINFO_INTERFACE_1_0 "PPB_URLResponseInfo;1.0"
   or
PP_URLRESPONSEPROPERTY_URL

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_list.h
File ppapi/proxy/interface_list.h (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_list.h...
ppapi/proxy/interface_list.h:49: InterfaceID id;
Should there be a prototype in case whichever header indirectly includes this
definition stops defining it?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
File ppapi/proxy/plugin_dispatcher.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.cc:134: bool handled = true;
Observation, no need to change in this CL but, this logic is reversed, since at
the moment this is set to true, it is not yet handled.
  bool failed = false;
  IPC_MESSGAGE_UNHANDLED(failed = true);

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.cc:226: *result =
!!GetPluginInterface(interface_name);
Would the style guide require this to be:
  *result = GetPluginInterface(interface_name) != NULL;

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
File ppapi/proxy/plugin_dispatcher_unittest.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher_unittest.cc:56: return
!!plugin_dispatcher()->proxies_[id].get();
If this is the normal way to do a pointer check, we should probably update the
style guide.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_audio_proxy.cc
File ppapi/proxy/ppb_audio_proxy.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_audio_proxy....
ppapi/proxy/ppb_audio_proxy.cc:183: HostResource* result) {
Does this need:
  thunk::EnterResourceCreation enter(instance);

I didn't see a trusted version of this interface in the CL.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_crypto_proxy.cc
File ppapi/proxy/ppb_crypto_proxy.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_crypto_proxy...
ppapi/proxy/ppb_crypto_proxy.cc:20: InterfaceProxy*
CreateCryptoProxy(Dispatcher* dispatcher) {
Is this still needed?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_crypto_proxy...
ppapi/proxy/ppb_crypto_proxy.cc:35: const InterfaceProxy::Info*
PPB_Crypto_Proxy::GetInfo() {
Shouldn't this be going away?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_crypto_proxy.h
File ppapi/proxy/ppb_crypto_proxy.h (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_crypto_proxy...
ppapi/proxy/ppb_crypto_proxy.h:22: static const Info* GetInfo();
You removed GetInfo from the other interfaces.

[noelallen_use_chromium, 2011-09-08 00:47:41]

A few questions to make sure didn't miss anything, but what's there LGTM.

BTW, if we annotate the IDL with [proxy(IPC)], we could auto generate:
 ppapi/thunk/interfaces_ppb_private.h
 ppapi/thunk/interfaces_ppb_public_dev.h
 ppapi/thunk/interfaces_ppb_public_stable.h

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:234: true);
Not, thunk::EnterResourceCreation enter(instance) ?

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppp_input_event_...
File ppapi/proxy/ppp_input_event_proxy.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppp_input_event_...
ppapi/proxy/ppp_input_event_proxy.cc:14: //#include
"ppapi/proxy/ppb_input_event_proxy.h"
Remove?

[brettw, 2011-09-08 05:34:29]

New snap up.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/dispatcher.cc
File ppapi/proxy/dispatcher.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/dispatcher.cc#ne...
ppapi/proxy/dispatcher.cc:69: if (msg.routing_id() <= 0 || msg.routing_id() >=
INTERFACE_ID_COUNT) {
Just the fact that this is all compiled at the same time with the same code.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/host_dispatcher.h
File ppapi/proxy/host_dispatcher.h (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/host_dispatcher....
ppapi/proxy/host_dispatcher.h:80: void set_allow_plugin_reentrancy() {
I added an additional sentence here and moved it back the the old place (I think
the move was a mistake).

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_list.h
File ppapi/proxy/interface_list.h (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_list.h...
ppapi/proxy/interface_list.h:49: InterfaceID id;
I'm not sure what you mean here.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
File ppapi/proxy/plugin_dispatcher.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.cc:134: bool handled = true;
This is normal Chrome style for these things so you don't have to set
handled=true for every message handler.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.cc:226: *result =
!!GetPluginInterface(interface_name);
It's not, to my knowledge, disallowed, but I wouldn't say it's the preferred way
either. I use this sometimes, and it's used in other places as well to avoid
compiler warnings about implicit bool conversions.
http://codesearch.google.com/codesearch#search/&exact_package=chromium&q=!!&t...

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_audio_proxy.cc
File ppapi/proxy/ppb_audio_proxy.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/ppb_audio_proxy....
ppapi/proxy/ppb_audio_proxy.cc:183: HostResource* result) {
This is mostly touching the proxy and AudioTrusted isn't proxied (it's used to
implement the proxy).

[noelallen_use_chromium, 2011-09-08 19:49:16]

LGTM.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/dispatcher.cc
File ppapi/proxy/dispatcher.cc (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/dispatcher.cc#ne...
ppapi/proxy/dispatcher.cc:69: if (msg.routing_id() <= 0 || msg.routing_id() >=
INTERFACE_ID_COUNT) {
Sure for our current uses.  I just wasn't sure if there was a non chrome.dll
case we need to support.

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_list.h
File ppapi/proxy/interface_list.h (right):

http://codereview.chromium.org/7740038/diff/5001/ppapi/proxy/interface_list.h...
ppapi/proxy/interface_list.h:49: InterfaceID id;
On 2011/09/08 05:34:29, brettw wrote:
> I'm not sure what you mean here.
nvm.  I was wondering if you should explicitly include interface_id.h