use variadic macros/templates in IPC message implementation

use variadic macros/templates in IPC message implementation

TBR=jam@chromium.org, mseaborn@chromium.org

Committed: https://crrev.com/8a5190449d48e06efa581390426dfa3bb6750f4c
Cr-Commit-Position: refs/heads/master@{#374316}

[mdempsky, 2015-12-17 08:54:36]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 08:55:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/20001

[commit-bot: I haz the power, 2015-12-17 09:12:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 09:12:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mdempsky, 2015-12-17 09:39:34]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 09:39:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/60001

[commit-bot: I haz the power, 2015-12-17 09:41:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 09:41:45]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mdempsky, 2015-12-17 09:48:11]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 09:49:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/80001

[commit-bot: I haz the power, 2015-12-17 09:50:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 09:50:56]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mdempsky, 2015-12-17 09:56:25]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 09:56:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/100001

[commit-bot: I haz the power, 2015-12-17 10:04:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 10:04:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mdempsky, 2015-12-17 14:36:10]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 14:36:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/280001

[commit-bot: I haz the power, 2015-12-17 15:11:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 15:11:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mdempsky, 2015-12-17 17:24:12]

The CQ bit was checked by mdempsky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 17:24:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/320001

[mdempsky, 2015-12-17 18:13:05]

Description was changed from

==========
use variadic macros in ipc implementation
==========

to

==========
use variadic macros/templates in IPC message implementation
==========

[danakj, 2015-12-19 00:15:04]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-12-19 00:15:06]

So.. I tried to digest this but it's really hard. The templates can be parsed
eventually though some more comments would help. The macros are nearly
impenetrable, but maybe they're just less familiar, I dunno.

It left me wondering why our IPC code looks like this.

Maybe refactoring this stuff would be more tractable if this work was split up
into much smaller pieces.

Maybe we could do better with C++11 than macro-template adventures.

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:40: // MSVC emits warning C4003 ("not enough actual
parameters for macro
True in MSVC 2015?

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:50: // Issue 3.
One wonders where Issue 2 is..

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:61: #define EXPORT_TEMPLATE_DECLARE(export) \
I think this needs a bunch more comments. I've managed to trace through to
EXPORT_TEMPLATE_STYLE_3 and forget most of the stack above me. I think this
could be documented in a way so that the connections between each are described
and one can follow each paragraph of templates and see how it's used from the
comments.

Or maybe more restructuring, I'm not sure.

It would be nice if it didn't require drawing diagrams on paper to read this
file.

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:94: TEST_##want, EXPORT_TEMPLATE_STYLE(export, ),
export), \
Same even for the tests. This ends up as TEST_DEFAULT, which is passed to the
INVOKE..

https://codereview.chromium.org/1532053002/diff/440001/content/renderer/rende...
File content/renderer/render_thread_impl_browsertest.cc (right):

https://codereview.chromium.org/1532053002/diff/440001/content/renderer/rende...
content/renderer/render_thread_impl_browsertest.cc:30: // TODO(mdempsky): Fix
properly.
Can you leave more context, what is properly?

https://codereview.chromium.org/1532053002/diff/440001/ipc/attachment_broker_...
File ipc/attachment_broker_messages.h (right):

https://codereview.chromium.org/1532053002/diff/440001/ipc/attachment_broker_...
ipc/attachment_broker_messages.h:21: #undef IPC_MESSAGE_EXPORT
Why is this moved?

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (left):

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:228: IPC_MESSAGE_DECL(EMPTY, CONTROL, msg_class, 0, 0,
(), ())
We used to have EMPTY in the 0 cases, but we won't anymore, will we? Why is that
ok?

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:235: IPC_MESSAGE_DECL(msg_class, Sync, Control,
IPC_TUPLE in, IPC_TUPLE out)
should these be IPC_TUPLE(in), IPC_TUPLE(out) or is this doing something else?

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:54: template <uint32_t Id, const char* Name,
typename... Ins>
There's a real lack of comments in here to explain what everything is

[vmpstr, 2015-12-19 00:26:46]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2015-12-19 00:26:47]

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:8: // Synopsis
Does the approach of directly checking if we're on MSVC work here?

ie

#if defined(COMPILER_MSVC)
...
#else
...
#endif

or is there something else going on?

[mdempsky, 2015-12-19 01:45:54]

On 2015/12/19 00:26:47, vmpstr wrote:
> Does the approach of directly checking if we're on MSVC work here?
> 
> ie
> 
> #if defined(COMPILER_MSVC)
> ...
> #else
> ...
> #endif
> 
> or is there something else going on?

No, because it also depends on whether IPC_MESSAGE_EXPORT (which is defined as
CONTENT_EXPORT, GPU_EXPORT, etc depending on messages being defined) is
__declspec(dllexport) or __declspec(dllimport), which in turn depends on whether
CONTENT_IMPLEMENTATION, GPU_IMPLEMENTATION, etc. are defined.

One option would be to change all of the

    #define IPC_MESSAGE_EXPORT FOO_EXPORT

sites to something like

    #define IPC_MESSAGE_EXPORT_PREFIX FOO

