Make the implementation .cc files go away, instead have the authors give us a...

Make the implementation .cc files go away, instead have the authors give us a header only.


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

[darin (slow to review), 2011-02-01 22:20:45]

http://codereview.chromium.org/6410007/diff/6003/chrome/common/indexed_db_mes...
File chrome/common/indexed_db_messages.h (right):

http://codereview.chromium.org/6410007/diff/6003/chrome/common/indexed_db_mes...
chrome/common/indexed_db_messages.h:53: #define IPC_STRUCT_NAME
IndexedDBHostMsg_IndexOpenCursor_Params
it might be nice to use indentation here to improve readability:

IPC_STRUCT_BEGIN(IndexedDBHostMsg_IndexOpenCursor_Params)
  IPC_STRUCT_MEMBER(type, name, flag)
  ...
IPC_STRUCT_END()

bonus points if you can eliminate the IPC_STRUCT_NAME define in favor of passing
the name as a parameter to IPC_STRUCT_BEGIN as i have shown above.  (i haven't
studied your code to see if that is possible.)

[Tom Sepez, 2011-02-02 20:47:05]

Latest patchset implements these ideas.
- Name of struct passed as begin parameter
- No distinction between first/next, via
    -- construction in body, not initializer list
    -- use of && 1 at end of read methods
    -- run-time check to insert commas in log methods
- INIT syntax still present.  
- Second include guard in xxx_messages.h to make it less brittle for ordinary
callers who happen to re-include it.

[Tom Sepez, 2011-02-02 21:54:31]

Note the need to apply the same tricks to the _param_traits.cc/h files.
When the struct/class is externally defined, we'd need to list the members as
before, but not have the code generate the struct declaration, or the ctor/dtor.
 

As in the present, we have the same issue of keeping the message code up to date
when the external deflation changes under such a scheme.

[jam, 2011-02-02 22:00:47]

this looks very good.  if we don't need the INIT stuff, can we take it out?

also, it's a little unfortunate that we have to list all the files in one
master file.  it's easy to miss when copying files.  i guess now this will
always result in linker errors, although it might not be so obvious to
someone why they're happening.

On Wed, Feb 2, 2011 at 1:54 PM, <tsepez@chromium.org> wrote:

> Note the need to apply the same tricks to the _param_traits.cc/h files.
> When the struct/class is externally defined, we'd need to list the members
> as
> before, but not have the code generate the struct declaration, or the
> ctor/dtor.
>
>
> As in the present, we have the same issue of keeping the message code up to
> date
> when the external deflation changes under such a scheme.


I don't understand this?


>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-02 22:11:36]

INIT controls whether we get a
  name = type(0);

shoved into the constructor body.  I'm not sure why some of these structs feel
the need to do these kinds of initializatons.  What happens if we take them out?

As to the second part, there still is a _param_traits.cc file for indexed_db . 
I'd like to make this go away.  If you tell us about the external types using
(similar) macros, we should be able to generate the traits (but not the struct
declarations or ctor/dtors).

[darin (slow to review), 2011-02-02 22:14:50]

On Wed, Feb 2, 2011 at 2:11 PM, <tsepez@chromium.org> wrote:

> INIT controls whether we get a
>  name = type(0);
>
> shoved into the constructor body.  I'm not sure why some of these structs
> feel
> the need to do these kinds of initializatons.  What happens if we take them
> out?
>

It has proven valuable to have these Param structs have a default
constructor that initializes members to reasonable default values.

You can achieve a similar nulling out by using the default constructor in an
initializer list:

Foo::Foo()
  : member1_(),
    member2_() {
}

Maybe this is useful?

-Darin



>
> As to the second part, there still is a _param_traits.cc file for
> indexed_db .
> I'd like to make this go away.  If you tell us about the external types
> using
> (similar) macros, we should be able to generate the traits (but not the
> struct
> declarations or ctor/dtors).
>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-02 22:28:41]

Ah, the problem with the initializer list is its syntax, and how it doesn't
tolerate a trailing comma.  We don't want to distinguish in macros between the
first member and subsequent member, but if we define the member macro to expand
to member1_(), then the last expansion results in membern_(), {} which doesn't
compile.

Something you could think to shove inbetweem that last comma and the {}?

[jam, 2011-02-02 22:47:45]

how about

Foo::Foo() {
 member1_ = type1();
 member2_ = type2();
}

On Wed, Feb 2, 2011 at 2:28 PM, <tsepez@chromium.org> wrote:

> Ah, the problem with the initializer list is its syntax, and how it doesn't
> tolerate a trailing comma.  We don't want to distinguish in macros between
> the
> first member and subsequent member, but if we define the member macro to
> expand
> to member1_(), then the last expansion results in membern_(), {} which
> doesn't
> compile.
>
> Something you could think to shove inbetweem that last comma and the {}?
>
>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-02 23:06:47]

Yes, that kind of constructor ought to do it. Now to make it actually work.

[Tom Sepez, 2011-02-02 23:29:23]

Could one of you enlighten me about what this might mean:

http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/11298/step...

[jam, 2011-02-02 23:31:32]

There are automated dependency checks to enforce the layering between the
different modules.  i.e. code in src/ipc doesn't include code from
src/chrome, which is what's tripping here.

On Wed, Feb 2, 2011 at 3:29 PM, <tsepez@chromium.org> wrote:

> Could one of you enlighten me about what this might mean:
>
>
>
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/11298/step...
>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-02 23:48:40]

So the question becomes, what's the right place for these generator files? 
They're going to be pulling in headers from various places, but the logic
regarding the expansion is part of IPC .

[jam, 2011-02-03 00:32:09]

On Wed, Feb 2, 2011 at 3:48 PM, <tsepez@chromium.org> wrote:

> So the question becomes, what's the right place for these generator files?
> They're going to be pulling in headers from various places, but the logic
> regarding the expansion is part of IPC .


ipc is a separate module, so it can't include code from chrome (i.e. there
are ipc_tests which are a separate target).  can this be done in a different
way so that there's one file in src\chrome that lists the headers?

>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-03 01:22:38]

Sure, but it gets a little more complicated.  Defines can't define other
defines, and defines can't force other includes, so you wind up with a situation
where to keep the IPC parts in /ipc and the common parts in /common, you end up
alternating includes. 

See the common_message_generator.cc file.  This should never need to be touched
as new messages are added, or as the IPC implementation changes under the
covers.

Adding a new message file involves adding its header to the list in
common_message_tree.h, which now lives in common and replaces the
ipc_message_tree.h.

[darin (slow to review), 2011-02-03 05:41:26]

You could make all param structs inherit from a dummy struct, and then make
the last line in the initializer list just mention that dummy struct.

struct NoParams {};

struct FooBar_Params : NoParams {
  FooBar_Params() :
    member1_(),
    member2_(),
    member3_(),
    NoParams() {}
};

This seems like it should work better as constructing a type and then using
the assignment operator can result in a copy in the generated code, though
perhaps the compiler could optimize it away in most cases.

-Darin

On Wed, Feb 2, 2011 at 2:28 PM, <tsepez@chromium.org> wrote:

> Ah, the problem with the initializer list is its syntax, and how it doesn't
> tolerate a trailing comma.  We don't want to distinguish in macros between
> the
> first member and subsequent member, but if we define the member macro to
> expand
> to member1_(), then the last expansion results in membern_(), {} which
> doesn't
> compile.
>
> Something you could think to shove inbetweem that last comma and the {}?
>
>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-03 17:25:46]

Clever.  I'll try it.

[Tom Sepez, 2011-02-03 17:57:20]

Actually, the NoParams() initializer has to come first; no matter, every
subsequent macro generates a leading comma.

