Remove webkit::ppapi::Resource.

Remove webkit::ppapi::Resource.

This makes all resource _impl's derive directly from ppapi::Resource so we
can share code better. This means removing PluginInstances and converting
them to PP_Instances.

This adds a new ResourceHelper static class to help in the conversion of
resources to PluginInstances for the _impl classes.

Overall the new code is in general slightly worse than the old because using
the ResourceHelper is more annoying than just calling instance() on the old
webkit::ppapi::Resource object. However, I'm hoping that, because this will
allow us to move more code into shared_impl and reduce duplicate logic, it
will eventually have a net positive effect.

This also adds a ScopedPPResource class that works just like a scoped_refptr. We need this in a few places. Most of the places that used the old ScopedResourceId class could be removed now since resources have PP_Resource IDs generated even when there's no plugin reference (this didn't use to be the case) so we can pass resources as input params to the plugin even when there's no plugin ref, as long as there's a scoped_refptr to the Resource.

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

[brettw, 2011-08-22 22:54:14]

ping

[viettrungluu, 2011-08-22 23:19:46]

Part 1

http://codereview.chromium.org/7669055/diff/3001/chrome/renderer/chrome_ppb_p...
File chrome/renderer/chrome_ppb_pdf_impl.cc (right):

http://codereview.chromium.org/7669055/diff/3001/chrome/renderer/chrome_ppb_p...
chrome/renderer/chrome_ppb_pdf_impl.cc:152: if
(!webkit::ppapi::ResourceTracker::Get()->GetInstance(instance))
I really wish we had a consistent convention for instance_id/instance (and
distinguishing PluginInstances and PP_Instances). You're making this file even
less consistent. :(

http://codereview.chromium.org/7669055/diff/3001/ppapi/shared_impl/resource_t...
File ppapi/shared_impl/resource_tracker.cc (right):

http://codereview.chromium.org/7669055/diff/3001/ppapi/shared_impl/resource_t...
ppapi/shared_impl/resource_tracker.cc:150:
CHECK(instance_map_.find(object->pp_instance()) != instance_map_.end());
Hrm, this could conceivably be a fairly hot method, so it'd probably be better
to keep the result of the .find() and then use it below, rather than doing two
searches.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/mock_re...
File webkit/plugins/ppapi/mock_resource.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/mock_re...
webkit/plugins/ppapi/mock_resource.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011, apparently.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_aud...
File webkit/plugins/ppapi/ppb_audio_impl.cc (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_aud...
webkit/plugins/ppapi/ppb_audio_impl.cc:82: PP_Resource config_id,
I think you should rename s/config_id/config/.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_aud...
webkit/plugins/ppapi/ppb_audio_impl.cc:95: bool PPB_Audio_Impl::Init(PP_Resource
config_id,
"

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_aud...
webkit/plugins/ppapi/ppb_audio_impl.cc:142: int32_t
PPB_Audio_Impl::OpenTrusted(PP_Resource config_id,
"

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_bro...
File webkit/plugins/ppapi/ppb_broker_impl.cc (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_bro...
webkit/plugins/ppapi/ppb_broker_impl.cc:69: PluginInstance* instance =
ResourceHelper::GetPluginInstance(this);
Maybe we should use plugin_instance for PluginInstances?

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_ima...
File webkit/plugins/ppapi/ppb_image_data_impl.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_ima...
webkit/plugins/ppapi/ppb_image_data_impl.h:131: #endif  //
WEBKIT_PLUGINS_PPAPI_PPB_IMAGE_DATA_IMPL_H_
I got here before getting bored.

[brettw, 2011-08-23 17:16:28]

Comments addressed, new snap up, PITA.

http://codereview.chromium.org/7669055/diff/3001/chrome/renderer/chrome_ppb_p...
File chrome/renderer/chrome_ppb_pdf_impl.cc (right):

http://codereview.chromium.org/7669055/diff/3001/chrome/renderer/chrome_ppb_p...
chrome/renderer/chrome_ppb_pdf_impl.cc:152: if
(!webkit::ppapi::ResourceTracker::Get()->GetInstance(instance))
This patch generally removes uses of PluginInstance. In the code, I just use
"instance" to refer to the PP_Instance, since there is nothing else, which is
why I did the same here. Since this class (unusually) also uses PluginInstance a
lot, I've put this back to instance_id.

[viettrungluu, 2011-08-23 17:28:13]

Oops, continued from where I left off, didn't notice the new patch set.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_scr...
File webkit/plugins/ppapi/ppb_scrollbar_impl.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_scr...
webkit/plugins/ppapi/ppb_scrollbar_impl.h:51: PPB_Scrollbar_Impl(PP_Instance
instance);
Could you add an "explicit" as a drive-by?

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (left):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Cough.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:184: ScopedResourceId
resource(this);
I assume the ScopedResourceId was merely used to get the resource ID, and not to
ensure the lifetime of the resource, right?

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_layer_impl.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_layer_impl.h:10: #include
"ppapi/shared_impl/resource.h"
nit: alphabetical order

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_wid...
File webkit/plugins/ppapi/ppb_widget_impl.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_wid...
webkit/plugins/ppapi/ppb_widget_impl.h:12: #include
"ppapi/shared_impl/resource.h"
alphabetical order

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_creation_impl.cc (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_creation_impl.cc:205: return (new
PPB_InputEvent_Impl(instance, data))->GetReference();
I don't know how I feel about the use of the result of |new ...| as the LHS to
->. I guess it's not illegal.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_helper.cc (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_helper.cc:26: return
GetPluginInstance(resource)->module();
Crashes if the resource has outlived its instance.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_helper.cc:31: return
GetPluginInstance(resource)->delegate();
"

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_helper.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_helper.h:34: // outlived its instance.
I -- or, better, you -- should go back and check that everything which uses this
safely handles the NULL return value.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_helper.h:39: static PluginDelegate*
GetPluginDelegate(const ::ppapi::Resource* resource);
"

[brettw, 2011-08-23 23:16:21]

New snap up.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (left):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:184: ScopedResourceId
resource(this);
Yes, you used to need the ScopedResourceId if the plugin didn't previously have
a ref, but now the pp_resource() is always valid.

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_helper.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_helper.h:34: // outlived its instance.
Most of the places I didn't check because I think they're safe. But maybe we
should do this check anyway so we're consistent about always checking. I'll do
that.

[brettw, 2011-08-23 23:16:51]

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_helper.h (right):

http://codereview.chromium.org/7669055/diff/3001/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_helper.h:34: // outlived its instance.
In the most recent patch I audited all callers and added checking, even when I
thought it would be OK.

[viettrungluu, 2011-08-24 01:20:10]

LGTM