and then we could use IPC_MESSAGE_EXPORT_PREFIX##_EXPORT and/or
defined(IPC_MESSAGE_EXPORT_PREFIX##_IMPLEMENTATION) as appropriate.  That
simplify/eliminate the need for export_templates.h.

[mdempsky, 2015-12-19 02:14:49]

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:40: // MSVC emits warning C4003 ("not enough actual
parameters for macro
On 2015/12/19 00:15:05, danakj (behind on reviews) wrote:
> True in MSVC 2015?

Version "19.00.23602.0" at http://webcompiler.cloudapp.net/ (which I think is
2015?) at least still emits C4003.

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:50: // Issue 3.
On 2015/12/19 00:15:05, danakj (behind on reviews) wrote:
> One wonders where Issue 2 is..

Typo.  Issue 2 had to do with chaining variadic macros, but in this latest patch
set I was able to eliminate needing to do that, so I removed the mention of the
issue.  (I thought I renumbered issue 3 too, but apparently not.)

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:61: #define EXPORT_TEMPLATE_DECLARE(export) \
On 2015/12/19 00:15:05, danakj (behind on reviews) wrote:
> I think this needs a bunch more comments.

Will do.  What sort of comments would be most useful?  Example inputs and what
they should expand to?

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:94: TEST_##want, EXPORT_TEMPLATE_STYLE(export, ),
export), \
On 2015/12/19 00:15:05, danakj (behind on reviews) wrote:
> Same even for the tests. This ends up as TEST_DEFAULT, which is passed to the
> INVOKE.. 
> 

FWIW, what happens here is that when the macros work as expected,
EXPORT_TEMPLATE_INVOKE(...) expands to

    static_assert(EXPORT_TEMPLATE_TEST_##want##_##got(,), #export)

since I define EXPORT_TEMPLATE_TEST_DEFAULT_DEFAULT(...) and
EXPORT_TEMPLATE_TEST_MSVC_HACK_MSVC_HACK(...) to true, that makes the
static_assert valid.

When the macros fail to work as intended, then it'll produce some other crazy
bogus line that causes a compiler failure.

(I'll put this into comments.)

https://codereview.chromium.org/1532053002/diff/440001/content/renderer/rende...
File content/renderer/render_thread_impl_browsertest.cc (right):

https://codereview.chromium.org/1532053002/diff/440001/content/renderer/rende...
content/renderer/render_thread_impl_browsertest.cc:30: // TODO(mdempsky): Fix
properly.
On 2015/12/19 00:15:05, danakj (behind on reviews) wrote:
> Can you leave more context, what is properly?

"Properly" here is creating browsertest_messages.h and
browsertest_messages_generator.cc and doing the full #include ritual like in
content/common/content_message_generator.cc.

The problem here is that ipc/ipc_messages_macro.h has already been #include'd by
the FOO_messages.h headers included above, so defining IPC_MESSAGE_IMPL here
wasn't doing anything.  It happened to work only because it's only used for
TestMsg_QuitRunLoop, which is a 0-argument async message, and the macros before
happened to implement enough of those even without IPC_MESSAGE_IMPL for this
file to work.

https://codereview.chromium.org/1532053002/diff/440001/ipc/attachment_broker_...
File ipc/attachment_broker_messages.h (right):

https://codereview.chromium.org/1532053002/diff/440001/ipc/attachment_broker_...
ipc/attachment_broker_messages.h:21: #undef IPC_MESSAGE_EXPORT
On 2015/12/19 00:15:06, danakj (behind on reviews) wrote:
> Why is this moved?

I was trying to fix the Windows component build and thought this was the
problem, though this didn't fix it (at least in isolation).  I'll check if this
can be reverted.

(Context: The IPC_STRUCTS_TRAITS_BEGIN macros define template specializations
using IPC_MESSAGE_EXPORT.  At one point I had Windows component build errors
relating to the messages below (e.g., AttachmentBrokerMsg_DuplicateWinHandle)
referring to PC::internal::HandleAttachmentWin::WireFormat's Traits.  I thought
the problem might have been that the traits specializations and messages were
using different EXPORT settings, so I moved this up so they'd have the same.)

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (left):

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:228: IPC_MESSAGE_DECL(EMPTY, CONTROL, msg_class, 0, 0,
(), ())
On 2015/12/19 00:15:06, danakj (behind on reviews) wrote:
> We used to have EMPTY in the 0 cases, but we won't anymore, will we? Why is
that
> ok?

The EMPTY cases used to be the same as 0-parameter ASYNC cases, except they
optimized away some of the degenerate code.  I didn't see any point in
continuing to special case them when the generic code could handle them the
same.

(I did at an earlier point partially specialize the ASYNC templates so they
would behave the same as EMPTY used to.  We could still do that if we find a
reason to.)

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:235: IPC_MESSAGE_DECL(msg_class, Sync, Control,
IPC_TUPLE in, IPC_TUPLE out)
On 2015/12/19 00:15:06, danakj (behind on reviews) wrote:
> should these be IPC_TUPLE(in), IPC_TUPLE(out) or is this doing something else?

The idea is callers write:

  IPC_SYNC_MESSAGE_CONTROL(MyMsg, (in1, in2, in3), (out1, out2))

which expands to

  IPC_MESSAGE_DECL(msg_class, Sync, Control, IPC_TUPLE (in1, in2, in3),
IPC_TUPLE (in1, in2, in3))

and the caller provided () are used to trigger expansion of the IPC_TUPLE
function-like macro.

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/440001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:54: template <uint32_t Id, const char* Name,
typename... Ins>
On 2015/12/19 00:15:06, danakj (behind on reviews) wrote:
> There's a real lack of comments in here to explain what everything is

I'll add some.  (To be fair, I preserved *both* of the comments from the macros
this code replaces...)

[mdempsky, 2015-12-19 02:17:05]

(I meant to add: I of course plan to add comments to clarify all of the code
you've asked about.  Just trying to give immediate answers in case they prompt
further questions/discussion.)

[mdempsky, 2015-12-19 02:32:42]

On 2015/12/19 01:45:54, mdempsky wrote:
> One option would be to change all of the
> 
>     #define IPC_MESSAGE_EXPORT FOO_EXPORT
> 
> sites to something like
> 
>     #define IPC_MESSAGE_EXPORT_PREFIX FOO
> 
> and then we could use IPC_MESSAGE_EXPORT_PREFIX##_EXPORT and/or
> defined(IPC_MESSAGE_EXPORT_PREFIX##_IMPLEMENTATION) as appropriate.  That
> simplify/eliminate the need for export_templates.h.

Actually, I wouldn't be able to rely on
defined(IPC_MESSAGE_EXPORT_PREFIX##_IMPLEMENTATION), because defined() is only
usable in #if expressions, and it's currently valid (and necessary in some
cases) to define IPC_MESSAGE_EXPORT to multiple different values.

Changing callers from:

    #undef IPC_MESSAGE_EXPORT
    #define IPC_MESSAGE_EXPORT FOO_EXPORT

to

    #undef IPC_MESSAGE_EXPORT
    #define IPC_MESSAGE_EXPORT FOO_EXPORT
    #undef IPC_MESSAGE_IMPLEMENTATION
    #if defined(FOO_IMPLEMENTATION)
    #define IPC_MESSAGE_IMPLEMENTATION FOO_IMPLEMENTATION
    #endif

would work, but it's kind of clumsy.

[dcheng, 2015-12-21 18:32:36]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-12-21 18:32:37]

So I think it's obvious that the current IPC implementation is a crazy macro
soup. A few questions though:

1) What makes the template+macro implementation superior to the macro
implementation?

2) Did you test that the template export magic works in a component + ASAN
build? That's been one of the situations that tripped up exports in the past.

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:83: #define EXPORT_TEMPLATE_STYLE_MATCH__attribute__(...)
DEFAULT
Any name that contains a double-underscore is reserved. The Google style header
guard used to use double-underscores, and we went to some effort to migrate away
from that.