Also toying with the idea of putting in an "args" parameter to the
IPC_STRUCT_MEMBER macro, to deal with cases where someone may have hand-written
a constructor along the lines of 
   member1_(17);

can take advantage of parenthesis balancing prior to macro argument binding to
permit multiple params in the inializer, e.g. options for declaring this are:

IPC_STRUCT_MEMBER(sometype, member1_, ())
IPC_STRUCT_MEMBER(sometype, member1_, (17))
IPC_STRUCT_MEMBER(sometype, member1_, ("foo", "bar", 32, "baz"))

Too complicated?  Thoughts?  Thanks heaps.

[darin (slow to review), 2011-02-03 18:29:56]

On Thu, Feb 3, 2011 at 9:57 AM, <tsepez@chromium.org> wrote:

> Actually, the NoParams() initializer has to come first; no matter, every
> subsequent macro generates a leading comma.
>

Oh, yeah... makes sense!


>
> Also toying with the idea of putting in an "args" parameter to the
> IPC_STRUCT_MEMBER macro, to deal with cases where someone may have
> hand-written
> a constructor along the lines of
>   member1_(17);
>
> can take advantage of parenthesis balancing prior to macro argument binding
> to
> permit multiple params in the inializer, e.g. options for declaring this
> are:
>
> IPC_STRUCT_MEMBER(sometype, member1_, ())
> IPC_STRUCT_MEMBER(sometype, member1_, (17))
> IPC_STRUCT_MEMBER(sometype, member1_, ("foo", "bar", 32, "baz"))
>
> Too complicated?  Thoughts?  Thanks heaps.


This seems fine to me.  Another option is to have IPC_STRUCT_MEMBER*N*, but
that is probably less fun.

-Darin



>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-03 18:35:08]

> Another option is to have IPC_STRUCT_MEMBER*N*, but
> that is probably less fun.
 
I really dislike _N macros; would someday like to change to nested macros along
the lines of IPC_STRUCT_ to make them go away ...

[darin (slow to review), 2011-02-03 18:56:03]

One final option is to have just two macros, one that supports an
initializer and one that does not.  That could be useful if the vast
majority of members do not require an explicit initializer.

  ...
  IPC_STRUCT_MEMBER(Foo, foo)
  IPC_STRUCT_MEMBER_INIT(Bar, bar, (1, 2))
  ...

-Darin


On Thu, Feb 3, 2011 at 10:35 AM, <tsepez@chromium.org> wrote:

> Another option is to have IPC_STRUCT_MEMBER*N*, but
>>
>> that is probably less fun.
>>
>
> I really dislike _N macros; would someday like to change to nested macros
> along
> the lines of IPC_STRUCT_ to make them go away ...
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-03 19:11:49]

>   IPC_STRUCT_MEMBER(Foo, foo)
>   IPC_STRUCT_MEMBER_INIT(Bar, bar, (1, 2))
>   ...


Makes sense.  Esp. since the first one is permanently defined in terms of the
second one and won't complicate the amount of things that need to be redefined
to generate code.

#define IPC_STRUCT_MEMBER(type, name) IPC_STRUCT_MEMBER_INIT(type, name, ())

[jam, 2011-02-03 19:15:30]

btw, i really wonder why we need the init case.  if someone is sending a
struct across IPC, they really should be filling in each member.

On Thu, Feb 3, 2011 at 11:11 AM, <tsepez@chromium.org> wrote:

>
>    IPC_STRUCT_MEMBER(Foo, foo)
>>   IPC_STRUCT_MEMBER_INIT(Bar, bar, (1, 2))
>>   ...
>>
>
>
> Makes sense.  Esp. since the first one is permanently defined in terms of
> the
> second one and won't complicate the amount of things that need to be
> redefined
> to generate code.
>
> #define IPC_STRUCT_MEMBER(type, name) IPC_STRUCT_MEMBER_INIT(type, name,
> ())
>
>
>
> http://codereview.chromium.org/6410007/
>

[darin (slow to review), 2011-02-03 19:46:03]

Agreed, but we've had bugs in the past where people sent uninitialized
memory.  I think it's important that we have a decent default constructor,
so then at least the failure mode of a sender not setting all fields will be
more consistent.

-Darin



On Thu, Feb 3, 2011 at 11:15 AM, John Abd-El-Malek <jam@chromium.org> wrote:

> btw, i really wonder why we need the init case.  if someone is sending a
> struct across IPC, they really should be filling in each member.
>
>
> On Thu, Feb 3, 2011 at 11:11 AM, <tsepez@chromium.org> wrote:
>
>>
>>    IPC_STRUCT_MEMBER(Foo, foo)
>>>   IPC_STRUCT_MEMBER_INIT(Bar, bar, (1, 2))
>>>   ...
>>>
>>
>>
>> Makes sense.  Esp. since the first one is permanently defined in terms of
>> the
>> second one and won't complicate the amount of things that need to be
>> redefined
>> to generate code.
>>
>> #define IPC_STRUCT_MEMBER(type, name) IPC_STRUCT_MEMBER_INIT(type, name,
>> ())
>>
>>
>>
>> http://codereview.chromium.org/6410007/
>>
>
>

[Tom Sepez, 2011-02-03 21:17:06]

Counter-example: use of -1 in the following:

PluginMsg_Init_Params::PluginMsg_Init_Params()
    : containing_window(0),
      load_manually(false),
      host_render_view_routing_id(-1) {
}

[Tom Sepez, 2011-02-04 19:54:29]

Spent some time converting a good hunk of chrome/common over to this to ensure
that we're on the right track.  So far, so good, but there's some repeated code
to deal with enums.  So it is time to attack the pesky enums.  

One change that came about as a result of the inability to forward declare an
enum (as you might a struct or class), is that the _END() macro for enums
neeeded the enum name.  To keep things consistent, Ive changed the struct macros
to do the same.  Not a big deal, since I'd be tempted to comment them anyways
along the lines of

IPC_STRUCT_BEGIN(mystruct)
...
IPC_STRUCT_END() // mystruct

so going to

IPC_STRUCT_END(mystruct)

feels ok to me.

[jam, 2011-02-04 22:02:30]

I've looked through all the messages in indexed_db_messages.h as an example.
 All the members that were previously initialized in the struct constructor,
and are now moved to the _INIT macro, are filled in the caller section.  I
find that this macro makes things unnecessarily complicated, and will lead
to redundant declaration of initial values.  So I'm still in favor of just
setting every member using () in constructor's body, for consistency, but
not having these INIT macros, as otherwise people will feel the need to use
them.

On Thu, Feb 3, 2011 at 1:17 PM, <tsepez@chromium.org> wrote:

> Counter-example: use of -1 in the following:
>
> PluginMsg_Init_Params::PluginMsg_Init_Params()
>    : containing_window(0),
>      load_manually(false),
>      host_render_view_routing_id(-1) {
>
> }
>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-04 23:19:25]

Alternatively, we could just leave the struct declarations as-is, and provide
only the param traits interface.  Downside is that you have to declare the
struct, and then register each member with IPC_, so its twice as many lines.

[Tom Sepez, 2011-02-04 23:23:20]

Anyways, here's the state of the world:

