Add a unified resource tracker shared between the proxy and the impl.

Add a unified resource tracker shared between the proxy and the impl.

This renames the old ResourceObjectBase to Resource and removes the old
PluginResource. It moves the resource tracker from the impl to the
shared_impl, and makes the proxy use it.

Some things become a little less neat because there's no proxy resource base
class. In particular GetDispatcher() is now gone. I considered whether to
add a helper base class that provides this function, but decided against it
and had individual resource classes implement this when their implementation
would find it useful. This is because ultimately I want more of this
functionality to move into the shared_impl, and it's easier to do that if
there are fewer proxy-specific things in the resources.

This changes the way that plugins are added to the tracker. Previously they
would only be in the tracker if the plugin had a reference to them, although
they could be alive if the impl had a scoped_ptr referencing an object. This
actually has the bug that if we then give the resource back to the plugin,
it wouldn't be refcounted properly and everything would get confused.

Now the tracker tracks all live resource objects whether or not the plugin
has a ref. This works basically like the var tracker (it would be nice if
the var and resource trackers shared more code, but that would further
complicate this already overcomplicated patch). The resource tracker takes an
extra ref whenever the plugin has one or more, and otherwise just tracks live
resources.

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

[brettw, 2011-08-15 19:32:54]

Sorry...

[dmichael (off chromium), 2011-08-17 16:28:07]

Still looking, but thought I'd publish what I've found so far. Mostly just
optional nits.

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

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_audio_proxy...
ppapi/proxy/ppb_audio_proxy.cc:184: GetReference();
optional nit: I think this would be more readable in 2 expressions. (Downside
being it would be inconsistent with what you did in the other files, which fit
in 1 line and look fine).

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_broker_prox...
File ppapi/proxy/ppb_broker_proxy.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_broker_prox...
ppapi/proxy/ppb_broker_proxy.cc:118: bool success =
PluginDispatcher::GetForResource(this)->Send(
optional nit: Most places I've seen so far, you're using GetForInstance instead,
even when you have the resource. Is there any reason for the inconsistency? Not
a big deal, but it would seem preferable to always use one or the other in
similar contexts, and maybe remove one of them if possible.

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_file_...
File ppapi/proxy/ppb_flash_file_proxy.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_file_...
ppapi/proxy/ppb_flash_file_proxy.cc:636:
PluginDispatcher::GetForInstance(file_ref->pp_instance());
Here's one of those places you could've used GetForResource but didn't.

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_file_...
ppapi/proxy/ppb_flash_file_proxy.cc:657:
PluginDispatcher::GetForInstance(file_ref->pp_instance());
ditto

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_tcp_s...
File ppapi/proxy/ppb_flash_tcp_socket_proxy.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_tcp_s...
ppapi/proxy/ppb_flash_tcp_socket_proxy.cc:107: }
optional nit: You didn't do this anywhere else...  you just used GetForResource
or GetForInstance explicitly. Might make sense to just do the same thing here as
everywhere else?

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_font_proxy.h
File ppapi/proxy/ppb_font_proxy.h (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_font_proxy....
ppapi/proxy/ppb_font_proxy.h:11: #include "base/scoped_ptr.h"
These 3 look a little out-of-place here. Are these necessary? I don't see them
used in the header.

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_graphics_2d...
File ppapi/proxy/ppb_graphics_2d_proxy.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_graphics_2d...
ppapi/proxy/ppb_graphics_2d_proxy.cc:105: //  return;
Since you're in the neighborhood, can you just delete this dead code? Or
uncomment...  they seem like good checks.

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
File ppapi/shared_impl/resource_tracker.h (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.h:25: // The returned pointer will be NULL if
there is no resource.
Maybe note that the reference count is unaffected?

[brettw, 2011-08-17 17:16:25]

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

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_audio_proxy...
ppapi/proxy/ppb_audio_proxy.cc:184: GetReference();
I want to be consistent so people that copy & paste get the same thing. I
changed the wrapping a bit to make it more clear.

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_tcp_s...
File ppapi/proxy/ppb_flash_tcp_socket_proxy.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_flash_tcp_s...
ppapi/proxy/ppb_flash_tcp_socket_proxy.cc:107: }
I did this in a few places where we used GetDispatcher() a lot so the code is
overall shorter having a helper function.

[dmichael (off chromium), 2011-08-17 19:02:54]

More comments...  still going.

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_video_decod...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/proxy/ppb_video_decod...
ppapi/proxy/ppb_video_decoder_proxy.cc:239: scoped_refptr<VideoDecoder>
decoder(new VideoDecoder(result));
Why are you using scoped_refptr here but not elsewhere? Please either change it
to match the others, or if this is necessary here, add a comment explaining why.

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource.h
File ppapi/shared_impl/resource.h (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource....
ppapi/shared_impl/resource.h:16: #define FOR_ALL_PPAPI_RESOURCE_APIS(F) \
I guess it's obvious, but it maybe good to comment that any new resources need
to be added here.

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource....
ppapi/shared_impl/resource.h:65: Resource(PP_Instance instance);
explicit

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource....
ppapi/shared_impl/resource.h:70: Resource(const HostResource& host_resource);
explicit

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource....
ppapi/shared_impl/resource.h:109: // Dynamic casting for this object. Returns
the pointer to the given type if
I think you meant to delete this line^^^^

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
File ppapi/shared_impl/resource_tracker.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:61: // When we go from 0 to 1 plugin ref
count, free the additional "real" ref
1 to 0

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:66: return true;
The return value seems to be true if the resource existed and the ref-count was
successfully decremented. It could easily be interpreted to mean 'the last
refcount was removed and the resource was deleted'. If the return is never used,
maybe just make it return void? If this behavior is required, then please
document it in the header.

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:81: // Due to the infrastructure of some
tests, the instance is uyregistered
uyregistered->unregistered

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:140: // If the plugin manages to create
too many resources, don't do crazy stuff.
Now that we're generating a resource for each input event (including, e.g.,
mouse move), and since we only have 29 bits to work with, it's getting almost
plausible that a long-running plugin would exhaust resources. Back of the
envelope says that at 1000/sec (I know, probably unlikely) it would take just
over 6 days.

Not for this CL...  but I think we might want to consider a more careful ID
assignment soon.

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
File ppapi/shared_impl/resource_tracker_unittest.cc (right):

http://codereview.chromium.org/7629017/diff/10001/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker_unittest.cc:144: EXPECT_EQ(0,
mock_resource_alive_count);
optional: For this and the previous test, you could also check that GetResource
returns NULL after the instance was deleted.