[mdempsky, 2015-12-23 23:17:46]

On 2015/12/21 18:32:37, dcheng wrote:
> 1) What makes the template+macro implementation superior to the macro
> implementation?

I think it's easier to read and make changes to the templated code than to
change the macro code.  E.g., after having extracted the templates, I see a lot
of room for further simplification that wasn't apparent to me before.

> 2) Did you test that the template export magic works in a component + ASAN
> build? That's been one of the situations that tripped up exports in the past.

Only component builds so far.  Are there any component+ASAN trybots?  I can
locally test Linux component+ASON, but not Windows component+ASAN.

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/440001/base/export_template.h...
base/export_template.h:83: #define EXPORT_TEMPLATE_STYLE_MATCH__attribute__(...)
DEFAULT
On 2015/12/21 18:32:37, dcheng wrote:
> Any name that contains a double-underscore is reserved.

Hm, indeed.  (I didn't realize C++ reserved double-underscore anywhere in an
identifier; C only reserves it at the beginning.)

How problematic is that?  I don't think there's a way to implement this header's
API without defining macro names containing double-underscores.  I could change
the EXPORT_TEMPLATE_STYLE_MATCH prefix to be arbitrarily complex (e.g., contain
a random 256-bit base64-encoded value) to minimize collisions, but they'll still
technically be reserved.

(We can do without EXPORT_TEMPLATE_{DECLARE,DEFINE}, but it requires changing
every "#define IPC_MESSAGE_EXPORT FOO_EXPORT" throughout the source tree.  Not
impossible, but I was hoping to keep the user-facing API untouched for now.)

[vmpstr, 2015-12-28 21:19:59]

On 2015/12/19 01:45:54, mdempsky wrote:
> On 2015/12/19 00:26:47, vmpstr wrote:
> > Does the approach of directly checking if we're on MSVC work here?
> > 
> > ie
> > 
> > #if defined(COMPILER_MSVC)
> > ...
> > #else
> > ...
> > #endif
> > 
> > or is there something else going on?
> 
> No, because it also depends on whether IPC_MESSAGE_EXPORT (which is defined as
> CONTENT_EXPORT, GPU_EXPORT, etc depending on messages being defined) is
> __declspec(dllexport) or __declspec(dllimport), which in turn depends on
whether
> CONTENT_IMPLEMENTATION, GPU_IMPLEMENTATION, etc. are defined.

Oh, that makes sense. Thanks.

