Add template SyncCall() to PluginResource.

Add template SyncCall() to PluginResource.

BUG=None
TEST=None


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

[yzshen1, 2012-10-10 01:03:27]

Hi, Brett and Raymes.

Would you please take a look at this CL? Thanks!

Brett: high-level idea and OWNER.
Raymes: coding details.

Note:
I introduced the Destination enum, so that we don't need two sets of methods
SyncBrowserCall/SyncRendererCall.
If you think it makes sense, I am going to make similar changes to other methods
in this class, in a separate CL.

[raymes, 2012-10-10 15:15:01]

On 2012/10/10 01:03:27, yzshen1 wrote:
> Hi, Brett and Raymes.
> 
> Would you please take a look at this CL? Thanks!
> 
> Brett: high-level idea and OWNER.
> Raymes: coding details.
> 
> Note:
> I introduced the Destination enum, so that we don't need two sets of methods
> SyncBrowserCall/SyncRendererCall.
> If you think it makes sense, I am going to make similar changes to other
methods
> in this class, in a separate CL.

I only looked at this at the high level. This looks cool. The only thing I don't
like is how much template mess gets put into this file/class. I would prefer
that we find a way to separate it out into a separate file. I think we could
have an Unpack<ReplyMsgClass>(ReplyMsgClass, A*, B*, ...) set of helper
functions in a separate file and the various versions of SyncReplyMsg would just
call that. This would move all the type checking stuff into a separate file and
also allow the logic to be reused for other purposes if desired. What do you
think?

[yzshen1, 2012-10-10 16:52:42]

> I only looked at this at the high level. This looks cool. The only thing I
don't
> like is how much template mess gets put into this file/class. I would prefer
> that we find a way to separate it out into a separate file. I think we could
> have an Unpack<ReplyMsgClass>(ReplyMsgClass, A*, B*, ...) set of helper
> functions in a separate file and the various versions of SyncReplyMsg would
just
> call that. This would move all the type checking stuff into a separate file
and
> also allow the logic to be reused for other purposes if desired. What do you
> think?

Yeah, I think it is a good idea. Unpack<> can also be used by the host side.
(But I haven't found a suitable place for this util file yet. shared_impl/
doesn't depend on ipc/.)

I will make the change.

[yzshen1, 2012-10-10 20:36:27]

Please take another look. Thanks!

[brettw, 2012-10-10 21:58:22]

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
File ppapi/proxy/ppapi_message_utils.h (right):

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:16: // TupleTypeMatch* check whether a tuple
type contains elements of the specified
Can you put the TupleTypeMatch in a nested "internal" namespace? I've done that
in a few other places for these internal implementation stuff.

[yzshen1, 2012-10-10 22:24:22]

Thanks!

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
File ppapi/proxy/ppapi_message_utils.h (right):

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:16: // TupleTypeMatch* check whether a tuple
type contains elements of the specified
On 2012/10/10 21:58:23, brettw wrote:
> Can you put the TupleTypeMatch in a nested "internal" namespace? I've done
that
> in a few other places for these internal implementation stuff.

Done.

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:16: // TupleTypeMatch* check whether a tuple
type contains elements of the specified
On 2012/10/10 21:58:23, brettw wrote:
> Can you put the TupleTypeMatch in a nested "internal" namespace? I've done
that
> in a few other places for these internal implementation stuff.

Done.

[raymes, 2012-10-11 04:45:54]

lgtm

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
File ppapi/proxy/ppapi_message_utils.h (right):

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:5: #ifndef PPAPI_PROXY_PPAPI_MESSAGE_UTILS_H_
Optional: combine this with ppapi/proxy/dispatch_reply_message.h? I'm not really
sure whether or not it's better though and I'm fine keeping them separate.

http://codereview.chromium.org/11028104/diff/5007/ppapi/proxy/ppapi_message_u...
File ppapi/proxy/ppapi_message_utils.h (right):

http://codereview.chromium.org/11028104/diff/5007/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:16: namespace internal {
Just curious: I saw Brett's comment but is there a reason we can't use an
anonymous namespace here?

http://codereview.chromium.org/11028104/diff/5007/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:72: tuple_type_not_match);
optional: maybe tuple_types_dont_match instead

[yzshen1, 2012-10-12 16:47:03]

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
File ppapi/proxy/ppapi_message_utils.h (right):

http://codereview.chromium.org/11028104/diff/5006/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:5: #ifndef PPAPI_PROXY_PPAPI_MESSAGE_UTILS_H_
I think it is better to keep it as a separate file. Because
dispatch_reply_message.h is only used by the proxy; while this file is used by
both the host and the proxy.


On 2012/10/11 04:45:54, raymes wrote:
> Optional: combine this with ppapi/proxy/dispatch_reply_message.h? I'm not
really
> sure whether or not it's better though and I'm fine keeping them separate.

http://codereview.chromium.org/11028104/diff/5007/ppapi/proxy/ppapi_message_u...
File ppapi/proxy/ppapi_message_utils.h (right):

http://codereview.chromium.org/11028104/diff/5007/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:16: namespace internal {
On 2012/10/11 04:45:55, raymes wrote:
> Just curious: I saw Brett's comment but is there a reason we can't use an
> anonymous namespace here?

Anonymous namespace is used to make sure things only visible in a translation
unit.
This file is a header which is going to be included, so it doesn't really help.

Using an internal namespace at least tells the users "this is not part of the
API that I provide".

http://codereview.chromium.org/11028104/diff/5007/ppapi/proxy/ppapi_message_u...
ppapi/proxy/ppapi_message_utils.h:72: tuple_type_not_match);
On 2012/10/11 04:45:55, raymes wrote:
> optional: maybe tuple_types_dont_match instead

Done.

[commit-bot: I haz the power, 2012-10-12 19:04:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11028104/14001

[commit-bot: I haz the power, 2012-10-12 19:19:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
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.

[commit-bot: I haz the power, 2012-10-12 19:57:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11028104/29001

[commit-bot: I haz the power, 2012-10-13 01:43:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-13 04:14:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/11028104/29001

[commit-bot: I haz the power, 2012-10-13 10:16:09]

Change committed as 161749