Simplfy Pepepr2 bolierplate code.

Add a GetAs<ResourceType> template.
Move all the dependencies of new PP_Resource types into plugin_resource.h.
BUG=none
TEST=none

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

[neb, 2010-06-25 19:53:16]

The idea here is to make getting pointers from ids the same for PPInstance,
PPModule and PPResource.
It also makes adding new Resources less error prone because only
pepper_resource.h needs to be touched.

[darin (slow to review), 2010-06-25 20:42:36]

I like this patch a lot.  Thanks!  Just a suggestion about naming:

http://codereview.chromium.org/2800028/diff/14001/5002
File webkit/glue/plugins/pepper_buffer.cc (right):

http://codereview.chromium.org/2800028/diff/14001/5002#newcode42
webkit/glue/plugins/pepper_buffer.cc:42:
ResourceTracker::FromPPResource<Buffer>(resource));
I think this would read even better if it were:

  scoped_refptr<Buffer> buffer(
      ResourceTracker::GetAs<Buffer>(resource));

That way, the function call reads a bit more like a sentence.

It seems reasonable for this method to not follow the FromPPInstance /
FromPPModule pattern since those methods are called on PluginInstance /
PluginModule and this method is not called on Buffer, URLLoader, URLRequestInfo,
etc.

http://codereview.chromium.org/2800028/diff/14001/5007
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/2800028/diff/14001/5007#newcode47
webkit/glue/plugins/pepper_resource.h:47: private:
nit: kill this "private:" label since it is unnecessary

[neb, 2010-06-25 21:00:15]

http://codereview.chromium.org/2800028/diff/14001/5002
File webkit/glue/plugins/pepper_buffer.cc (right):

http://codereview.chromium.org/2800028/diff/14001/5002#newcode42
webkit/glue/plugins/pepper_buffer.cc:42:
ResourceTracker::FromPPResource<Buffer>(resource));
What do you think then about putting it into Resource class, so as to read:

Resource::GetAs<Buffer>(resource)

On 2010/06/25 20:42:36, darin wrote:
> I think this would read even better if it were:
> 
>   scoped_refptr<Buffer> buffer(
>       ResourceTracker::GetAs<Buffer>(resource));
> 
> That way, the function call reads a bit more like a sentence.
> 
> It seems reasonable for this method to not follow the FromPPInstance /
> FromPPModule pattern since those methods are called on PluginInstance /
> PluginModule and this method is not called on Buffer, URLLoader,
URLRequestInfo,
> etc.

http://codereview.chromium.org/2800028/diff/14001/5007
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/2800028/diff/14001/5007#newcode47
webkit/glue/plugins/pepper_resource.h:47: private:
On 2010/06/25 20:42:36, darin wrote:
> nit: kill this "private:" label since it is unnecessary

Done.

[darin (slow to review), 2010-06-27 03:59:35]

On Fri, Jun 25, 2010 at 2:00 PM, <neb@chromium.org> wrote:

>
> http://codereview.chromium.org/2800028/diff/14001/5002
> File webkit/glue/plugins/pepper_buffer.cc (right):
>
> http://codereview.chromium.org/2800028/diff/14001/5002#newcode42
> webkit/glue/plugins/pepper_buffer.cc:42:
> ResourceTracker::FromPPResource<Buffer>(resource));
> What do you think then about putting it into Resource class, so as to
> read:
>
> Resource::GetAs<Buffer>(resource)


^^^ Even better!

-Darin


>
>
> On 2010/06/25 20:42:36, darin wrote:
>
>> I think this would read even better if it were:
>>
>
>    scoped_refptr<Buffer> buffer(
>>       ResourceTracker::GetAs<Buffer>(resource));
>>
>
>  That way, the function call reads a bit more like a sentence.
>>
>
>  It seems reasonable for this method to not follow the FromPPInstance /
>> FromPPModule pattern since those methods are called on PluginInstance
>>
> /
>
>> PluginModule and this method is not called on Buffer, URLLoader,
>>
> URLRequestInfo,
>
>> etc.
>>
>
> http://codereview.chromium.org/2800028/diff/14001/5007
> File webkit/glue/plugins/pepper_resource.h (right):
>
> http://codereview.chromium.org/2800028/diff/14001/5007#newcode47
> webkit/glue/plugins/pepper_resource.h:47: private:
> On 2010/06/25 20:42:36, darin wrote:
>
>> nit: kill this "private:" label since it is unnecessary
>>
>
> Done.
>
>
> http://codereview.chromium.org/2800028/show
>

[neb, 2010-06-28 02:45:03]

LGTY?

[darin (slow to review), 2010-06-28 15:28:11]