> 
> One option would be to change all of the
> 
>     #define IPC_MESSAGE_EXPORT FOO_EXPORT
> 
> sites to something like
> 
>     #define IPC_MESSAGE_EXPORT_PREFIX FOO
> 
> and then we could use IPC_MESSAGE_EXPORT_PREFIX##_EXPORT and/or
> defined(IPC_MESSAGE_EXPORT_PREFIX##_IMPLEMENTATION) as appropriate.  That
> simplify/eliminate the need for export_templates.h.

My main concern is that it seems export_templates is as complicated of template
code as you can write :) I doubt there will be many changes there (aside from
removing msvc hacks when appropriate), but anyone reading it is going to have a
fun time.

I think it's fine as it is, but if it can be simpler that would be awesome.

(sorry for the noise :P)

[mdempsky, 2016-01-05 02:37:49]

Reverted some unnecessary changes, added a lot more comments, and renamed
EXPORT_TEMPLATE_STYLE_MATCH__* to
EXPORT_TEMPLATE_STYLE_MATCH_foj3FJo5StF0OvIzl7oMxA__* because of the forbidden
double underscores.

Windows compile trybots still look happy on Windows.  Still haven't tested
component/asan build on Windows, but it seems to work on Linux.  (GN build with
"use_goma=true; is_component_build=true; is_asan=true".)

[mdempsky, 2016-01-07 01:49:27]

PTAL PS30

I went ahead and applied a few of the followup simplifications that I had in
mind.  In particular removing the redundancy from MessageSchema and
SyncMessageSchema, but also simplifying some of the helper templates.  I think
ipc_message_templates.h should be much easier to understand now.

Also, PS30 now shrinks stripped chrome release binaries on Linux by 130kB (from
111.71M to 111.58M), although it increases the unstripped binaries by 500kB
(from 159.26MB to 159.76MB).

[mdempsky, 2016-01-07 06:31:20]

On 2016/01/07 01:49:27, mdempsky wrote:
> Also, PS30 now shrinks stripped chrome release binaries on Linux by 130kB
(from
> 111.71M to 111.58M), although it increases the unstripped binaries by 500kB
> (from 159.26MB to 159.76MB).

As of PS33, the unstripped binary size has now *decreased* by 80kB versus
baseline (from 159.26MB to 159.18MB).  The stripped binary size is unchanged
from PS30.

[mdempsky, 2016-01-19 19:10:24]

Ping.

[mdempsky, 2016-02-04 22:40:59]

Ping.

[vmpstr, 2016-02-04 23:43:27]

This looks good to me (modulo comments). However, because of the density of the
code, I think someone else should take a look as well.

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:40: // MSVC emits warning C4003 ("not enough actual
parameters for macro
Is this still the case with MSVC2015?

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:56: // The correct replacement for "F(j())" is "ok", but
MSVC replaces it
Is this still the case with MSVC2015?

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:113: // conflict with implementations, we include
"foj3FJo5StF0OvIzl7oMxA" (a random
I think this is probably overkill, but sure why not :)

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:147: #define EXPORT_TEMPLATE_TEST(want, export)          
                      \
maybe #undef these after the tests?

https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...
components/nacl/browser/nacl_file_host.cc:47: const uint64_t& file_token_lo,
Why this change?

https://codereview.chromium.org/1532053002/diff/660001/content/renderer/input...
File content/renderer/input/input_event_filter.cc (right):

https://codereview.chromium.org/1532053002/diff/660001/content/renderer/input...
content/renderer/input/input_event_filter.cc:29: #define IPC_MESSAGE_DECL(name,
...) \
Where is this used? I tried to see if you updated the callsites to use the first
parameter as name instead of third, but I can't really find where this is used
:)

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:70: // TODO(mdempsky): Reevaluate once MSVC 2015.
I'm not really up to date, but is MSVC2015 here? Or very close to here?

Also,
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