[dmichael (off chromium), 2011-08-17 20:33:44]

I'm done for now... only a couple more comments.

http://codereview.chromium.org/7629017/diff/10001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.h (right):

http://codereview.chromium.org/7629017/diff/10001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.h:42: // Resource overrides.
Don't need both 'Resource overrides' comments

http://codereview.chromium.org/7629017/diff/10001/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource.h (right):

http://codereview.chromium.org/7629017/diff/10001/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource.h:17: class Resource : public ::ppapi::Resource {
Would it be worthwhile to rename this HostResource? It could be slightly
confusing as-is when host-side implementations refer to 'Resource'.

[brettw, 2011-08-17 20:43:18]

I think I addressed everything.

http://codereview.chromium.org/7629017/diff/10001/webkit/plugins/ppapi/resour...
File webkit/plugins/ppapi/resource.h (right):

http://codereview.chromium.org/7629017/diff/10001/webkit/plugins/ppapi/resour...
webkit/plugins/ppapi/resource.h:17: class Resource : public ::ppapi::Resource {
I'm going to be removing this altogether shortly in a followup. If that doesn't
work I'll rename it.

[dmichael (off chromium), 2011-08-17 21:35:50]

just some nits left. o/w LGTM

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

http://codereview.chromium.org/7629017/diff/14005/ppapi/proxy/ppb_audio_proxy...
ppapi/proxy/ppb_audio_proxy.cc:90:
PluginDispatcher::GetForInstance(pp_instance())->Send(
Sorry, didn't mean to imply that I marked all the places that could use
GetForResource. This is another.

http://codereview.chromium.org/7629017/diff/14005/ppapi/proxy/ppb_audio_proxy...
ppapi/proxy/ppb_audio_proxy.cc:99:
PluginDispatcher::GetForInstance(pp_instance())->Send(
ditto

http://codereview.chromium.org/7629017/diff/14005/ppapi/proxy/ppb_font_proxy.h
File ppapi/proxy/ppb_font_proxy.h (right):

http://codereview.chromium.org/7629017/diff/14005/ppapi/proxy/ppb_font_proxy....
ppapi/proxy/ppb_font_proxy.h:9: #include "base/memory/ref_counted.h"
So callback and ref_counted are necessary? I don't see them used.

http://codereview.chromium.org/7629017/diff/14005/ppapi/shared_impl/resource_...
File ppapi/shared_impl/resource_tracker.cc (right):

http://codereview.chromium.org/7629017/diff/14005/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:61: // When we go from 0 to 1 plugin ref
count, free the additional "real" ref
0 to 1 -> 1 to 0

http://codereview.chromium.org/7629017/diff/14005/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:66: return;
nit: the last return is unnecessary here and in AddRefResource

http://codereview.chromium.org/7629017/diff/14005/ppapi/shared_impl/resource_...
ppapi/shared_impl/resource_tracker.cc:81: // Due to the infrastructure of some
tests, the instance is uyregistered
uyregistered->unregistered