[PPAPI] ResourceVar now holds a pending renderer and browser host ID.

[PPAPI] ResourceVar now holds a pending renderer and browser host ID.

It is likely that resources will be backed by a host in the renderer or
browser, or both. Therefore, HostResourceVar should provide a consistent
way to tell the plugin the pending host IDs.

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

[Matt Giuca, 2013-09-24 10:06:01]

Hi,

Firstly: I'm not sure whether I should put all three of you guys as reviewers
every time. It might save time if I only add one or two of you (I suppose raymes
since he's working closely with me, and one other). I'm about to send out about
half a dozen CLs in the next few days about this, so would you prefer me to
split it up between reviewers? Or just include everybody and see who wants to
review each one?

Some rationale for this one: the reason why HostResourceVar takes the renderer
ID as a constructor input, but has a separate setter for the browser ID, is that
the renderer ID is known at the time you construct the HostResourceVar (in the
renderer), whereas you will only get the browser ID later in an async response
from the browser. This is also the reason why the pending host IDs cannot be
included as part of the creation message, because you can't modify the message
one it has been created (that explains the browser ID, and the renderer ID has
its own field for consistency).

[yzshen1, 2013-09-24 16:50:09]

On 2013/09/24 10:06:01, Matt Giuca wrote:
> Hi,
> 
> Firstly: I'm not sure whether I should put all three of you guys as reviewers
> every time. It might save time if I only add one or two of you (I suppose
raymes
> since he's working closely with me, and one other). I'm about to send out
about
> half a dozen CLs in the next few days about this, so would you prefer me to
> split it up between reviewers? Or just include everybody and see who wants to
> review each one?

I think it should be okay to split the reviewing work.
And it would be nice if you could send all of us a brief mail/doc to talk about
the general plan of these CLs.
Because every CL only does part of the work, it may not be easy to give
suggestions about some high-level design decisions.

What do you think?

> 
> Some rationale for this one: the reason why HostResourceVar takes the renderer
> ID as a constructor input, but has a separate setter for the browser ID, is
that
> the renderer ID is known at the time you construct the HostResourceVar (in the
> renderer), whereas you will only get the browser ID later in an async response
> from the browser. This is also the reason why the pending host IDs cannot be
> included as part of the creation message, because you can't modify the message
> one it has been created (that explains the browser ID, and the renderer ID has
> its own field for consistency).

[yzshen1, 2013-09-24 17:03:30]

LGTM with one nit

https://codereview.chromium.org/24196005/diff/1/content/renderer/pepper/host_...
File content/renderer/pepper/host_resource_var.h (right):

https://codereview.chromium.org/24196005/diff/1/content/renderer/pepper/host_...
content/renderer/pepper/host_resource_var.h:41: void SetPendingBrowserHostId(int
id) {
nit: please use set_pending_browser_host_id().

(GetPendingBrowserHostID is virtual, so that one is okay.)

[raymes, 2013-09-24 23:32:22]

On 2013/09/24 16:50:09, yzshen1 wrote:
> On 2013/09/24 10:06:01, Matt Giuca wrote:
> > Hi,
> > 
> > Firstly: I'm not sure whether I should put all three of you guys as
reviewers
> > every time. It might save time if I only add one or two of you (I suppose
> raymes
> > since he's working closely with me, and one other). I'm about to send out
> about
> > half a dozen CLs in the next few days about this, so would you prefer me to
> > split it up between reviewers? Or just include everybody and see who wants
to
> > review each one?
> 
> I think it should be okay to split the reviewing work.
> And it would be nice if you could send all of us a brief mail/doc to talk
about
> the general plan of these CLs.
> Because every CL only does part of the work, it may not be easy to give
> suggestions about some high-level design decisions.
> 
> What do you think?
> 
> > 
> > Some rationale for this one: the reason why HostResourceVar takes the
renderer
> > ID as a constructor input, but has a separate setter for the browser ID, is
> that
> > the renderer ID is known at the time you construct the HostResourceVar (in
the
> > renderer), whereas you will only get the browser ID later in an async
response
> > from the browser. This is also the reason why the pending host IDs cannot be
> > included as part of the creation message, because you can't modify the
message
> > one it has been created (that explains the browser ID, and the renderer ID
has
> > its own field for consistency).

I agree with Yuzhu. 1 or 2 reviewers is fine.

Yuzhu: Matt has sent out some docs/mail about this. Did you need more detail
about something in particular?

[raymes, 2013-09-24 23:37:31]

lgtm

[Matt Giuca, 2013-09-25 00:59:38]

Mailed out the document requested to interested parties.

https://codereview.chromium.org/24196005/diff/1/content/renderer/pepper/host_...
File content/renderer/pepper/host_resource_var.h (right):

https://codereview.chromium.org/24196005/diff/1/content/renderer/pepper/host_...
content/renderer/pepper/host_resource_var.h:41: void SetPendingBrowserHostId(int
id) {
On 2013/09/24 17:03:30, yzshen1 wrote:
> nit: please use set_pending_browser_host_id().
> 
> (GetPendingBrowserHostID is virtual, so that one is okay.)

Done.

[commit-bot: I haz the power, 2013-09-25 01:03:47]

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

[yzshen, 2013-09-25 05:10:00]

Hi,

I think Matt's doc is quite clear. Thanks for writing that!


On Tue, Sep 24, 2013 at 4:32 PM, <raymes@chromium.org> wrote:

> On 2013/09/24 16:50:09, yzshen1 wrote:
>
>> On 2013/09/24 10:06:01, Matt Giuca wrote:
>> > Hi,
>> >
>> > Firstly: I'm not sure whether I should put all three of you guys as
>>
> reviewers
>
>> > every time. It might save time if I only add one or two of you (I
>> suppose
>> raymes
>> > since he's working closely with me, and one other). I'm about to send
>> out
>> about
>> > half a dozen CLs in the next few days about this, so would you prefer
>> me to
>> > split it up between reviewers? Or just include everybody and see who
>> wants
>>
> to
>
>> > review each one?
>>
>
>  I think it should be okay to split the reviewing work.
>> And it would be nice if you could send all of us a brief mail/doc to talk
>>
> about
>
>> the general plan of these CLs.
>> Because every CL only does part of the work, it may not be easy to give
>> suggestions about some high-level design decisions.
>>
>
>  What do you think?
>>
>
>  >
>> > Some rationale for this one: the reason why HostResourceVar takes the
>>
> renderer
>
>> > ID as a constructor input, but has a separate setter for the browser
>> ID, is
>> that
>> > the renderer ID is known at the time you construct the HostResourceVar
>> (in
>>
> the
>
>> > renderer), whereas you will only get the browser ID later in an async
>>
> response
>
>> > from the browser. This is also the reason why the pending host IDs
>> cannot be
>> > included as part of the creation message, because you can't modify the
>>
> message
>
>> > one it has been created (that explains the browser ID, and the renderer
>> ID
>>
> has
>
>> > its own field for consistency).
>>
>
> I agree with Yuzhu. 1 or 2 reviewers is fine.
>
> Yuzhu: Matt has sent out some docs/mail about this. Did you need more
> detail
> about something in particular?
>
>
https://codereview.chromium.**org/24196005/<https://codereview.chromium.org/2...
>



-- 
Best regards,
Yuzhu Shen.

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-25 07:20:52]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[dmichael (off chromium), 2013-09-25 21:25:15]

lgtm

https://codereview.chromium.org/24196005/diff/8001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/24196005/diff/8001/ppapi/proxy/raw_var_data.c...
ppapi/proxy/raw_var_data.cc:716:
m->WriteInt(static_cast<int>(pending_browser_host_id_));
These are already int, right? So no need to static_cast.

[Matt Giuca, 2013-09-25 23:57:00]

https://codereview.chromium.org/24196005/diff/8001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/24196005/diff/8001/ppapi/proxy/raw_var_data.c...
ppapi/proxy/raw_var_data.cc:716:
m->WriteInt(static_cast<int>(pending_browser_host_id_));
On 2013/09/25 21:25:15, dmichael wrote:
> These are already int, right? So no need to static_cast.

Done. Good catch.

[commit-bot: I haz the power, 2013-09-26 01:13:04]

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

[commit-bot: I haz the power, 2013-09-26 07:38:35]

Change committed as 225380