has default argument for a template member (ctor) of a templated class, and it
seems to work on MSVC. Does the ctor behave differently from regular functions?

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:74: template <bool X = (x), typename
std::enable_if<X, bool>::type = false>
Is the extra bool X = (x) just convenience? Or does something not work if you
just pass x to enable_if?

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:91: // TODO(mdempsky): Remove.
Can you elaborate on this todo?

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:95: MessageT(const Ins&... ins) :
MessageT(Routing{MSG_ROUTING_CONTROL}, ins...) {
Is this syntax allowed? I keep being confused between initializer_lists and
uniform initialization syntax and all that...

Routing{MSG_ROUTING_CONTROL} is what I'm referring to.

[mdempsky, 2016-02-05 00:32:32]

vmpstr: Thanks!

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:40: // MSVC emits warning C4003 ("not enough actual
parameters for macro
On 2016/02/04 23:43:26, vmpstr wrote:
> Is this still the case with MSVC2015?

Yes, at least tested using "Compiler version: 19.00.23720.0 (x86). Last updated:
Jan 20, 2016" at http://webcompiler.cloudapp.net/ (which I believe is MSVC2015).

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:56: // The correct replacement for "F(j())" is "ok", but
MSVC replaces it
On 2016/02/04 23:43:26, vmpstr wrote:
> Is this still the case with MSVC2015?

Yes.  If I test this file with webcompiler but change EXPORT_TEMPLATE_STYLE_2 to
not use EXPORT_TEMPLATE_STYLE_3, then it fails to compile.

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:113: // conflict with implementations, we include
"foj3FJo5StF0OvIzl7oMxA" (a random
On 2016/02/04 23:43:26, vmpstr wrote:
> I think this is probably overkill, but sure why not :)

Acknowledged.

https://codereview.chromium.org/1532053002/diff/660001/base/export_template.h...
base/export_template.h:147: #define EXPORT_TEMPLATE_TEST(want, export)          
                      \
On 2016/02/04 23:43:26, vmpstr wrote:
> maybe #undef these after the tests?

Done.

https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...
components/nacl/browser/nacl_file_host.cc:47: const uint64_t& file_token_lo,
On 2016/02/04 23:43:26, vmpstr wrote:
> Why this change?

Previously MySyncMessage::WriteReplyParams was a templated member function that
took any parameter types, but then always converted them to "const T&".  In this
CL I changed it to simply take "const T&" arguments directly.

Most WriteReplyParams users simply called it directly and were unaffected, but
this file use a function-pointer to indirectly call one of a couple different
WriteReplyParams functions.  Because it's no longer templated though, the
static_cast<WriteFileInfoReply> can't force a particular template instantiation
anymore.  Hence changing this type.

https://codereview.chromium.org/1532053002/diff/660001/content/renderer/input...
File content/renderer/input/input_event_filter.cc (right):

https://codereview.chromium.org/1532053002/diff/660001/content/renderer/input...
content/renderer/input/input_event_filter.cc:29: #define IPC_MESSAGE_DECL(name,
...) \
On 2016/02/04 23:43:27, vmpstr wrote:
> Where is this used? I tried to see if you updated the callsites to use the
first
> parameter as name instead of third, but I can't really find where this is used
> :)

It's used indirectly via the #include "content/common/input_messages.h" below. 
That file declares a bunch of messages, which will then be instantiated using
this IPC_MESSAGE_DECL macro definition instead, which is how they implement
GetInputMessageTypeName's switch statement.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:70: // TODO(mdempsky): Reevaluate once MSVC 2015.
On 2016/02/04 23:43:27, vmpstr wrote:
> I'm not really up to date, but is MSVC2015 here? Or very close to here?

No idea.

> Also,
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> 
> has default argument for a template member (ctor) of a templated class, and it
> seems to work on MSVC. Does the ctor behave differently from regular
functions?

I don't recall anything special about constructors vs other member functions. 
If someone can easily confirm that it works in MSVC 2015 now, I'll change this
CL.  Otherwise, I'd rather try playing with that in a separate followup CL.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:74: template <bool X = (x), typename
std::enable_if<X, bool>::type = false>
On 2016/02/04 23:43:27, vmpstr wrote:
> Is the extra bool X = (x) just convenience? Or does something not work if you
> just pass x to enable_if?

I'm pretty sure it's necessary.  I think it's because if you just did

    template <typename std::enable_if<(x), bool>::type = false>

then the std::enable_if<(x), bool>::type type will be expanded when the class
template is instantiated.  But since (x) is false, it won't evaluate to a valid
type, and the entire class will be ill-formed.

By using

    template <typename bool X = (x), ...>

the value of X is potentially user-controlled, so the compiler defers
determining std::enable_if<X, bool>::type until the function is actually used,
which allows SFINAE to actually work.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:91: // TODO(mdempsky): Remove.
On 2016/02/04 23:43:27, vmpstr wrote:
> Can you elaborate on this todo?

Done.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:95: MessageT(const Ins&... ins) :
MessageT(Routing{MSG_ROUTING_CONTROL}, ins...) {
On 2016/02/04 23:43:27, vmpstr wrote:
> Is this syntax allowed? I keep being confused between initializer_lists and
> uniform initialization syntax and all that...
> 
> Routing{MSG_ROUTING_CONTROL} is what I'm referring to. 

Blah, looks like still no.  Done.

[danakj, 2016-02-05 00:41:04]

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:70: // TODO(mdempsky): Reevaluate once MSVC 2015.
On 2016/02/05 00:32:32, mdempsky wrote:
> On 2016/02/04 23:43:27, vmpstr wrote:
> > I'm not really up to date, but is MSVC2015 here? Or very close to here?
> 
> No idea.
> 
> > Also,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> > 
> > has default argument for a template member (ctor) of a templated class, and
it
> > seems to work on MSVC. Does the ctor behave differently from regular
> functions?
> 
> I don't recall anything special about constructors vs other member functions. 
> If someone can easily confirm that it works in MSVC 2015 now, I'll change this
> CL.  Otherwise, I'd rather try playing with that in a separate followup CL.

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/6Wd16Iqhy9Y

[vmpstr, 2016-02-05 01:24:31]

This lgtm. It would be nice if someone else reviewed as well though.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:70: // TODO(mdempsky): Reevaluate once MSVC 2015.
On 2016/02/05 00:32:32, mdempsky wrote:
> On 2016/02/04 23:43:27, vmpstr wrote:
> > I'm not really up to date, but is MSVC2015 here? Or very close to here?
> 
> No idea.
> 
> > Also,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
> > 
> > has default argument for a template member (ctor) of a templated class, and
it
> > seems to work on MSVC. Does the ctor behave differently from regular
> functions?
> 
> I don't recall anything special about constructors vs other member functions. 
> If someone can easily confirm that it works in MSVC 2015 now, I'll change this
> CL.  Otherwise, I'd rather try playing with that in a separate followup CL.

My point was simply that this seems to already work in MSVC (since scoped_ptr is
compiled by whatever the current compiler is). But maybe there's something else
that's subtle going on.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:74: template <bool X = (x), typename
std::enable_if<X, bool>::type = false>
On 2016/02/05 00:32:32, mdempsky wrote:
> On 2016/02/04 23:43:27, vmpstr wrote:
> > Is the extra bool X = (x) just convenience? Or does something not work if
you
> > just pass x to enable_if?
> 
> I'm pretty sure it's necessary.  I think it's because if you just did
> 
>     template <typename std::enable_if<(x), bool>::type = false>
> 
> then the std::enable_if<(x), bool>::type type will be expanded when the class
> template is instantiated.  But since (x) is false, it won't evaluate to a
valid
> type, and the entire class will be ill-formed.
> 
> By using
> 
>     template <typename bool X = (x), ...>
> 
> the value of X is potentially user-controlled, so the compiler defers
> determining std::enable_if<X, bool>::type until the function is actually used,
> which allows SFINAE to actually work.

I guess my initial concern was that if one explicitly specifies "true" for the
first parameter, then it enables either one of the ctors. However, TIL that it's
actually impossible to explicitly specify ctor template parameters, so this
looks fine to me :)