http://codereview.chromium.org/2800028/diff/34001/21002
File webkit/glue/plugins/pepper_buffer.cc (right):

http://codereview.chromium.org/2800028/diff/34001/21002#newcode40
webkit/glue/plugins/pepper_buffer.cc:40: scoped_refptr<Buffer> buffer(
some of these no longer require a line break.  would be nice to unwrap them.

http://codereview.chromium.org/2800028/diff/34001/21011
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/2800028/diff/34001/21011#newcode37
webkit/glue/plugins/pepper_resource.h:37: return scoped_refptr<T>();
nit: these three lines can be condensed to:

  return resource.get() ? resource->Cast<T>() : NULL;

I believe that should work given the scoped_refptr(T*) constructor.

http://codereview.chromium.org/2800028/diff/34001/21011#newcode71
webkit/glue/plugins/pepper_resource.h:71: template <> inline Buffer*
Resource::Cast<Buffer>()
I wonder if a macro wouldn't be nice for readability here:

#define DEFINE_RESOURCE_CAST(Type)                   \
  template <> inline Type* Resource::Cast<Type>() {  \
      return As##Type();                             \
  }

DEFINE_RESOURCE_CAST(Buffer)
DEFINE_RESOURCE_CAST(DeviceContext2D)
DEFINE_RESOURCE_CAST(DirectoryReader)
...
DEFINE_RESOURCE_CAST(URLResponseInfo)

#undef DEFINE_RESOURCE_CAST

http://codereview.chromium.org/2800028/diff/34001/21011#newcode73
webkit/glue/plugins/pepper_resource.h:73: template <> inline DeviceContext2D*
Resource::Cast<DeviceContext2D>()
at the very least, you should use google style...

template <> inline Buffer* Resource::Cast<Buffer>() {
  return AsBuffer();
}

[neb, 2010-06-28 17:42:44]

http://codereview.chromium.org/2800028/diff/34001/21002
File webkit/glue/plugins/pepper_buffer.cc (right):

http://codereview.chromium.org/2800028/diff/34001/21002#newcode40
webkit/glue/plugins/pepper_buffer.cc:40: scoped_refptr<Buffer> buffer(
On 2010/06/28 15:28:11, darin wrote:
> some of these no longer require a line break.  would be nice to unwrap them.

Done in all files.

http://codereview.chromium.org/2800028/diff/34001/21011
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/2800028/diff/34001/21011#newcode37
webkit/glue/plugins/pepper_resource.h:37: return scoped_refptr<T>();

On 2010/06/28 15:28:11, darin wrote:
> nit: these three lines can be condensed to:
> 
>   return resource.get() ? resource->Cast<T>() : NULL;
> 
> I believe that should work given the scoped_refptr(T*) constructor.

Done. Also, scoped_refptr has the void* operator, so that get() is not needed.
void* is also used when bool is needed, so this becomes:

return resource ? resource->Cast<T>() : NULL;

http://codereview.chromium.org/2800028/diff/34001/21011#newcode71
webkit/glue/plugins/pepper_resource.h:71: template <> inline Buffer*
Resource::Cast<Buffer>()
On 2010/06/28 15:28:11, darin wrote:
> I wonder if a macro wouldn't be nice for readability here:
> 
> #define DEFINE_RESOURCE_CAST(Type)                   \
>   template <> inline Type* Resource::Cast<Type>() {  \
>       return As##Type();                             \
>   }
> 
> DEFINE_RESOURCE_CAST(Buffer)
> DEFINE_RESOURCE_CAST(DeviceContext2D)
> DEFINE_RESOURCE_CAST(DirectoryReader)
> ...
> DEFINE_RESOURCE_CAST(URLResponseInfo)
> 
> #undef DEFINE_RESOURCE_CAST

Done.

http://codereview.chromium.org/2800028/diff/34001/21011#newcode73
webkit/glue/plugins/pepper_resource.h:73: template <> inline DeviceContext2D*
Resource::Cast<DeviceContext2D>()

On 2010/06/28 15:28:11, darin wrote:
> at the very least, you should use google style...
> 
> template <> inline Buffer* Resource::Cast<Buffer>() {
>   return AsBuffer();
> }

I did the previous suggestion.

[darin (slow to review), 2010-06-28 19:57:07]

LGTM, thanks!

http://codereview.chromium.org/2800028/diff/19002/10013
File webkit/glue/plugins/pepper_resource.h (right):

http://codereview.chromium.org/2800028/diff/19002/10013#newcode86
webkit/glue/plugins/pepper_resource.h:86: }  // namespace pepper
nit: add a new line above the close of the namespace