Refactor the way singleton-style resources are exposed via PPB_Instance

Refactor the way singleton-style resources are exposed via PPB_Instance

Currently a lot of boilerplate needs to be added everytime we add a new singleton style resource and as we add more of them this will accumulate. This patch reduces the amount of code needed by exposing a GetSingletonResource function in PPB_Instance_API which takes an ID specifying what type of resource is needed. A new Enter type, EnterInstanceAPI, is provided which is templated on the singleton API you want access to.


BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170221

[raymes, 2012-11-20 21:14:59]

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/ppb_flash_clip...
File ppapi/thunk/ppb_flash_clipboard_thunk.cc (right):

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:20: FLASH_CLIPBOARD_SINGLETON_ID);
Note that we could make this even easier by associating the SingletonID with the
API (e.g. PPB_Flash_Clipboard_API::kSingletonID) and then EnterInstanceAPI could
automatically grab it. Let me know your thoughts on this.

[yzshen1, 2012-11-20 21:47:25]

Some drive-by comments.

https://codereview.chromium.org/11359063/diff/4014/ppapi/proxy/plugin_dispatc...
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/11359063/diff/4014/ppapi/proxy/plugin_dispatc...
ppapi/proxy/plugin_dispatcher.h:56: typedef std::map<SingletonResourceID,
linked_ptr<Resource> >
Resource itself is a ref-counted object. You shouldn't use linked_ptr()

https://codereview.chromium.org/11359063/diff/4014/ppapi/proxy/plugin_dispatc...
ppapi/proxy/plugin_dispatcher.h:57: SingletonResourceMap;
4 space indent.

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:256: SingletonResourceID singleton_resource_id) :
EnterBase() {
We can deduce the SingletonResourceID from ApiT, right?
One way to consider is template specialization.

[raymes, 2012-11-20 23:30:23]

https://codereview.chromium.org/11359063/diff/4014/ppapi/proxy/plugin_dispatc...
File ppapi/proxy/plugin_dispatcher.h (right):

https://codereview.chromium.org/11359063/diff/4014/ppapi/proxy/plugin_dispatc...
ppapi/proxy/plugin_dispatcher.h:56: typedef std::map<SingletonResourceID,
linked_ptr<Resource> >
On 2012/11/20 21:47:25, yzshen1 wrote:
> Resource itself is a ref-counted object. You shouldn't use linked_ptr()

Done.

https://codereview.chromium.org/11359063/diff/4014/ppapi/proxy/plugin_dispatc...
ppapi/proxy/plugin_dispatcher.h:57: SingletonResourceMap;
On 2012/11/20 21:47:25, yzshen1 wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/enter.h#newcod...
ppapi/thunk/enter.h:256: SingletonResourceID singleton_resource_id) :
EnterBase() {
Yes I was considering whether or not I wanted to do it. I think it's better to.
I added constants to the API definitions.

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/ppb_flash_clip...
File ppapi/thunk/ppb_flash_clipboard_thunk.cc (right):

https://codereview.chromium.org/11359063/diff/4014/ppapi/thunk/ppb_flash_clip...
ppapi/thunk/ppb_flash_clipboard_thunk.cc:20: FLASH_CLIPBOARD_SINGLETON_ID);
On 2012/11/20 21:14:59, raymes wrote:
> Note that we could make this even easier by associating the SingletonID with
the
> API (e.g. PPB_Flash_Clipboard_API::kSingletonID) and then EnterInstanceAPI
could
> automatically grab it. Let me know your thoughts on this.

Done.