Although I would've hoped that

template <typename std::enable_if<Meta::kKind == MessageKind::CONTROL,
bool>::type = false>

would also work (where that equality is what "x" should be)

https://codereview.chromium.org/1532053002/diff/680001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/680001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:61: Routing(int32_t id) : id(id){};
nit: semicolon not required.

[mdempsky, 2016-02-05 02:06:11]

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:70: // TODO(mdempsky): Reevaluate once MSVC 2015.
On 2016/02/05 01:24:31, vmpstr wrote:
> My point was simply that this seems to already work in MSVC (since scoped_ptr
is
> compiled by whatever the current compiler is). But maybe there's something
else
> that's subtle going on.

Understood.  I'm going to lean towards something more subtle is going on,
because those lines from scoped_ptr.h are from Nov 2015, and I know this was
failing on the Windows trybots more recently than that.

https://codereview.chromium.org/1532053002/diff/660001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:74: template <bool X = (x), typename
std::enable_if<X, bool>::type = false>
On 2016/02/05 01:24:31, vmpstr wrote:
> Although I would've hoped that
> 
> template <typename std::enable_if<Meta::kKind == MessageKind::CONTROL,
> bool>::type = false>
> 
> would also work (where that equality is what "x" should be)

I did too, but alas, that's not what the C++ committee decided. :(

https://codereview.chromium.org/1532053002/diff/680001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/680001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:61: Routing(int32_t id) : id(id){};
On 2016/02/05 01:24:31, vmpstr wrote:
> nit: semicolon not required.

Done.  (That also seems to have explained clang-format omitting the space before
the braces.)

[mdempsky, 2016-02-05 18:42:23]

mdempsky@chromium.org changed reviewers:
+ tsepez@chromium.org

[mdempsky, 2016-02-05 18:42:24]

+tsepez for ipc/OWNERS

[Tom Sepez, 2016-02-05 19:15:19]

tsepez@chromium.org changed reviewers:
+ jam@chromium.org

[Tom Sepez, 2016-02-05 19:16:20]

On 2016/02/05 18:42:24, mdempsky wrote:
> +tsepez for ipc/OWNERS

I like it, but I'm still trying to wrap my head around the details.
+jam@ as he often has an opinion on these kinds of things.

[Tom Sepez, 2016-02-05 19:23:08]

Next question:  Can we split off the template_export.h stuff into its own CL? 
There's got to be some other stuff that would want to use this, and it would be
a shame to lose it if the IPC portion needed to be reverted.

https://codereview.chromium.org/1532053002/diff/720001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/720001/base/export_template.h...
base/export_template.h:15: // instantiation definition instead.
nit: definitions

[mdempsky, 2016-02-05 19:36:21]

On 2016/02/05 19:23:08, Tom Sepez wrote:
> Next question:  Can we split off the template_export.h stuff into its own CL?

Yeah, I can do that.

[mdempsky, 2016-02-05 19:38:39]

https://codereview.chromium.org/1532053002/diff/720001/base/export_template.h
File base/export_template.h (right):

https://codereview.chromium.org/1532053002/diff/720001/base/export_template.h...
base/export_template.h:15: // instantiation definition instead.
On 2016/02/05 19:23:08, Tom Sepez wrote:
> nit: definitions

Done.

[Tom Sepez, 2016-02-05 19:44:24]

https://codereview.chromium.org/1532053002/diff/720001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/720001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:445: #define IPC_SYNC_MESSAGE_CONTROL0_0
IPC_SYNC_MESSAGE_CONTROL0_x
What does the compiler error message look like when I mess up a declaration by
providing too many or two few inputs vs. what I get today? Folks do that all the
time, and hopefully identifying the cause nowdays isn't too cryptic.

we're going to be tolerant of extra/fewer outputs vs the macro, but that's not
necessarily a bad thing once we rename stuff.

