[PPAPI] Added a new Var subclass, ResourceVar.

[PPAPI] Added a new Var subclass, ResourceVar.

This var type holds PP_Resource objects, and will have vartype
PP_VARTYPE_RESOURCE. Currently, there is no such vartype, so it is not
yet possible to use this class.

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

[Matt Giuca, 2013-08-23 08:41:16]

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h
File ppapi/shared_impl/var.h (right):

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:197: struct PPAPI_SHARED_EXPORT ResourceInfo {
The reason I put this in a separate struct (instead of just putting these fields
in ResourceVar) is because we need the exact same struct in ResourceRawVarData
in the next CL. (It is not possible to include ResourceVar in the
ResourceRawVarData because of its base class.)

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:216: class PPAPI_SHARED_EXPORT ResourceVar : public Var
{
Now I am a bit uncertain about exactly what methods it is appropriate to expose
here. It's possible we won't need all of these and the best policy may be to
only add them as necessary. I'll have more of a think about this on Monday.

[raymes, 2013-08-26 00:06:11]

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h
File ppapi/shared_impl/var.h (right):

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:212: PP_FileSystemType file_system_type;
As discussed could this be an IPC::Message which is the create message instead?

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:216: class PPAPI_SHARED_EXPORT ResourceVar : public Var
{
I would just have a set/get ResourceInfo. A setter may not even be needed?

[yzshen1, 2013-08-26 17:20:46]

Thanks, Matt!

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h
File ppapi/shared_impl/var.h (right):

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:194: // ResourceVar
-----------------------------------------------------------------
Please consider a separate file. (Just as array and dict var.)

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:200: // Pending host resource ID in the renderer. 0 if
there is already a real
nit, but better to be consistent: this is a "resource host" ID, not a "host
resource" ID.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:202: int pending_renderer_id;
how about pending_*host*_id or pending_renderer_host_id?

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:208: // (indicating that there is a pending host
resource).
nit, but better to be consistent: pending "resource host", not "host resource".
(There are other places below)

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:212: PP_FileSystemType file_system_type;
+1 to use some more generic format instead of having specific data fields for
different resources.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:222: void SetPendingId(int pending_renderer_id);
The setters and getters are using different naming format. Is there any reason
for that?

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:230: // Gets the resource ID associated with this var.
"pending host ID", please

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:246: ResourceInfo* mutable_data() { return &data_; }
Maybe the following two signatures look more consistent?

const ResourceInfo* data() const;
ResourceInfo* data();

[Matt Giuca, 2013-08-28 07:12:13]

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h
File ppapi/shared_impl/var.h (right):

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:194: // ResourceVar
-----------------------------------------------------------------
On 2013/08/26 17:20:46, yzshen1 wrote:
> Please consider a separate file. (Just as array and dict var.)

Done.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:197: struct PPAPI_SHARED_EXPORT ResourceInfo {
Gone.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:200: // Pending host resource ID in the renderer. 0 if
there is already a real
Gone.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:202: int pending_renderer_id;
Gone.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:208: // (indicating that there is a pending host
resource).
Gone.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:212: PP_FileSystemType file_system_type;
On 2013/08/26 17:20:46, yzshen1 wrote:
> +1 to use some more generic format instead of having specific data fields for
> different resources.

Done.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:216: class PPAPI_SHARED_EXPORT ResourceVar : public Var
{
On 2013/08/26 00:06:11, raymes wrote:
> I would just have a set/get ResourceInfo. A setter may not even be needed?

Done.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:222: void SetPendingId(int pending_renderer_id);
On 2013/08/26 17:20:46, yzshen1 wrote:
> The setters and getters are using different naming format. Is there any reason
> for that?

Gone.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:230: // Gets the resource ID associated with this var.
Gone.

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:246: ResourceInfo* mutable_data() { return &data_; }
Gone.

But for reference, I'm following the pattern from the protocol buffers C++ API,
which makes a const version that returns a reference, and a "mutable_" non-const
version that returns a pointer. I think this is good because const references
should be preferred over pointers where possible.

https://codereview.chromium.org/23346009/diff/17001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23346009/diff/17001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:8: #include "ipc/ipc_message.h"
Note: This violates a deps rule (shared_impl can't depend upon ipc). I hope to
get this rule changed (see https://codereview.chromium.org/23532006/), but if
not, we'll have to forward-declare IPC::Message, and make it a scoped_ptr.

[yzshen1, 2013-08-28 17:31:58]

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h
File ppapi/shared_impl/var.h (right):

https://codereview.chromium.org/23346009/diff/1/ppapi/shared_impl/var.h#newco...
ppapi/shared_impl/var.h:246: ResourceInfo* mutable_data() { return &data_; }
On 2013/08/28 07:12:14, Matt Giuca wrote:
> Gone.
> 
> But for reference, I'm following the pattern from the protocol buffers C++
API,
> which makes a const version that returns a reference, and a "mutable_"
non-const
> version that returns a pointer. I think this is good because const references
> should be preferred over pointers where possible.

Okay.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.cc (right):

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.cc:14: : creation_message_(creation_message) {}
You should also init resource_id_.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:23: ResourceVar(PP_Resource resource_id);
explicit?

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:27: // plugin-side resource host. Its type
depends on the type of resource.
rule of thumb: "resource host" at the brower/renderer side; "resource" at the
plugin side. there is no "plugin-side" resource host. :)

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:28: ResourceVar(const IPC::Message&
creation_message);
explicit, please.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:34: PP_Resource resource_id() const { return
resource_id_; }
To be more consistent with other code, please consider to use pp_resource() and
pp_resource_.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:51: PP_Resource resource_id_;
Are you planning to use this same ResourceVar class in both the renderer process
and the plugin process?

One thing that you will need to handle is how to keep the resource corresponding
to |resource_id_| alive. When a resource PP_Var is alive, the resource it points
to should stay alive.

At the plugin side, we can hold 1 ref to |resource_id_|. At the host side, we
cannot do so, because the ref count of |resource_id_| is not managed there.
(This isn't true in the proxy/impl old design. But file system is using the new
design now.)

What if a resource var is passed from plugin process to renderer process? One
way I can think up is to make sure that the plugin side holds 1 ref to the
resource var (which holds 1 ref to |resource_id_|) when the resource var is
alive at the renderer side.
But I am okay that we do that in future changes, if we make sure that currently
we only pass newly-created resource var from renderer to plugin.

[Matt Giuca, 2013-08-29 01:44:47]

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.cc (right):

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.cc:14: : creation_message_(creation_message) {}
On 2013/08/28 17:31:58, yzshen1 wrote:
> You should also init resource_id_.

Done.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:23: ResourceVar(PP_Resource resource_id);
On 2013/08/28 17:31:58, yzshen1 wrote:
> explicit?

Done.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:27: // plugin-side resource host. Its type
depends on the type of resource.
On 2013/08/28 17:31:58, yzshen1 wrote:
> rule of thumb: "resource host" at the brower/renderer side; "resource" at the
> plugin side. there is no "plugin-side" resource host. :)

Done.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:28: ResourceVar(const IPC::Message&
creation_message);
On 2013/08/28 17:31:58, yzshen1 wrote:
> explicit, please.

Done.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:34: PP_Resource resource_id() const { return
resource_id_; }
On 2013/08/28 17:31:58, yzshen1 wrote:
> To be more consistent with other code, please consider to use pp_resource()
and
> pp_resource_.

Done.

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:51: PP_Resource resource_id_;
I haven't thought much about the reference counting here, but it doesn't seem
like there'll be a huge problem.

When passing a resource from host to plugin, we create it on the plugin and
increment it to 1. When the user is finished with it, we remove the reference
and it gets freed on both the plugin and host side automatically, correct?

When passing a resource from plugin to host, it will have a reference count of
1. When it arrives on the host side, part of the process of converting to a V8
value will take all the important information from the resource and store it in
the V8 value. The host side will not need to hold a reference to the resource.
Which means the plugin can free the resource when it needs to; the JavaScript
value should remain intact.

Or, is the problem the asynchronous nature of the protocol? What if I pass a
resource to the host, and then decrement its reference, causing it to be deleted
in the host? Is there a possibility that the "delete resource host" message will
arrive before the "post message"?

[yzshen1, 2013-08-30 00:25:26]

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:51: PP_Resource resource_id_;
> When passing a resource from host to plugin, we create it on the plugin and
> increment it to 1. When the user is finished with it, we remove the reference
> and it gets freed on both the plugin and host side automatically, correct?
Correct. This part is relatively easy.


> When passing a resource from plugin to host, it will have a reference count of
> 1. When it arrives on the host side, part of the process of converting to a V8
> value will take all the important information from the resource and store it
in
> the V8 value. The host side will not need to hold a reference to the resource.
> Which means the plugin can free the resource when it needs to; the JavaScript
> value should remain intact.

Correct. (But please also see my comments for your next paragraph.)

My concern is not directly related to passing a object into JS.
It is that all the other kinds of PP_Var are able to live "stably" at the
renderer side. In other words, at the renderer side once you hold a reference to
a PP_Var, you know that the contents it points to will be alive as long as you
want.
It is good to make resource PP_Var behave in the same way.
But maybe this is not a blocking issue, as long as we make sure resource PP_Vars
are used only in posting messages. Just something to keep in mind.

> 
> Or, is the problem the asynchronous nature of the protocol? What if I pass a
> resource to the host, and then decrement its reference, causing it to be
deleted
> in the host? Is there a possibility that the "delete resource host" message
will
> arrive before the "post message"?
No, the messages will arrive in order.
However, if we need to post a task to run on the message loop and the resource
var is needed there, the "delete resource host" message can get in the middle.

[Matt Giuca, 2013-08-30 02:55:19]

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
File ppapi/shared_impl/resource_var.h (right):

https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
ppapi/shared_impl/resource_var.h:51: PP_Resource resource_id_;
Thanks for explaining.

OK it sounds like none of these will be an issue for this CL. Can we proceed
with committing this so I can move on to the next CL? (I have about 4 WIP CLs
lined up behind this one ;))

[yzshen1, 2013-08-30 16:50:24]

On 2013/08/30 02:55:19, Matt Giuca wrote:
>
https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
> File ppapi/shared_impl/resource_var.h (right):
> 
>
https://codereview.chromium.org/23346009/diff/21001/ppapi/shared_impl/resourc...
> ppapi/shared_impl/resource_var.h:51: PP_Resource resource_id_;
> Thanks for explaining.
> 
> OK it sounds like none of these will be an issue for this CL. Can we proceed
> with committing this so I can move on to the next CL? (I have about 4 WIP CLs
> lined up behind this one ;))

LGTM

Please wait for LGTM from Raymes, too.

[raymes, 2013-09-01 23:41:27]

We will still need fields for pending host IDs correct?

[Matt Giuca, 2013-09-02 00:00:02]

On 2013/09/01 23:41:27, raymes wrote:
> We will still need fields for pending host IDs correct?

No, I don't think so. The pending host IDs are encapsulated within the message
(not shown in this CL). It's possible we'll need them for something generic,
independent of the message type, but it hasn't come up so far.

[raymes, 2013-09-02 00:20:50]

lgtm

I think we may need them down the track, but it's easy to add them later.

On Mon, Sep 2, 2013 at 10:00 AM,  <mgiuca@chromium.org> wrote:
> On 2013/09/01 23:41:27, raymes wrote:
>>
>> We will still need fields for pending host IDs correct?
>
>
> No, I don't think so. The pending host IDs are encapsulated within the
> message
> (not shown in this CL). It's possible we'll need them for something generic,
> independent of the message type, but it hasn't come up so far.
>
> https://codereview.chromium.org/23346009/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-02 06:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/23346009/37001

[commit-bot: I haz the power, 2013-09-02 06:34:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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.

[Matt Giuca, 2013-09-03 01:42:39]

I got it working on Win (and hopefully Mac). Raymes, can you take a quick look
at the new changes to ppapi_internal.gyp?

[raymes, 2013-09-03 03:01:51]

lgtm

On Tue, Sep 3, 2013 at 11:42 AM,  <mgiuca@chromium.org> wrote:
> I got it working on Win (and hopefully Mac). Raymes, can you take a quick
> look
> at the new changes to ppapi_internal.gyp?
>
>
> https://codereview.chromium.org/23346009/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-03 03:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/23346009/66001

[commit-bot: I haz the power, 2013-09-03 07:09:46]

Change committed as 220905