[dmichael (off chromium), 2012-11-21 17:26:26]

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:255: EnterInstanceAPI(PP_Instance instance) : EnterBase() {
nit: explicit

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:256: functions_ = NULL;
nit: Why not initialize this in the initializer list?

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:273: ApiT* functions() { return functions_; }
I'm noticing we don't do this in EnterInstance either, but that should probably
be "const ApiT* functions() const"
(unless that's getting too crazy with const)

https://codereview.chromium.org/11359063/diff/9006/ppapi/proxy/ppb_instance_p...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/11359063/diff/9006/ppapi/proxy/ppb_instance_p...
ppapi/proxy/ppb_instance_proxy.cc:345: Resource* new_singleton(NULL);
How about making this a scoped_refptr? Just so if you or somebody else comes
along and adds an early return, we won't leak.

https://codereview.chromium.org/11359063/diff/9006/ppapi/proxy/ppb_instance_p...
ppapi/proxy/ppb_instance_proxy.cc:365: // the cases.
What about moving the #ifdef to the end, and having something like:
#else
    case FLASH_SINGLETON_ID:
    case FLASH_CLIPBOARD_SINGLETON_ID:
      NOTREACHED();
#endif

...I really like the compiler warning that points us at pieces of code that need
to be updated when we add an enum value.

https://codereview.chromium.org/11359063/diff/9006/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/11359063/diff/9006/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:321: :
::ppapi::Resource(::ppapi::Resource::Untracked()),
nit: using ppapi::Resource and using ppapi::thunk::PPB_Gamepad_API might make
some of this cleaner (it also is more consistent to do it that way in this file)

[raymes, 2012-11-21 20:59:10]

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:255: EnterInstanceAPI(PP_Instance instance) : EnterBase() {
On 2012/11/21 17:26:26, dmichael wrote:
> nit: explicit

Done.

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:256: functions_ = NULL;
On 2012/11/21 17:26:26, dmichael wrote:
> nit: Why not initialize this in the initializer list?

Done.

https://codereview.chromium.org/11359063/diff/13002/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:273: ApiT* functions() { return functions_; }
As discussed, we can't return const ApiT* because it would mean all the API
methods have to be const. But I made functions() const.

https://codereview.chromium.org/11359063/diff/9006/ppapi/proxy/ppb_instance_p...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/11359063/diff/9006/ppapi/proxy/ppb_instance_p...
ppapi/proxy/ppb_instance_proxy.cc:345: Resource* new_singleton(NULL);
On 2012/11/21 17:26:27, dmichael wrote:
> How about making this a scoped_refptr? Just so if you or somebody else comes
> along and adds an early return, we won't leak.

Done.

https://codereview.chromium.org/11359063/diff/9006/ppapi/proxy/ppb_instance_p...
ppapi/proxy/ppb_instance_proxy.cc:365: // the cases.
On 2012/11/21 17:26:27, dmichael wrote:
> What about moving the #ifdef to the end, and having something like:
> #else
>     case FLASH_SINGLETON_ID:
>     case FLASH_CLIPBOARD_SINGLETON_ID:
>       NOTREACHED();
> #endif
> 
> ...I really like the compiler warning that points us at pieces of code that
need
> to be updated when we add an enum value.

Done.

https://codereview.chromium.org/11359063/diff/9006/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/11359063/diff/9006/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:321: :
::ppapi::Resource(::ppapi::Resource::Untracked()),
On 2012/11/21 17:26:27, dmichael wrote:
> nit: using ppapi::Resource and using ppapi::thunk::PPB_Gamepad_API might make
> some of this cleaner (it also is more consistent to do it that way in this
file)

Done.

[dmichael (off chromium), 2012-11-21 21:02:38]

Nice, lgtm

[brettw, 2012-11-27 05:55:10]

lgtm

https://codereview.chromium.org/11359063/diff/13010/ppapi/shared_impl/api_id.h
File ppapi/shared_impl/api_id.h (right):

https://codereview.chromium.org/11359063/diff/13010/ppapi/shared_impl/api_id....
ppapi/shared_impl/api_id.h:80: enum SingletonResourceID {
Can you put this in a separate header? As we convert the proxy stuff over, I'd
expect the ApiID to go away.

https://codereview.chromium.org/11359063/diff/13010/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://codereview.chromium.org/11359063/diff/13010/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:255: explicit EnterInstanceAPI(PP_Instance instance) :
EnterBase(),
The style guide either puts everything on one line, or puts the colon on the
next line and each initializer on a separate line.

[raymes, 2012-11-27 17:48:45]

https://codereview.chromium.org/11359063/diff/13010/ppapi/shared_impl/api_id.h
File ppapi/shared_impl/api_id.h (right):

https://codereview.chromium.org/11359063/diff/13010/ppapi/shared_impl/api_id....
ppapi/shared_impl/api_id.h:80: enum SingletonResourceID {
On 2012/11/27 05:55:10, brettw wrote:
> Can you put this in a separate header? As we convert the proxy stuff over, I'd
> expect the ApiID to go away.

Done.

https://codereview.chromium.org/11359063/diff/13010/ppapi/thunk/enter.h
File ppapi/thunk/enter.h (right):

https://codereview.chromium.org/11359063/diff/13010/ppapi/thunk/enter.h#newco...
ppapi/thunk/enter.h:255: explicit EnterInstanceAPI(PP_Instance instance) :
EnterBase(),
On 2012/11/27 05:55:10, brettw wrote:
> The style guide either puts everything on one line, or puts the colon on the
> next line and each initializer on a separate line.

Done.

[commit-bot: I haz the power, 2012-11-27 17:50:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359063/18004

[commit-bot: I haz the power, 2012-11-27 18:00:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359063/11011

[commit-bot: I haz the power, 2012-11-28 15:07:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359063/11011

[commit-bot: I haz the power, 2012-11-28 15:07:52]

Failed to apply patch for ppapi/proxy/ppb_instance_proxy.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/proxy/ppb_instance_proxy.cc
  Hunk #2 FAILED at 332.
  1 out of 2 hunks FAILED -- saving rejects to file
ppapi/proxy/ppb_instance_proxy.cc.rej

Patch:       ppapi/proxy/ppb_instance_proxy.cc
Index: ppapi/proxy/ppb_instance_proxy.cc
diff --git a/ppapi/proxy/ppb_instance_proxy.cc
b/ppapi/proxy/ppb_instance_proxy.cc
index
cb8389051226f958fd05363f56c29527a1849c14..3f2f7f543604c52fd0e9067d70e6b10a57543ba2
100644
--- a/ppapi/proxy/ppb_instance_proxy.cc
+++ b/ppapi/proxy/ppb_instance_proxy.cc
@@ -4,6 +4,7 @@
 
 #include "ppapi/proxy/ppb_instance_proxy.h"
 
+#include "base/memory/ref_counted.h"
 #include "build/build_config.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/pp_time.h"
@@ -331,67 +332,49 @@ thunk::PPB_Flash_API* PPB_Instance_Proxy::GetFlashAPI() {
   return static_cast<PPB_Flash_Proxy*>(ip);
 }
 
-// TODO(raymes): We can most likely cut down this boilerplate for grabbing
-// singleton resource APIs.
-thunk::PPB_Flash_Functions_API* PPB_Instance_Proxy::GetFlashFunctionsAPI(
-    PP_Instance instance) {
-#if !defined(OS_NACL) && !defined(NACL_WIN64)
+Resource* PPB_Instance_Proxy::GetSingletonResource(PP_Instance instance,
+                                                   SingletonResourceID id) {
   InstanceData* data = static_cast<PluginDispatcher*>(dispatcher())->
       GetInstanceData(instance);
-  if (!data)
-    return NULL;
-
-  if (!data->flash_resource.get()) {
-    Connection connection(
-        PluginGlobals::Get()->plugin_proxy_delegate()->GetBrowserSender(),
-        dispatcher());
-    data->flash_resource = new FlashResource(connection, instance);
-  }
-  return data->flash_resource.get();
-#else
-  // Flash functions aren't implemented for nacl.
-  NOTIMPLEMENTED();
-  return NULL;
-#endif  // !defined(OS_NACL) && !defined(NACL_WIN64)
-}
 
-thunk::PPB_Flash_Clipboard_API* PPB_Instance_Proxy::GetFlashClipboardAPI(
-    PP_Instance instance) {
+  InstanceData::SingletonResourceMap::iterator it =
+      data->singleton_resources.find(id);
+  if (it != data->singleton_resources.end())
+    return it->second.get();
+
+  scoped_refptr<Resource> new_singleton;
+  Connection connection(
+      PluginGlobals::Get()->plugin_proxy_delegate()->GetBrowserSender(),
+      dispatcher());
+
+  switch (id) {
+    case GAMEPAD_SINGLETON_ID:
+      new_singleton = new GamepadResource(connection, instance);
+      break;
+// Flash resources aren't needed for NaCl.
 #if !defined(OS_NACL) && !defined(NACL_WIN64)
-  InstanceData* data = static_cast<PluginDispatcher*>(dispatcher())->
-      GetInstanceData(instance);
-  if (!data)
-    return NULL;
-
-  if (!data->flash_clipboard_resource.get()) {
-    Connection connection(
-        PluginGlobals::Get()->plugin_proxy_delegate()->GetBrowserSender(),
-        dispatcher());
-    data->flash_clipboard_resource =
-        new FlashClipboardResource(connection, instance);
-  }
-  return data->flash_clipboard_resource.get();
+    case FLASH_SINGLETON_ID:
+      new_singleton = new FlashResource(connection, instance);
+      break;
+    case FLASH_CLIPBOARD_SINGLETON_ID:
+      new_singleton = new FlashClipboardResource(connection, instance);
+      break;
 #else
-  // Flash functions aren't implemented for nacl.
-  NOTIMPLEMENTED();
-  return NULL;
+    case FLASH_SINGLETON_ID:
+    case FLASH_CLIPBOARD_SINGLETON_ID:
+      NOTREACHED();
+      break;
 #endif  // !defined(OS_NACL) && !defined(NACL_WIN64)
-}
+  }
 
-thunk::PPB_Gamepad_API* PPB_Instance_Proxy::GetGamepadAPI(
-    PP_Instance instance) {
-  InstanceData* data = static_cast<PluginDispatcher*>(dispatcher())->
-      GetInstanceData(instance);
-  if (!data)
+  if (!new_singleton) {
+    // Getting here implies that a constructor is missing in the above switch.
+    NOTREACHED();
     return NULL;
-
-  if (!data->gamepad_resource.get()) {
-    Connection connection(
-        PluginGlobals::Get()->plugin_proxy_delegate()->GetBrowserSender(),
-        dispatcher());
-    data->gamepad_resource = new GamepadResource(connection, instance);
   }
-  return data->gamepad_resource.get();
+
+  data->singleton_resources[id] = new_singleton;
+  return new_singleton.get();
 }
 
 int32_t PPB_Instance_Proxy::RequestInputEvents(PP_Instance instance,

[commit-bot: I haz the power, 2012-11-28 17:42:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359063/4039

[commit-bot: I haz the power, 2012-11-28 18:02:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-28 18:08:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11359063/4045