[mdempsky, 2016-02-05 19:49:54]

https://codereview.chromium.org/1532053002/diff/720001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/720001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:445: #define IPC_SYNC_MESSAGE_CONTROL0_0
IPC_SYNC_MESSAGE_CONTROL0_x
On 2016/02/05 19:44:24, Tom Sepez wrote:
> What does the compiler error message look like when I mess up a declaration by
> providing too many or two few inputs vs. what I get today?

No error.  If you do

  IPC_SYNC_MESSAGE_CONTROL1_2(a, b, c, d)

it will be silently treated as

  IPC_SYNC_MESSAGE_CONTROL((a), (b, c, d))

If you think that's undesirable, I can change the legacy macro definitions to
something like:

  #define IPC_SYNC_MESSAGE_CONTROL1_2(a, b, c) IPC_MESSAGE_CONTROL((a), (b, c))

to ensure they don't get misused before I'm able to update all the messages.

[Tom Sepez, 2016-02-05 19:58:49]

https://codereview.chromium.org/1532053002/diff/740001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/740001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:258: using msg_name = IPC::MessageT<msg_name##_Meta>;  
               \
I would have guessed that with this, we get duplicate conflicting using
statements at top level. What am I missing?

[mdempsky, 2016-02-05 20:03:39]

https://codereview.chromium.org/1532053002/diff/720001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/720001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:445: #define IPC_SYNC_MESSAGE_CONTROL0_0
IPC_SYNC_MESSAGE_CONTROL0_x
(Done in PS39, BTW.)

https://codereview.chromium.org/1532053002/diff/740001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/740001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:258: using msg_name = IPC::MessageT<msg_name##_Meta>;  
               \
On 2016/02/05 19:58:49, Tom Sepez wrote:
> I would have guessed that with this, we get duplicate conflicting using
> statements at top level. What am I missing?

IPC_MESSAGE_DECL previously defined msg_name as a struct type, so invoking it
multiple times for the same msg_name identifier would already have resulted in
conflicting definition errors.

[Tom Sepez, 2016-02-05 20:08:38]

https://codereview.chromium.org/1532053002/diff/740001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/740001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:258: using msg_name = IPC::MessageT<msg_name##_Meta>;  
               \
On 2016/02/05 20:03:39, mdempsky wrote:
> On 2016/02/05 19:58:49, Tom Sepez wrote:
> > I would have guessed that with this, we get duplicate conflicting using
> > statements at top level. What am I missing?
> 
> IPC_MESSAGE_DECL previously defined msg_name as a struct type, so invoking it
> multiple times for the same msg_name identifier would already have resulted in
> conflicting definition errors.

Obviously. I didn't notice it was a macro parameter somehow. Sorry.

[Tom Sepez, 2016-02-05 20:22:32]

Its elegant. What does it do to the binary size?  LGTM otherwise.

[mdempsky, 2016-02-05 20:25:58]

On 2016/02/05 20:22:32, Tom Sepez wrote:
> Its elegant. What does it do to the binary size?  LGTM otherwise.

At least as of PS33, it marginally shrunk both stripped and unstripped binary
sizes on Linux.  I don't expect any of the tweaks since then should have
affected things.

    On 2016/01/07 01:49:27, mdempsky wrote:
    > Also, PS30 now shrinks stripped chrome release binaries on Linux by 130kB
(from
    > 111.71M to 111.58M), although it increases the unstripped binaries by
500kB
    > (from 159.26MB to 159.76MB).

    As of PS33, the unstripped binary size has now *decreased* by 80kB versus
    baseline (from 159.26MB to 159.18MB).  The stripped binary size is unchanged
    from PS30.

[Tom Sepez, 2016-02-05 20:26:52]

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:403: // TODO(mdempsky): Replace uses with generic
names.
note: Now there's no reason to limit the size to 5 -- bumping up against this
limit is really annoying to message writers. We can grow this without any code
changes?

[mdempsky, 2016-02-05 20:31:59]

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:403: // TODO(mdempsky): Replace uses with generic
names.
On 2016/02/05 20:26:52, Tom Sepez wrote:
> note: Now there's no reason to limit the size to 5 -- bumping up against this
> limit is really annoying to message writers. We can grow this without any code
> changes?

Correct.  There are no limits on the number of parameters that can be used with
the new IPC_(SYNC_)MESSAGE_(CONTROL|ROUTED) macros.

[mdempsky, 2016-02-05 20:34:10]

danakj/dcheng: Interested whether you have any outstanding concerns.  If not,
I'll split out export_template.h into a separate CL per tsepez's feedback.

[dcheng, 2016-02-05 21:31:41]

My other main question is why does export_template.h need to be in //base?

https://codereview.chromium.org/1532053002/diff/760001/components/nacl/browse...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/1532053002/diff/760001/components/nacl/browse...
components/nacl/browser/nacl_file_host.cc:47: const uint64_t& file_token_lo,
Why change these to const ref?

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:37: // TODO(mdempsky): Apply UnwrapTraits like
base::DispatchToMethod?
Do we need that here though? I don't think we'd need to forward WeakPtrs or any
sort of fun stuff like that.

[mdempsky, 2016-02-05 22:36:06]