$ ls -l chrome/common/*messages.cc
-rw-r----- 1 tsepez eng     0 Feb  4 12:47 chrome/common/autofill_messages.cc
-rw-r----- 1 tsepez eng 20118 Feb  3 15:06 chrome/common/automation_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:55 chrome/common/database_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:55 chrome/common/devtools_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  4 12:12 chrome/common/dom_storage_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:55
chrome/common/file_utilities_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  4 14:40 chrome/common/gpu_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:54
chrome/common/mime_registry_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:54 chrome/common/nacl_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:54 chrome/common/pepper_file_messages.cc
-rw-r----- 1 tsepez eng  1153 Feb  3 17:54 chrome/common/pepper_messages.cc
-rw-r----- 1 tsepez eng  3207 Feb  3 18:26 chrome/common/plugin_messages.cc
-rw-r----- 1 tsepez eng 31284 Feb  4 15:10 chrome/common/render_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:52 chrome/common/service_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 16:48
chrome/common/speech_input_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 16:50 chrome/common/utility_messages.cc
-rw-r----- 1 tsepez eng     0 Feb  3 17:17 chrome/common/worker_messages.cc

many of these would get small constructors under the scheme in the previous
comment.

[jam, 2011-02-04 23:34:32]

I really like doing everything with the the macros, otherwise it's confusing
to list everything twice (and also error-prone).

Not sure why it's harder to migrate existing code, my gut feeling is almost
all IPC members are set at the caller.

On Fri, Feb 4, 2011 at 3:19 PM, <tsepez@chromium.org> wrote:

> Alternatively, we could just leave the struct declarations as-is, and
> provide
> only the param traits interface.  Downside is that you have to declare the
> struct, and then register each member with IPC_, so its twice as many
> lines.
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-07 18:29:40]

Latest patch set removes the _INIT() macros.

[Tom Sepez, 2011-02-07 18:39:39]

Patch set #13 removes the indexed db files I was using as a test.
This is hopefully the small non-distruptive version of the CL that could someday
be landed.

[jam, 2011-02-07 20:17:01]

this is looking very good, can't wait to see all of our code using it :)  what's
the plan for switching all the files to use it?

http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
File chrome/common/common_message_tree.h (right):

http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
chrome/common/common_message_tree.h:28: #include
"chrome/common/gpu_param_traits.h"
curious why this is needed?  without looking into it in details, it seems that
this should be in one of the gpu message headers?

http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
File ipc/ipc_message_constructor_macros.h (right):

http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
ipc/ipc_message_constructor_macros.h:21: #define IPC_STRUCT_END(struct_name) {}
why do we need struct_name in IPC_STRUCT_END?  can't we do the work that's done
there in the IPC_STRUCT_BEGIN?

http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h#newcod...
ipc/ipc_message_utils.h:124: // A dummy struct to place first just to allow
leading commas for all
this isn't needed anymore right?

http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macros.h
File ipc/ipc_param_traits_log_macros.h (right):

http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macro...
ipc/ipc_param_traits_log_macros.h:32: #define IPC_ENUM_END(enum_name)
IPC_ENUM_TRAITS_END(enum_name)
ditto: can we avoid listing the struct twice in IPC_ENUM_BEGIN and IPC_ENUM_END?

[Tom Sepez, 2011-02-07 20:36:37]

reworked the comments in ipc_message_macros in patch #16

[tsepez (do not use), 2011-02-07 20:47:35]

On Mon, Feb 7, 2011 at 12:17 PM, <jam@chromium.org> wrote:

> this is looking very good, can't wait to see all of our code using it :)
>  what's
> the plan for switching all the files to use it?
>
> Good question.   Hoping you and Darin can help out here; you've worked with
the message file owners before.  I'm more than willing to do the
implementation; more concerned about how these guys get pulled into the
loop.


>
>
>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
> File chrome/common/common_message_tree.h (right):
>
>
>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
> chrome/common/common_message_tree.h:28: #include
> "chrome/common/gpu_param_traits.h"
> curious why this is needed?  without looking into it in details, it
> seems that this should be in one of the gpu message headers?
>
> There are places where the same struct is used in XXX_messages.h and
YYY_messages.h.  Folks
have moved these common declarations into a separate traits file, which is
included (guarded) by
both XXX_messages.h and YYY_messages.h.   These will provide the appropriate
type definitions
to callers of both, but the ParamTraits for these types won't get
instantiated unless mentioned
explicity in this file.


>
>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
> File ipc/ipc_message_constructor_macros.h (right):
>
>
>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
> ipc/ipc_message_constructor_macros.h:21: #define
> IPC_STRUCT_END(struct_name) {}
> why do we need struct_name in IPC_STRUCT_END?  can't we do the work
> that's done there in the IPC_STRUCT_BEGIN?
>
> This is done for parallelism with IPC_ENUM_BEGIN, where it is needed.


> http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h
> File ipc/ipc_message_utils.h (right):
>
>
>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h#newcod...
> ipc/ipc_message_utils.h:124: // A dummy struct to place first just to
> allow leading commas for all
> this isn't needed anymore right?
>
No, we still need to do this.  We are still placing default constructors
(actually zero-arg initializers) into the initialization list for each type.


>
>
>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macros.h
> File ipc/ipc_param_traits_log_macros.h (right):
>
>
>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macro...
> ipc/ipc_param_traits_log_macros.h:32: #define IPC_ENUM_END(enum_name)
> IPC_ENUM_TRAITS_END(enum_name)
> ditto: can we avoid listing the struct twice in IPC_ENUM_BEGIN and
> IPC_ENUM_END?
>
>
> Nope.  we can't generate traits until the entire enum is defined.  We need
the name to generate traits.  There's no way to forward-declare an enum
(little quirk of C).


> http://codereview.chromium.org/6410007/
>

[jam, 2011-02-07 20:49:59]

On Mon, Feb 7, 2011 at 12:28 PM, Thomas Sepez <tsepez@google.com> wrote:

>
>
> On Mon, Feb 7, 2011 at 12:17 PM, <jam@chromium.org> wrote:
>
>> this is looking very good, can't wait to see all of our code using it :)
>>  what's
>> the plan for switching all the files to use it?
>>
>> Good question.   Hoping you and Darin can help out here; you've worked
> with the message file owners before.  I'm more than willing to do the
> implementation; more concerned about how these guys get pulled into the
> loop.
>

when i've done refactoring like this in the past, I've usually just gone in
and updated all the usage.  it's usually easy to find who to send it for
review to based on looking at the svn history of each file.


>
>
>>
>>
>>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
>> File chrome/common/common_message_tree.h (right):
>>
>>
>>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
>> chrome/common/common_message_tree.h:28: #include
>> "chrome/common/gpu_param_traits.h"
>> curious why this is needed?  without looking into it in details, it
>> seems that this should be in one of the gpu message headers?
>>
>> There are places where the same struct is used in XXX_messages.h and
> YYY_messages.h.  Folks
> have moved these common declarations into a separate traits file, which is
> included (guarded) by
> both XXX_messages.h and YYY_messages.h.   These will provide the
> appropriate type definitions
> to callers of both, but the ParamTraits for these types won't get
> instantiated unless mentioned
> explicity in this file.
>

It looks like this is only because of GPUInfo.
 ViewMsg_GpuChannelEstablished should just move into gpu_messages (which
already lists browser->renderer messages) and we can avoid this special
case.


>
>>
>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
>> File ipc/ipc_message_constructor_macros.h (right):
>>
>>
>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
>> ipc/ipc_message_constructor_macros.h:21: #define
>> IPC_STRUCT_END(struct_name) {}
>> why do we need struct_name in IPC_STRUCT_END?  can't we do the work
>> that's done there in the IPC_STRUCT_BEGIN?
>>
>> This is done for parallelism with IPC_ENUM_BEGIN, where it is needed.
>

I don't think we need to match that, if it's not needed  (i.e. why be
consistent with the macro for enums and not for message classes?).  Most
people just copy and paste, so for enums they'll start with another one and
it'll be easy to see that it needs to be added at the end.


>
>> http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h
>> File ipc/ipc_message_utils.h (right):
>>
>>
>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h#newcod...
>> ipc/ipc_message_utils.h:124: // A dummy struct to place first just to
>> allow leading commas for all
>> this isn't needed anymore right?
>>
> No, we still need to do this.  We are still placing default constructors
> (actually zero-arg initializers) into the initialization list for each type.
>
>

This wouldn't be needed if the members are initialized in the constructor's
body, right?  But I guess this avoids calling constructors twice, so I'm
fine with it.


>
>>
>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macros.h
>> File ipc/ipc_param_traits_log_macros.h (right):
>>
>>
>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macro...
>> ipc/ipc_param_traits_log_macros.h:32: #define IPC_ENUM_END(enum_name)
>> IPC_ENUM_TRAITS_END(enum_name)
>> ditto: can we avoid listing the struct twice in IPC_ENUM_BEGIN and
>> IPC_ENUM_END?
>>
>>
>> Nope.  we can't generate traits until the entire enum is defined.  We need
> the name to generate traits.  There's no way to forward-declare an enum
> (little quirk of C).
>

My reading of the code (correct me if I'm wrong), is that this is only used
in:

+#define IPC_ENUM_TRAITS_END(enum_name) \
+      default: \
+        state = "INVALID " #enum_name; \
+        break; \
+    } \
+    LogParam(state, l); \
+  }

I don't think it's that important to log the struct name when we get an
unknown value.

Also, for enums, I'm not convinced we need to use macros to declare each
item.  Why can't we just declare the enums normally, i.e. like today, and
then just have a macro that generates the serialization by casting it to an
int?


>
>> http://codereview.chromium.org/6410007/
>>
>
>

[darin (slow to review), 2011-02-07 21:33:07]

On Mon, Feb 7, 2011 at 12:49 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Mon, Feb 7, 2011 at 12:28 PM, Thomas Sepez <tsepez@google.com> wrote:
>
>>
>>
>> On Mon, Feb 7, 2011 at 12:17 PM, <jam@chromium.org> wrote:
>>
>>> this is looking very good, can't wait to see all of our code using it :)
>>>  what's
>>> the plan for switching all the files to use it?
>>>
>>> Good question.   Hoping you and Darin can help out here; you've worked
>> with the message file owners before.  I'm more than willing to do the
>> implementation; more concerned about how these guys get pulled into the
>> loop.
>>
>
> when i've done refactoring like this in the past, I've usually just gone in
> and updated all the usage.  it's usually easy to find who to send it for
> review to based on looking at the svn history of each file
>

in this case, a note to chromium-dev would be nice too as this is a
significant change.
-darin


> .
>
>
>>
>>
>>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
>>> File chrome/common/common_message_tree.h (right):
>>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
>>> chrome/common/common_message_tree.h:28: #include
>>> "chrome/common/gpu_param_traits.h"
>>> curious why this is needed?  without looking into it in details, it
>>> seems that this should be in one of the gpu message headers?
>>>
>>> There are places where the same struct is used in XXX_messages.h and
>> YYY_messages.h.  Folks
>> have moved these common declarations into a separate traits file, which is
>> included (guarded) by
>> both XXX_messages.h and YYY_messages.h.   These will provide the
>> appropriate type definitions
>> to callers of both, but the ParamTraits for these types won't get
>> instantiated unless mentioned
>> explicity in this file.
>>
>
> It looks like this is only because of GPUInfo.
>  ViewMsg_GpuChannelEstablished should just move into gpu_messages (which
> already lists browser->renderer messages) and we can avoid this special
> case.
>
>
>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
>>> File ipc/ipc_message_constructor_macros.h (right):
>>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
>>> ipc/ipc_message_constructor_macros.h:21: #define
>>> IPC_STRUCT_END(struct_name) {}
>>> why do we need struct_name in IPC_STRUCT_END?  can't we do the work
>>> that's done there in the IPC_STRUCT_BEGIN?
>>>
>>> This is done for parallelism with IPC_ENUM_BEGIN, where it is needed.
>>
>
> I don't think we need to match that, if it's not needed  (i.e. why be
> consistent with the macro for enums and not for message classes?).  Most
> people just copy and paste, so for enums they'll start with another one and
> it'll be easy to see that it needs to be added at the end.
>
>
>>
>>> http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h
>>> File ipc/ipc_message_utils.h (right):
>>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h#newcod...
>>> ipc/ipc_message_utils.h:124: // A dummy struct to place first just to
>>> allow leading commas for all
>>> this isn't needed anymore right?
>>>
>> No, we still need to do this.  We are still placing default constructors
>> (actually zero-arg initializers) into the initialization list for each type.
>>
>>
>
> This wouldn't be needed if the members are initialized in the constructor's
> body, right?  But I guess this avoids calling constructors twice, so I'm
> fine with it.
>
>
>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macros.h
>>> File ipc/ipc_param_traits_log_macros.h (right):
>>>
>>>
>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macro...
>>> ipc/ipc_param_traits_log_macros.h:32: #define IPC_ENUM_END(enum_name)
>>> IPC_ENUM_TRAITS_END(enum_name)
>>> ditto: can we avoid listing the struct twice in IPC_ENUM_BEGIN and
>>> IPC_ENUM_END?
>>>
>>>
>>> Nope.  we can't generate traits until the entire enum is defined.  We
>> need the name to generate traits.  There's no way to forward-declare an enum
>> (little quirk of C).
>>
>
> My reading of the code (correct me if I'm wrong), is that this is only used
> in:
>
> +#define IPC_ENUM_TRAITS_END(enum_name) \
> +      default: \
> +        state = "INVALID " #enum_name; \
> +        break; \
> +    } \
> +    LogParam(state, l); \
> +  }
>
> I don't think it's that important to log the struct name when we get an
> unknown value.
>
> Also, for enums, I'm not convinced we need to use macros to declare each
> item.  Why can't we just declare the enums normally, i.e. like today, and
> then just have a macro that generates the serialization by casting it to an
> int?
>
>
>>
>>> http://codereview.chromium.org/6410007/
>>>
>>
>>
>

[jam, 2011-02-07 21:35:50]

On Mon, Feb 7, 2011 at 1:09 PM, Thomas Sepez <tsepez@google.com> wrote:

>
>
> On Mon, Feb 7, 2011 at 12:49 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>>
>>
>> On Mon, Feb 7, 2011 at 12:28 PM, Thomas Sepez <tsepez@google.com> wrote:
>>
>>>
>>>
>>> On Mon, Feb 7, 2011 at 12:17 PM, <jam@chromium.org> wrote:
>>>
>>>> this is looking very good, can't wait to see all of our code using it :)
>>>>  what's
>>>> the plan for switching all the files to use it?
>>>>
>>>> Good question.   Hoping you and Darin can help out here; you've worked
>>> with the message file owners before.  I'm more than willing to do the
>>> implementation; more concerned about how these guys get pulled into the
>>> loop.
>>>
>>
>> when i've done refactoring like this in the past, I've usually just gone
>> in and updated all the usage.  it's usually easy to find who to send it for
>> review to based on looking at the svn history of each file.
>>
>>
> OK.  Sounds like a plan.
>
>>
>>>
>>>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
>>>> File chrome/common/common_message_tree.h (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/chrome/common/common_message_t...
>>>> chrome/common/common_message_tree.h:28: #include
>>>> "chrome/common/gpu_param_traits.h"
>>>> curious why this is needed?  without looking into it in details, it
>>>> seems that this should be in one of the gpu message headers?
>>>>
>>>> There are places where the same struct is used in XXX_messages.h and
>>> YYY_messages.h.  Folks
>>> have moved these common declarations into a separate traits file, which
>>> is included (guarded) by
>>> both XXX_messages.h and YYY_messages.h.   These will provide the
>>> appropriate type definitions
>>> to callers of both, but the ParamTraits for these types won't get
>>> instantiated unless mentioned
>>> explicity in this file.
>>>
>>
>> It looks like this is only because of GPUInfo.
>>  ViewMsg_GpuChannelEstablished should just move into gpu_messages (which
>> already lists browser->renderer messages) and we can avoid this special
>> case.
>>
>
> OK.  Makes sense.
>
>>
>>
>>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
>>>> File ipc/ipc_message_constructor_macros.h (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_constructor_ma...
>>>> ipc/ipc_message_constructor_macros.h:21: #define
>>>> IPC_STRUCT_END(struct_name) {}
>>>> why do we need struct_name in IPC_STRUCT_END?  can't we do the work
>>>> that's done there in the IPC_STRUCT_BEGIN?
>>>>
>>>> This is done for parallelism with IPC_ENUM_BEGIN, where it is needed.
>>>
>>
>> I don't think we need to match that, if it's not needed  (i.e. why be
>> consistent with the macro for enums and not for message classes?).  Most
>> people just copy and paste, so for enums they'll start with another one and
>> it'll be easy to see that it needs to be added at the end.
>>
> Ok.  Fair enough.
>
>>
>>
>>>
>>>> http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h
>>>> File ipc/ipc_message_utils.h (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_message_utils.h#newcod...
>>>> ipc/ipc_message_utils.h:124: // A dummy struct to place first just to
>>>> allow leading commas for all
>>>> this isn't needed anymore right?
>>>>
>>> No, we still need to do this.  We are still placing default constructors
>>> (actually zero-arg initializers) into the initialization list for each type.
>>>
>>>
>>
>> This wouldn't be needed if the members are initialized in the
>> constructor's body, right?  But I guess this avoids calling constructors
>> twice, so I'm fine with it.
>>
>>
> Right.  We agreed putting the constructors in the body would be a bad
> thing.
>
>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macros.h
>>>> File ipc/ipc_param_traits_log_macros.h (right):
>>>>
>>>>
>>>>
http://codereview.chromium.org/6410007/diff/42/ipc/ipc_param_traits_log_macro...
>>>> ipc/ipc_param_traits_log_macros.h:32: #define IPC_ENUM_END(enum_name)
>>>> IPC_ENUM_TRAITS_END(enum_name)
>>>> ditto: can we avoid listing the struct twice in IPC_ENUM_BEGIN and
>>>> IPC_ENUM_END?
>>>>
>>>>
>>>> Nope.  we can't generate traits until the entire enum is defined.  We
>>> need the name to generate traits.  There's no way to forward-declare an enum
>>> (little quirk of C).
>>>
>>
>> My reading of the code (correct me if I'm wrong), is that this is only
>> used in:
>>
>> +#define IPC_ENUM_TRAITS_END(enum_name) \
>> +      default: \
>> +        state = "INVALID " #enum_name; \
>> +        break; \
>> +    } \
>> +    LogParam(state, l); \
>> +  }
>>
>> I don't think it's that important to log the struct name when we get an
>> unknown value.
>>
> Almost.  The IPC_ENUM_TRAITS_END macro doesn't need it if we don't log it,
> but the IPC_ENUM_END() macro does.
>
>
>>
>> Also, for enums, I'm not convinced we need to use macros to declare each
>> item.  Why can't we just declare the enums normally, i.e. like today, and
>> then just have a macro that generates the serialization by casting it to an
>> int?
>>
>> Yes, you could do that, but I'd like to have the set of values available
> to me when I add a fuzz:: method to the param traits.
>
This is important; constraining to unexpected but in-range values vs. random
> out of range ...
>

This means that the enum has to be defined in chrome\common (or another
layer that knows about IPC), right?  This isn't how we do this now, since
sometimes the struct is defined in src\webkit, or even
src\third_party\WebKit (i.e. in WebKit repo), and we just define the traits
in src\chrome\common.  I think we still want to keep this, as otherwise we
would have to duplicate enums between these layers, or list each value
twice.

For examples, see WebMenuItem::Type (webkit\glue\webmenuitem.h) and
WebKit::WebMediaPlayerAction::Type
(third_party\WebKit\Source\WebKit\chromium\public\WebMediaPlayerAction.h).


>
>
>>
>>>
>>>> http://codereview.chromium.org/6410007/
>>>>
>>>
>>>
>>
>

[Tom Sepez, 2011-02-07 21:41:51]

Ok.  With the externally defined structs, yes we have to repeat the members in
an IPC_STRUCT_MEMBER(), but I could see saving having to duplicate the enums.

[Tom Sepez, 2011-02-07 21:49:59]

John, you're absolutely sure that the folks who spent the time implementing
those hand-coded loggers for enums aren't going to be annoyed that they have to
look up the value, right?  I'm taking this out now.  Let's hope we don't catch
too much flak for it.

[jam, 2011-02-07 21:51:28]

On Mon, Feb 7, 2011 at 1:41 PM, <tsepez@chromium.org> wrote:

> Ok.  With the externally defined structs, yes we have to repeat the members
> in
> an IPC_STRUCT_MEMBER(), but I could see saving having to duplicate the
> enums.
>

I don't understand what you mean?

I feel pretty strongly that duplicating enums is a no-go.


> http://codereview.chromium.org/6410007/
>

[jam, 2011-02-07 21:59:53]

On Mon, Feb 7, 2011 at 1:49 PM, <tsepez@chromium.org> wrote:

> John, you're absolutely sure that the folks who spent the time implementing
> those hand-coded loggers for enums aren't going to be annoyed that they
> have to
> look up the value, right?  I'm taking this out now.  Let's hope we don't
> catch
> too much flak for it.


Yep, send anyone who complains my way :)  IPC logging gets little usage, and
it'll still be possible to do what's done now, just with a little more
effort to look at the header.  The logging traits were just copied from
other file most likely.

>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-07 22:13:19]

Ok, one more look.  Fixed a few typos.  Thanks.

[jam, 2011-02-07 23:34:55]

http://codereview.chromium.org/6410007/diff/3052/chrome/common/common_message...
File chrome/common/common_message_tree.h (right):

http://codereview.chromium.org/6410007/diff/3052/chrome/common/common_message...
chrome/common/common_message_tree.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
nit: it's not clear when looking at the filename why it should be called tree?

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_constructor_...
File ipc/ipc_message_constructor_macros.h (right):

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_constructor_...
ipc/ipc_message_constructor_macros.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
nit: since this file isn't for ipc_messages, but for structs, how about just
struct_constructor_macros.h?

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
ipc/ipc_message_macros.h:1: // Copyright (c) 2006-2011 The Chromium Authors. All
rights reserved.
nit: while you're changing this, you can just make only 2011.  the date range
isn't required.

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
ipc/ipc_message_macros.h:19: //   - An inclusion of your XXX_messages.h file
inside common_message_tree.h
nit: the ipc module shouldn't refer (even in comments) to chrome code (i.e.
src\chrome\common_message_tree.h) since that's a layering violation.  i.e.
someone who uses ipc in a test might create a new messages.h, which we already
have.  we wouldn't want that to be put in common_message_tree.h

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
ipc/ipc_message_macros.h:40: // which declares types and is singly-evalutead,
and another which invokes
nit: evalutead

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
ipc/ipc_message_macros.h:46: //     #if IPC_MESSAGE_REINCLUDED ||
!defined(XXX_MESSAGES_SECTION)
out of curiosity: what would happen if we didn't have this guard?  it's not
needed for consumers of the messages (i.e. files that send/receive) right?

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
ipc/ipc_message_macros.h:1338: // Clean up IPC_MESSAGE_START in this unguarded
section.
I'm confused, why is this needed?

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_null_macros.h
File ipc/ipc_message_null_macros.h (right):

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_null_macros....
ipc/ipc_message_null_macros.h:9: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
nit: looks like the header and comment is included twice

[Tom Sepez, 2011-02-08 00:35:19]

On 2011/02/07 23:34:55, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6410007/diff/3052/chrome/common/common_message...
> File chrome/common/common_message_tree.h (right):
> 
>
http://codereview.chromium.org/6410007/diff/3052/chrome/common/common_message...
> chrome/common/common_message_tree.h:1: // Copyright (c) 2010 The Chromium
> Authors. All rights reserved.
> nit: it's not clear when looking at the filename why it should be called tree?
> 
> 

Indeed.  I was having a hard time deciding what it should be called.  It dawns
upon me that it should be called common_message_generator.h to go along with the
common_message_generator.cc that pulls it in.

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_constructor_...
> File ipc/ipc_message_constructor_macros.h (right):
> 
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_constructor_...
> ipc/ipc_message_constructor_macros.h:1: // Copyright (c) 2010 The Chromium
> Authors. All rights reserved.
> nit: since this file isn't for ipc_messages, but for structs, how about just
> struct_constructor_macros.h?
> 
Sure.  I like shorter names.
> http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h
> File ipc/ipc_message_macros.h (right):
> 
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
> ipc/ipc_message_macros.h:1: // Copyright (c) 2006-2011 The Chromium Authors.
All
> rights reserved.
> nit: while you're changing this, you can just make only 2011.  the date range
> isn't required.

NP.

>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
> ipc/ipc_message_macros.h:19: //   - An inclusion of your XXX_messages.h file
> inside common_message_tree.h
> nit: the ipc module shouldn't refer (even in comments) to chrome code (i.e.
> src\chrome\common_message_tree.h) since that's a layering violation.  i.e.
> someone who uses ipc in a test might create a new messages.h, which we already
> have.  we wouldn't want that to be put in common_message_tree.h
> 

Right.  Will have to add wording about "your generator file", an example of
which is ... common_ .
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
> ipc/ipc_message_macros.h:40: // which declares types and is singly-evalutead,
> and another which invokes
> nit: evalutead
> 
Yup.
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
> ipc/ipc_message_macros.h:46: //     #if IPC_MESSAGE_REINCLUDED ||
> !defined(XXX_MESSAGES_SECTION)
> out of curiosity: what would happen if we didn't have this guard?  it's not
> needed for consumers of the messages (i.e. files that send/receive) right?
> 
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
> ipc/ipc_message_macros.h:1338: // Clean up IPC_MESSAGE_START in this unguarded
> section.
> I'm confused, why is this needed?
> 
  CXX(target) out/Debug/obj.target/ppapi_proxy/ppapi/proxy/host_dispatcher.o
In file included from ./chrome/common/render_messages.h:535,
                 from
chrome/browser/debugger/extension_ports_remote_service.cc:25:
./chrome/common/render_messages_internal.h:73:1: error: "IPC_MESSAGE_START"
redefined
In file included from ./chrome/common/devtools_messages.h:14,
                 from ./chrome/browser/debugger/devtools_manager.h:15,
                 from
chrome/browser/debugger/extension_ports_remote_service.cc:18:
./chrome/common/devtools_messages_internal.h:43:1: error: this is the location
of the previous definition
In file included from ./chrome/common/render_messages.h:535,
                 from chrome/browser/debugger/debugger_remote_service.cc:22:
./chrome/common/render_messages_internal.h:73:1: error: "IPC_MESSAGE_START"
redefined
In file included from ./chrome/common/devtools_messages.h:14,
                 from ./chrome/browser/debugger/devtools_manager.h:15,
                 from chrome/browser/debugger/debugger_remote_service.cc:15:
./chrome/common/devtools_messages_internal.h:43:1: error: this is the location
of the previous definition

I put an include guard into ipc_message_macros.  The upshot is that in places
where there are multiple includes and files not cleaning up this symbol
themselves, we need to interleave an undef with every include of the ipc file.
> http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_null_macros.h
> File ipc/ipc_message_null_macros.h (right):
> 
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_null_macros....
> ipc/ipc_message_null_macros.h:9: // Copyright (c) 2010 The Chromium Authors.
All
> rights reserved.
> nit: looks like the header and comment is included twice
Thanks.

I'll incorporate these ideas and revise.

[jam, 2011-02-08 00:52:10]

On Mon, Feb 7, 2011 at 4:35 PM, <tsepez@chromium.org> wrote:

> On 2011/02/07 23:34:55, John Abd-El-Malek wrote:
>
>
>
http://codereview.chromium.org/6410007/diff/3052/chrome/common/common_message...
>
>> File chrome/common/common_message_tree.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/chrome/common/common_message...
>
>> chrome/common/common_message_tree.h:1: // Copyright (c) 2010 The Chromium
>> Authors. All rights reserved.
>> nit: it's not clear when looking at the filename why it should be called
>> tree?
>>
>
>
>
> Indeed.  I was having a hard time deciding what it should be called.  It
> dawns
> upon me that it should be called common_message_generator.h to go along
> with the
> common_message_generator.cc that pulls it in.
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_constructor_...
>
>> File ipc/ipc_message_constructor_macros.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_constructor_...
>
>> ipc/ipc_message_constructor_macros.h:1: // Copyright (c) 2010 The Chromium
>> Authors. All rights reserved.
>> nit: since this file isn't for ipc_messages, but for structs, how about
>> just
>> struct_constructor_macros.h?
>>
>
> Sure.  I like shorter names.
>
>  http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h
>> File ipc/ipc_message_macros.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
>
>> ipc/ipc_message_macros.h:1: // Copyright (c) 2006-2011 The Chromium
>> Authors.
>>
> All
>
>> rights reserved.
>> nit: while you're changing this, you can just make only 2011.  the date
>> range
>> isn't required.
>>
>
> NP.
>
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
>
>> ipc/ipc_message_macros.h:19: //   - An inclusion of your XXX_messages.h
>> file
>> inside common_message_tree.h
>> nit: the ipc module shouldn't refer (even in comments) to chrome code
>> (i.e.
>> src\chrome\common_message_tree.h) since that's a layering violation.  i.e.
>> someone who uses ipc in a test might create a new messages.h, which we
>> already
>> have.  we wouldn't want that to be put in common_message_tree.h
>>
>
>
> Right.  Will have to add wording about "your generator file", an example of
> which is ... common_ .
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
>
>> ipc/ipc_message_macros.h:40: // which declares types and is
>> singly-evalutead,
>> and another which invokes
>> nit: evalutead
>>
>
> Yup.
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
>
>> ipc/ipc_message_macros.h:46: //     #if IPC_MESSAGE_REINCLUDED ||
>> !defined(XXX_MESSAGES_SECTION)
>> out of curiosity: what would happen if we didn't have this guard?  it's
>> not
>> needed for consumers of the messages (i.e. files that send/receive) right?
>>
>
Does your comment below address this as well?  It's not clear to me.  Can
you explain why this code is needed, and if we can get rid of it?


>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
>
>> ipc/ipc_message_macros.h:1338: // Clean up IPC_MESSAGE_START in this
>> unguarded
>> section.
>> I'm confused, why is this needed?
>>
>
>  CXX(target) out/Debug/obj.target/ppapi_proxy/ppapi/proxy/host_dispatcher.o
> In file included from ./chrome/common/render_messages.h:535,
>                 from
> chrome/browser/debugger/extension_ports_remote_service.cc:25:
> ./chrome/common/render_messages_internal.h:73:1: error: "IPC_MESSAGE_START"
> redefined
> In file included from ./chrome/common/devtools_messages.h:14,
>                 from ./chrome/browser/debugger/devtools_manager.h:15,
>                 from
> chrome/browser/debugger/extension_ports_remote_service.cc:18:
> ./chrome/common/devtools_messages_internal.h:43:1: error: this is the
> location
> of the previous definition
> In file included from ./chrome/common/render_messages.h:535,
>                 from chrome/browser/debugger/debugger_remote_service.cc:22:
> ./chrome/common/render_messages_internal.h:73:1: error: "IPC_MESSAGE_START"
> redefined
> In file included from ./chrome/common/devtools_messages.h:14,
>                 from ./chrome/browser/debugger/devtools_manager.h:15,
>                 from chrome/browser/debugger/debugger_remote_service.cc:15:
> ./chrome/common/devtools_messages_internal.h:43:1: error: this is the
> location
> of the previous definition
>
> I put an include guard into ipc_message_macros.  The upshot is that in
> places
> where there are multiple includes and files not cleaning up this symbol
> themselves, we need to interleave an undef with every include of the ipc
> file.


This undef will be interleaved in which include, you mean in the includes in
src\ipc?  If so, then that's preferable since that code is written once, as
opposed to the ipc headers in chrome\common which is n times.


>
>
>>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_null_macros.h
>> File ipc/ipc_message_null_macros.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_null_macros....
>
>> ipc/ipc_message_null_macros.h:9: // Copyright (c) 2010 The Chromium
>> Authors.
>>
> All
>
>> rights reserved.
>> nit: looks like the header and comment is included twice
>>
> Thanks.
>
> I'll incorporate these ideas and revise.
>
>
>
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-08 01:14:05]

Here's another set.  But did you want struct_constructor_macros.h or
ipc_struct_constructor_macros.h as the new file name?  Looks like everything
else begins with ipc_ in that directory.

[jam, 2011-02-08 19:08:07]

On Mon, Feb 7, 2011 at 5:14 PM, <tsepez@chromium.org> wrote:

> Here's another set.  But did you want struct_constructor_macros.h or
> ipc_struct_constructor_macros.h as the new file name?  Looks like
> everything
> else begins with ipc_ in that directory.


most files begin with ipc_ because they used to be in chrome\common.  when
they moved to ipc\, they should dropped the "ipc_" part (just like how we
don't prefix "base_" to files in src\base).  it'd be good for new files to
not start with ipc_.

>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-09 22:20:12]

One more cut at this in change set #20 ... fire away, my friends.  Thanks.

[jam, 2011-02-09 23:06:14]

this is looking very good (i.e. lgtm at this point).  can you convert the
indexed db header so that we can see what an example looks like?

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

http://codereview.chromium.org/6410007/diff/3052/ipc/ipc_message_macros.h#new...
ipc/ipc_message_macros.h:40: // which declares types and is singly-evalutead,
and another which invokes
On 2011/02/07 23:34:55, John Abd-El-Malek wrote:
> nit: evalutead

ping

http://codereview.chromium.org/6410007/diff/12001/ipc/ipc_message_macros.h#ne...
ipc/ipc_message_macros.h:1365: 
wouldn't it be better to just do it only in this file, and hence not depend on
each file remembering to do it?

http://codereview.chromium.org/6410007/diff/12001/ipc/ipc_param_traits_log_ma...
File ipc/ipc_param_traits_log_macros.h (right):

http://codereview.chromium.org/6410007/diff/12001/ipc/ipc_param_traits_log_ma...
ipc/ipc_param_traits_log_macros.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
nit: i think all the ipc_param_* files can lose the "ipc_" prefix

[Tom Sepez, 2011-02-09 23:18:07]

I've fixed the "evaluated" (finally).  
I've changed the comment about cleanup IPC_MESSAGE_START to make it a permanent
feature.

But I'm reluctant to rename all the ipc_param_trait_* files, to keep them
consistent with the existing ipc_param_traits.h.  Reoving this one's prefix may
touch a bunch of places.

I'm also reluctant to check in the new indexed_db file as part of this CL.  Is
that what you had in mind?  I wanted these to go in their own CLs later.  

I could upload the indexed_db.h again for review purposes.  
Let me know what you think.

[jam, 2011-02-09 23:28:07]

On Wed, Feb 9, 2011 at 3:18 PM, <tsepez@chromium.org> wrote:

> I've fixed the "evaluated" (finally).
> I've changed the comment about cleanup IPC_MESSAGE_START to make it a
> permanent
> feature.
>
> But I'm reluctant to rename all the ipc_param_trait_* files, to keep them
> consistent with the existing ipc_param_traits.h.  Reoving this one's prefix
> may
> touch a bunch of places.
>

There are about 18 files that include it, we can easily switch them over in
a different cl, as well as rename ipc_param_traits.h  But we should name the
new files as best as we can, and not add redundant prefixes because there's
one bad example already present (it's also inconsistent with how we name
files in other modules).


> I'm also reluctant to check in the new indexed_db file as part of this CL.
>  Is
> that what you had in mind?  I wanted these to go in their own CLs later.
>

Yeah I was thinking just so we can see it, but I don't feel strongly, and a
separate CL is fine.

>
> I could upload the indexed_db.h again for review purposes.
> Let me know what you think.
>
>
>
>
> http://codereview.chromium.org/6410007/
>

[jam, 2011-02-10 00:22:19]

indexed_db_messages looks great :)

http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
File chrome/common/indexed_db_messages.h (right):

http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
chrome/common/indexed_db_messages.h:6: #define
CHROME_COMMON_INDEXED_DB_MESSAGES_H_
we don't need the ifdef guard since all the includes have their own right?

http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
chrome/common/indexed_db_messages.h:408: #undef IPC_MESSAGE_START
this isn't be needed right?

http://codereview.chromium.org/6410007/diff/12020/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

http://codereview.chromium.org/6410007/diff/12020/ipc/ipc_message_macros.h#ne...
ipc/ipc_message_macros.h:1365: // XXX_message.h files fail do so themselves.
nit: since we'll only want to undef in this file, the comment shouldn't say that
the message headers should also do it

[tsepez (do not use), 2011-02-10 00:26:42]

On Wed, Feb 9, 2011 at 4:22 PM, <jam@chromium.org> wrote:

> indexed_db_messages looks great :)
>
>
>
>
http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
> File chrome/common/indexed_db_messages.h (right):
>
>
>
http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
> chrome/common/indexed_db_messages.h:6: #define
> CHROME_COMMON_INDEXED_DB_MESSAGES_H_
> we don't need the ifdef guard since all the includes have their own
> right?
>
In this particular case, you might get away with it.  But if you ever had to
introduce any type definitions, etc, then this breaks.  Better to keep it
around.   The convention of having macros after the includes may seem a
little strange, but it works in practice.



>
>
>
http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
> chrome/common/indexed_db_messages.h:408: #undef IPC_MESSAGE_START
> this isn't be needed right?
>
> It's best for these files to clean up after themselves, just because.
 Polluting the namespace with definitions that aren't needed beyond this
scope seems bad.


> http://codereview.chromium.org/6410007/diff/12020/ipc/ipc_message_macros.h
>
> File ipc/ipc_message_macros.h (right):
>
>
>
http://codereview.chromium.org/6410007/diff/12020/ipc/ipc_message_macros.h#ne...
> ipc/ipc_message_macros.h:1365: // XXX_message.h files fail do so
> themselves.
> nit: since we'll only want to undef in this file, the comment shouldn't
> say that the message headers should also do it
>
> Disagree. See comment about namespace pollution.

>
> http://codereview.chromium.org/6410007/
>

[jam, 2011-02-10 02:06:14]

On Wed, Feb 9, 2011 at 4:26 PM, Thomas Sepez <tsepez@google.com> wrote:

>
>
> On Wed, Feb 9, 2011 at 4:22 PM, <jam@chromium.org> wrote:
>
>> indexed_db_messages looks great :)
>>
>>
>>
>>
http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
>> File chrome/common/indexed_db_messages.h (right):
>>
>>
>>
http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
>> chrome/common/indexed_db_messages.h:6: #define
>> CHROME_COMMON_INDEXED_DB_MESSAGES_H_
>> we don't need the ifdef guard since all the includes have their own
>> right?
>>
> In this particular case, you might get away with it.  But if you ever had
> to introduce any type definitions, etc, then this breaks.
>
Better to keep it around.   The convention of having macros after the
> includes may seem a little strange, but it works in practice.
>

It doesn't look clear that if someone introduces a typedef, they would have
to do it in the include guard above.  Having the ifdef guard around headers
that already have it seems redundant.  If it's required for a section that
can't be included multiple times, it seems we should only add the ifguard
then, and only around the area that needs it.  For most files that don't
have typedefs, they can then have a simpler format, and not have to think
about where to start/end the ifguard.

>
>
>
>>
>>
>>
http://codereview.chromium.org/6410007/diff/12020/chrome/common/indexed_db_me...
>> chrome/common/indexed_db_messages.h:408: #undef IPC_MESSAGE_START
>> this isn't be needed right?
>>
>> It's best for these files to clean up after themselves, just because.
>

The goal with the message headers has been to make them as easy as possible
to write.  This encourages people to putmessages for new features in new
files, and not use existing ones.  When we used to have files that needed
ifdefs in the middle, multiple files had to include it, etc, developers were
 incentivized to add everything into one giant file
(render_messages_internal.h). Anything we can do to make it easier and less
work to create IPC files will have a multiplicative effect.  Undeffing the
IPC_MESSAGE_START is something that can be done by ipc_message_macros.h,
which is already done, so I think given that this change makes the files
even easier to write, it shouldn't take a step backwards for this one ifdef.


>  Polluting the namespace with definitions that aren't needed beyond this
> scope seems bad.
>


>
>
>> http://codereview.chromium.org/6410007/diff/12020/ipc/ipc_message_macros.h
>>
>> File ipc/ipc_message_macros.h (right):
>>
>>
>>
http://codereview.chromium.org/6410007/diff/12020/ipc/ipc_message_macros.h#ne...
>> ipc/ipc_message_macros.h:1365: // XXX_message.h files fail do so
>> themselves.
>> nit: since we'll only want to undef in this file, the comment shouldn't
>> say that the message headers should also do it
>>
>> Disagree. See comment about namespace pollution.
>
>>
>> http://codereview.chromium.org/6410007/
>>
>
>

[Tom Sepez, 2011-02-10 02:52:20]

Ok. We'll do it your way on the #undef IPC_MESSAGE_START.  It just doesn't
matter.  

When converting over an existing message file, if you proceed one message at a
time, you do in fact wind up with a rather large section in the middle which
contains the old traits declarations not yet migrated which have to be #ifdef'd.
  

Giving a simple description of how to to this may be more difficult than
describing the current guard situation.

[Tom Sepez, 2011-02-10 17:31:39]

Yet another patch set.  No include guards.  You may want to proof the language
in ipc_message_macros.h.  Thanks.

[jam, 2011-02-10 18:26:44]

On Wed, Feb 9, 2011 at 6:52 PM, <tsepez@chromium.org> wrote:

> Ok. We'll do it your way on the #undef IPC_MESSAGE_START.  It just doesn't
> matter.
>
> When converting over an existing message file, if you proceed one message
> at a
> time, you do in fact wind up with a rather large section in the middle
> which
> contains the old traits declarations not yet migrated which have to be
> #ifdef'd.
>

Why would we convert part of a file at a time?  It seems easier to go one
file at a time.


>
>
> Giving a simple description of how to to this may be more difficult than
> describing the current guard situation.
>
>
> http://codereview.chromium.org/6410007/
>

[Tom Sepez, 2011-02-10 18:32:31]

> Why would we convert part of a file at a time?  It seems easier to go > one
file at a time.

For a large file like render_messages.h, I'd approach it in smaller pieces.  But
that's just because I like smaller, more isolated CLs.
Doesn't matter either way.

[jam, 2011-02-10 18:43:33]

lgtm

http://codereview.chromium.org/6410007/diff/20001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

http://codereview.chromium.org/6410007/diff/20001/ipc/ipc_message_macros.h#ne...
ipc/ipc_message_macros.h:79: // definitions present in your XXX_messages.h file.
 Ideally, you'd move
btw thinking more about this, I think typedefs in headers will cause us problems
either way, so we should probably try to avoid them.  the issue is that most
likely a typedef will be used in a message (as opposed to a struct), and so
it'll need to be used as an argument in the IPC dispatcher.  Since that function
is most likely declared in a header, we'd then have to redeclare the typedef or
include the header in the header.  both of these seem non-ideal.

http://codereview.chromium.org/6410007/diff/20001/ipc/ipc_message_macros.h#ne...
ipc/ipc_message_macros.h:91: //      #ifndef SOME_GUARD_MACRO
nit: we don't really need an ifdef guard around forward declaration right?

[Tom Sepez, 2011-02-10 18:55:54]

On 2011/02/10 18:43:33, John Abd-El-Malek wrote:
> lgtm
> 
> http://codereview.chromium.org/6410007/diff/20001/ipc/ipc_message_macros.h
> File ipc/ipc_message_macros.h (right):
> 
>
http://codereview.chromium.org/6410007/diff/20001/ipc/ipc_message_macros.h#ne...
> ipc/ipc_message_macros.h:79: // definitions present in your XXX_messages.h
file.
>  Ideally, you'd move
> btw thinking more about this, I think typedefs in headers will cause us
problems
> either way, so we should probably try to avoid them.  the issue is that most
> likely a typedef will be used in a message (as opposed to a struct), and so
> it'll need to be used as an argument in the IPC dispatcher.  Since that
function
> is most likely declared in a header, we'd then have to redeclare the typedef
or
> include the header in the header.  both of these seem non-ideal.
> 
> 

I'll try and wordsmith this to stress the non-ideal nature.  I need to turn a
new version anyway to deal with the whitespace at end of lines that got missed
because pre-submit checks aren't working.

http://codereview.chromium.org/6410007/diff/20001/ipc/ipc_message_macros.h#ne...
> ipc/ipc_message_macros.h:91: //      #ifndef SOME_GUARD_MACRO
> nit: we don't really need an ifdef guard around forward declaration right?

In the current implementation, you'd probably be able to get away with this, but
there are cases where we might have issues if we wanted to do other things.  For
example, if we were building the table of messages that I showed a few weeks
ago, then the #include of the message generator would occur inside some
initializer, ie. I don't want to preclude cases like:

#define IPC_MESSAGE_SOMETHING(...) { some intialzier },
some_table my_table = {
#include "common_message_generator.h"
};

in which case any "noise" from the headers apart from the macros would not be
ok.

[Tom Sepez, 2011-02-10 19:29:56]

New wording in ipc-message_macros discouraging type declarations.

[jam, 2011-02-10 21:24:27]

lgtm

On Thu, Feb 10, 2011 at 11:29 AM, <tsepez@chromium.org> wrote:

> New wording in ipc-message_macros discouraging type declarations.
>
>
> http://codereview.chromium.org/6410007/
>

[darin (slow to review), 2011-02-10 21:33:24]

http://codereview.chromium.org/6410007/diff/22006/chrome/common/indexed_db_me...
File chrome/common/indexed_db_messages.h (right):

http://codereview.chromium.org/6410007/diff/22006/chrome/common/indexed_db_me...
chrome/common/indexed_db_messages.h:120: IPC_STRUCT_MEMBER(int, transaction_id)
indentation nit