PPAPI/NaCl: Move handle extraction code to ppapi/proxy

PPAPI/NaCl: Move handle extraction code to ppapi/proxy

BUG=165201
TEST=
TBR=jln@chromium.org

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

[dmichael (off chromium), 2013-01-15 20:28:06]

mseaborn for chrome/nacl
bbudge & teravest for the rest

[teravest, 2013-01-15 21:14:34]

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
File ppapi/proxy/handle_converter.h (right):

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
ppapi/proxy/handle_converter.h:37: bool ConvertNativeHandlesToPosix(const
IPC::Message& msg,
It feels gross to me to use a scoped_ptr<> as an output type.

It might not be worth the extra work, but I wonder if we should roll (handles,
new_msg_ptr) into a new type, that can also indicate whether we had to create a
new message.

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
ppapi/proxy/handle_converter.h:41: // |SendingSyncMessage| allows
HandleConverter to know what sync messages are
The name here is a little funny. Even RegisterSyncMessage might be better.

[bbudge, 2013-01-15 22:20:25]

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
File ppapi/proxy/handle_converter.h (right):

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
ppapi/proxy/handle_converter.h:41: // |SendingSyncMessage| allows
HandleConverter to know what sync messages are
On 2013/01/15 21:14:34, teravest wrote:
> The name here is a little funny. Even RegisterSyncMessage might be better. 

+1 or even RegisterSyncMessageForReply.

[dmichael (off chromium), 2013-01-15 23:18:59]

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
File ppapi/proxy/handle_converter.h (right):

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
ppapi/proxy/handle_converter.h:37: bool ConvertNativeHandlesToPosix(const
IPC::Message& msg,
On 2013/01/15 21:14:34, teravest wrote:
> It feels gross to me to use a scoped_ptr<> as an output type.
> 
> It might not be worth the extra work, but I wonder if we should roll (handles,
> new_msg_ptr) into a new type, that can also indicate whether we had to create
a
> new message.
I struggled with a good way to do this. I also considered using scoped_ptr for
the return type, and making |success| an out-param.

Making a new type seems kind of ugly; extra code + a bool and a scoped_ptr can
have 4 states, only two of which are valid. I'd kind of rather just use
scoped_ptr if I can.

Alternately...  how do you guys feel about just always rewriting the method?
Apparently the performance is good enough on Windows, where we care the most...

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
ppapi/proxy/handle_converter.h:41: // |SendingSyncMessage| allows
HandleConverter to know what sync messages are
On 2013/01/15 22:20:25, bbudge1 wrote:
> On 2013/01/15 21:14:34, teravest wrote:
> > The name here is a little funny. Even RegisterSyncMessage might be better. 
> 
> +1 or even RegisterSyncMessageForReply.

I didn't like it either :-)

Thanks for the good suggestions.

[dmichael (off chromium), 2013-01-15 23:47:06]

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
File ppapi/proxy/handle_converter.h (right):

https://codereview.chromium.org/11894003/diff/6001/ppapi/proxy/handle_convert...
ppapi/proxy/handle_converter.h:37: bool ConvertNativeHandlesToPosix(const
IPC::Message& msg,
On 2013/01/15 23:18:59, dmichael wrote:
> On 2013/01/15 21:14:34, teravest wrote:
> > It feels gross to me to use a scoped_ptr<> as an output type.
> > 
> > It might not be worth the extra work, but I wonder if we should roll
(handles,
> > new_msg_ptr) into a new type, that can also indicate whether we had to
create
> a
> > new message.
> I struggled with a good way to do this. I also considered using scoped_ptr for
> the return type, and making |success| an out-param.
> 
> Making a new type seems kind of ugly; extra code + a bool and a scoped_ptr can
> have 4 states, only two of which are valid. I'd kind of rather just use
> scoped_ptr if I can.
> 
> Alternately...  how do you guys feel about just always rewriting the method?
> Apparently the performance is good enough on Windows, where we care the
most...

FWIW I checked, and scoped_ptr<Foo>* is not without precedent:
https://code.google.com/p/chromium/source/search?q=scoped_ptr<.*>*

[teravest, 2013-01-16 16:33:10]

lgtm

I guess scoped_ptr<Foo>* is fine, it just feels weird to me.
Personally, I'd rather see Foo**, but I don't feel too strongly.

[bbudge, 2013-01-16 17:47:28]

lgtm w/nit

https://codereview.chromium.org/11894003/diff/12001/ppapi/proxy/handle_conver...
File ppapi/proxy/handle_converter.h (right):

https://codereview.chromium.org/11894003/diff/12001/ppapi/proxy/handle_conver...
ppapi/proxy/handle_converter.h:41: // |SendingSyncMessage| allows
HandleConverter to know what sync messages are
Method name doesn't match comment

[Mark Seaborn, 2013-01-16 17:51:01]

LGTM for chrome/nacl

[dmichael (off chromium), 2013-01-16 18:34:31]

+jln TBR for ppapi_messages.h: I didn't change messages. I only added an include
because I moved SerializedHandle out of serialized_structs.h and in to its own
file.

I'm going to stick with scoped_ptr<IPC::Message>*; We prefer scoped_ptr almost
any time we can in Chromium, so it's not a big imposition to have it in the API,
I think. Using IPC::Message** would make the caller code pretty awkward to get
it to do the right thing.

[jln (very slow on Chromium), 2013-01-16 18:36:02]

ppapi_messages.h lgtm

[commit-bot: I haz the power, 2013-01-16 18:39:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/11894003/23019

[commit-bot: I haz the power, 2013-01-16 21:38:59]

Retried try job too often on win7_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-16 22:15:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/11894003/23019

[commit-bot: I haz the power, 2013-01-16 23:23:54]

Change committed as 177257