On 2016/02/05 21:31:41, dcheng wrote:
> My other main question is why does export_template.h need to be in //base?

It doesn't have to be.  It just seemed unspecific to IPC; useful in some other
(niche) cases; and //base is where most other compiler-specific abstractions end
up.

https://codereview.chromium.org/1532053002/diff/760001/components/nacl/browse...
File components/nacl/browser/nacl_file_host.cc (right):

https://codereview.chromium.org/1532053002/diff/760001/components/nacl/browse...
components/nacl/browser/nacl_file_host.cc:47: const uint64_t& file_token_lo,
On 2016/02/05 21:31:41, dcheng wrote:
> Why change these to const ref?

See
https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1532053002/diff/760001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:37: // TODO(mdempsky): Apply UnwrapTraits like
base::DispatchToMethod?
On 2016/02/05 21:31:41, dcheng wrote:
> Do we need that here though? I don't think we'd need to forward WeakPtrs or
any
> sort of fun stuff like that.

I don't know, hence the question mark. :)

IPC is essentially the only user of base::DispatchToMethod, which does apply
UnwrapTraits, so it just seemed odd/inconsistent to me that the only time we
skip applying UnwrapTraits is when dispatching a message to a method that takes
an extra WITH_PARAM parameter.

[jam, 2016-02-05 22:43:50]

On 2016/02/05 19:16:20, Tom Sepez wrote:
> On 2016/02/05 18:42:24, mdempsky wrote:
> > +tsepez for ipc/OWNERS
> 
> I like it, but I'm still trying to wrap my head around the details.
> +jam@ as he often has an opinion on these kinds of things.

I have no strong feelings (i.e. I defer to you)

My only comment is that we shouldn't invest too much time in this code given the
push to move to mojo ipc..

[mdempsky, 2016-02-08 21:49:04]

mdempsky@chromium.org changed reviewers:
+ thestig@chromium.org

[mdempsky, 2016-02-08 21:49:05]

+thestig for base/OWNERS

thestig: It seems the only remaining point of contention for this CL is where
export_template.h should go.  My inclination was to put it in //base because
then it can be reused for things like
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

But otherwise I'm fine with moving it into //ipc.

[Lei Zhang, 2016-02-09 01:57:46]

On 2016/02/08 21:49:05, mdempsky wrote:
> +thestig for base/OWNERS
> 
> thestig: It seems the only remaining point of contention for this CL is where
> export_template.h should go.  My inclination was to put it in //base because
> then it can be reused for things like
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> 
> But otherwise I'm fine with moving it into //ipc.

How about we stick this in ipc/ for now and discuss the merits of moving it to
base/ in a separate CL?

[mdempsky, 2016-02-09 02:06:53]

On 2016/02/09 01:57:46, Lei Zhang wrote:
> How about we stick this in ipc/ for now and discuss the merits of moving it to
> base/ in a separate CL?

Done.

[mdempsky, 2016-02-09 03:03:23]

Description was changed from

==========
use variadic macros/templates in IPC message implementation
==========

to

==========
use variadic macros/templates in IPC message implementation

TBR=jam@chromium.org, mseaborn@chromium.org
==========

[mdempsky, 2016-02-09 03:05:51]

mdempsky@chromium.org changed reviewers:
+ mseaborn@chromium.org

[mdempsky, 2016-02-09 03:05:53]

TBR:

mseaborn for components/nacl (see
https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...
for rationale on WriteFileInfoReply change)

jam for content/renderer

[mdempsky, 2016-02-09 03:06:04]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2016-02-09 03:06:05]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1532053002/#ps800001
(title: "move for real")

[commit-bot: I haz the power, 2016-02-09 03:08:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532053002/800001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532053002/800001

[commit-bot: I haz the power, 2016-02-09 05:41:55]

Description was changed from

==========
use variadic macros/templates in IPC message implementation

TBR=jam@chromium.org, mseaborn@chromium.org
==========

to

==========
use variadic macros/templates in IPC message implementation

TBR=jam@chromium.org, mseaborn@chromium.org
==========

[commit-bot: I haz the power, 2016-02-09 05:41:56]

Committed patchset #41 (id:800001)

[commit-bot: I haz the power, 2016-02-09 05:42:52]

Description was changed from

==========
use variadic macros/templates in IPC message implementation

TBR=jam@chromium.org, mseaborn@chromium.org
==========

to

==========
use variadic macros/templates in IPC message implementation

TBR=jam@chromium.org, mseaborn@chromium.org

Committed: https://crrev.com/8a5190449d48e06efa581390426dfa3bb6750f4c
Cr-Commit-Position: refs/heads/master@{#374316}
==========

[commit-bot: I haz the power, 2016-02-09 05:42:53]

Patchset 41 (id:??) landed as
https://crrev.com/8a5190449d48e06efa581390426dfa3bb6750f4c
Cr-Commit-Position: refs/heads/master@{#374316}

[Mark Seaborn, 2016-02-09 18:16:56]

On 2016/02/09 03:05:53, mdempsky wrote:
> TBR:
> 
> mseaborn for components/nacl (see
>
https://codereview.chromium.org/1532053002/diff/660001/components/nacl/browse...
> for rationale on WriteFileInfoReply change)

LGTM for components/nacl/.