Make it easier to create new IPC channel types (i.e. renderer/plugin). Inste...

Make it easier/less work/less error-prone to create new IPC channel types (i.e. renderer/plugin).Instead of having each message file include the internal one several times with different ifdefs, move that logic to ipc_message_macros.h.  Also make the message class starting IDs come from an enum to ensure we don't use a value twice.  I simplified the logging code a bit so we don't need X_messages.cc files.Clean up places that we were doing manual packing/unpacking.  Most of this was in the automation code.  I added a few new template functions to make it convenient to read the parameters from a message, and updated the code to use them.I also removed unnecessary includes of render/plugin_messages.h from headers to speed up compiling.I moved the traits of IPC structs beside the struct definition to make it more apparent what's going on, so we avoid people modifying the struct and forgetting to update the traits.Amit: please look at chrome/test/automation/tab_proxy.ccMarc-Antoine: chrome/browser/printing/*Matt: the rest

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

[jam, 2009-02-03 09:34:32]

http://codereview.chromium.org/20015/diff/1/11
File chrome/common/ipc_fuzzing_tests.cc (right):

http://codereview.chromium.org/20015/diff/1/11#newcode99
Line 99: // We don't actually use the messages defined in this fiele, but we do
this
Note: I fixed the spelling here after uploading.

[amit, 2009-02-03 18:53:06]

nice work!

http://codereview.chromium.org/20015/diff/1/5
File chrome/test/automation/tab_proxy.cc (right):

http://codereview.chromium.org/20015/diff/1/5#newcode121
Line 121: return matches;
should set active_ordinal before returning

http://codereview.chromium.org/20015/diff/1/5#newcode237
Line 237: int TabProxy::Reload() {
Most of the above functions have very similar code. So perhaps a template
function to reduce redundancy?

http://codereview.chromium.org/20015/diff/1/5#newcode259
Line 259: if (succeeded) {
Would be better use the same pattern to club this check together with  the
SendAndWaitForResponse.

http://codereview.chromium.org/20015/diff/1/5#newcode606
Line 606: AutomationMsg_CloseTabResponse::Read(response, &succeeded);
passing 'succeeded' to AutomationMsg_CloseTabResponse::Read would overwrite the
reference value returned from Read with the return value of Read. Please check
other places in this file with similar issue.

I think adding && succeeded at the end would correct this but would be confusing
so its better to have a separate bool 'result'.

http://codereview.chromium.org/20015/diff/1/5#newcode681
Line 681: bool succeeded;
why separate declaration?

http://codereview.chromium.org/20015/diff/1/5#newcode702
Line 702: AutomationMsg_ActionOnSSLBlockingPageResponse::ID, INFINITE,
&is_timeout) &&
overflowing 80 char margin

[M-A Ruel, 2009-02-03 19:35:06]

Looks good to me. Most of the comments are nits except the enum thing. I would
prefer to move some utility functions to a corresponding .cc file but that can
be done afterward.

http://codereview.chromium.org/20015/diff/1/37
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/20015/diff/1/37#newcode119
Line 119: const ViewHostMsg_Accessibility_Out_Params&
BrowserAccessibilityManager::response() {
80 cols

http://codereview.chromium.org/20015/diff/1/28
File chrome/browser/printing/print_view_manager.cc (left):

http://codereview.chromium.org/20015/diff/1/28#oldcode249
Line 249: DCHECK_GT(print_params_.document_cookie, 0);
I'm making a change which will delete most of this code but that's fine if you
want to check that in first.

http://codereview.chromium.org/20015/diff/1/32
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/20015/diff/1/32#newcode23
Line 23: enum ViewHostMsg_ImeControl;
You can't forward declare an enum, it's not valid C++ and gcc will not compile
it.

http://codereview.chromium.org/20015/diff/1/41
File chrome/common/ipc_logging.cc (right):

http://codereview.chromium.org/20015/diff/1/41#newcode42
Line 42: LogFunction* g_log_function_mapping[16];
16 = ? Please use a constant or at least comment on where it comes from., e.g.
bits 16-12 from a message?

http://codereview.chromium.org/20015/diff/1/16
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20015/diff/1/16#newcode42
Line 42: // compatability with old builds.
compatibility

http://codereview.chromium.org/20015/diff/1/16#newcode55
Line 55: // than 16 entries, that code needs to be updated.
LAST_IPC_MESSAGE_START,

COMPILED_ASSERT(LAST_IPC_MESSAGE_START <= 16);

Maybe overkill but still.

http://codereview.chromium.org/20015/diff/1/21
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/20015/diff/1/21#newcode394
Line 394: static void Log(const param_type& p, std::wstring* l) {
This won't build on linux&mac? Could the implementation be in the .cc file so
this header wouldn't require windows.h?

http://codereview.chromium.org/20015/diff/1/6
File chrome/test/automation/automation_messages.h (right):

http://codereview.chromium.org/20015/diff/1/6#newcode19
Line 19: #define MESSAGES_INTERNAL_FILE
"chrome/test/automation/automation_messages_internal.h"
80 cols. Use \

[Matt Perry, 2009-02-03 20:08:59]

nice cleanup!

http://codereview.chromium.org/20015/diff/1/12
File chrome/common/ipc_message_macros.h (right):

http://codereview.chromium.org/20015/diff/1/12#newcode98
Line 98: #error This file should only be included by X_messages.h, which needs
to define MESSAGES_INTERNAL_FILE first.
Can this be wrapped using \ ?  Or does this work:
#error "foo "
       "continued foo."

http://codereview.chromium.org/20015/diff/1/12#newcode101
Line 101: #ifndef IPC_MESSAGE_MACROS_INCLUDE_BLOCK
I think this block would make more sense at the top of the file.  We don't need
to #undef everything before the first pass anyway, right?

http://codereview.chromium.org/20015/diff/1/12#newcode107
Line 107: #include MESSAGES_INTERNAL_FILE
Wow, I didn't know you could #include a macro.  Cool.

http://codereview.chromium.org/20015/diff/1/12#newcode134
Line 134: label##Start = label##MsgStart << 12, \
nit: add
  _YouForgotToAddThisMessageTypeTo_ipc_message_utils_h = label##MsgStart

or some such, so that a bogus label will generate a somewhat understandable
error message.

http://codereview.chromium.org/20015/diff/1/21
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/20015/diff/1/21#newcode77
Line 77: struct PluginHostMsg_URLRequest_Params {
Any reason why these Params structs shouldn't be in the IPC namespace?

[jam, 2009-02-03 21:16:40]

Thanks, updated per comments.  I also cleaned up the ipc_logging stuff a bit
more & fixed some compile errors on unit_tests.

http://codereview.chromium.org/20015/diff/1/37
File chrome/browser/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/20015/diff/1/37#newcode119
Line 119: const ViewHostMsg_Accessibility_Out_Params&
BrowserAccessibilityManager::response() {
On 2009/02/03 19:35:06, M-A wrote:
> 80 cols

I wasn't a big fan of putting the return type on a different line, but sure

http://codereview.chromium.org/20015/diff/1/28
File chrome/browser/printing/print_view_manager.cc (left):

http://codereview.chromium.org/20015/diff/1/28#oldcode249
Line 249: DCHECK_GT(print_params_.document_cookie, 0);
On 2009/02/03 19:35:06, M-A wrote:
> I'm making a change which will delete most of this code but that's fine if you
> want to check that in first.

yeah i saw your recent checkins with this code, but I needed to remove this in
order to get rid of the render_messages.h include in the header.

http://codereview.chromium.org/20015/diff/1/32
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/20015/diff/1/32#newcode23
Line 23: enum ViewHostMsg_ImeControl;
On 2009/02/03 19:35:06, M-A wrote:
> You can't forward declare an enum, it's not valid C++ and gcc will not compile
> it.

ah I didn't know. ok I changed this to an int in the message handler to avoid
bringing in render_messages.h in a header & added a comment why we're doing
this.

http://codereview.chromium.org/20015/diff/1/41
File chrome/common/ipc_logging.cc (right):

http://codereview.chromium.org/20015/diff/1/41#newcode42
Line 42: LogFunction* g_log_function_mapping[16];
On 2009/02/03 19:35:06, M-A wrote:
> 16 = ? Please use a constant or at least comment on where it comes from., e.g.
> bits 16-12 from a message?

made it use the LastMsgIndex from IPCMessageStart

http://codereview.chromium.org/20015/diff/1/12
File chrome/common/ipc_message_macros.h (right):

http://codereview.chromium.org/20015/diff/1/12#newcode98
Line 98: #error This file should only be included by X_messages.h, which needs
to define MESSAGES_INTERNAL_FILE first.
On 2009/02/03 20:08:59, Matt Perry wrote:
> Can this be wrapped using \ ?  Or does this work:
> #error "foo "
>        "continued foo."

not sure, but either way this file doesn't obey the 80 character limit, so I
think it's ok like this..

http://codereview.chromium.org/20015/diff/1/12#newcode101
Line 101: #ifndef IPC_MESSAGE_MACROS_INCLUDE_BLOCK
On 2009/02/03 20:08:59, Matt Perry wrote:
> I think this block would make more sense at the top of the file.  We don't
need
> to #undef everything before the first pass anyway, right?

good idea, done

http://codereview.chromium.org/20015/diff/1/12#newcode134
Line 134: label##Start = label##MsgStart << 12, \
On 2009/02/03 20:08:59, Matt Perry wrote:
> nit: add
>   _YouForgotToAddThisMessageTypeTo_ipc_message_utils_h = label##MsgStart
> 
> or some such, so that a bogus label will generate a somewhat understandable
> error message.

yeah I wanted to have something like this, but I couldn't figure out how to do
it.  We'd really need something like 

int _YouForgotToAddThisMessageTypeTo_ipc_message_utils_h = label##MsgStart; \

but this file gets included in a lot of places so we'd get multiple defined
symbols linker errors..

http://codereview.chromium.org/20015/diff/1/16
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/20015/diff/1/16#newcode42
Line 42: // compatability with old builds.
On 2009/02/03 19:35:06, M-A wrote:
> compatibility

Done.

http://codereview.chromium.org/20015/diff/1/16#newcode55
Line 55: // than 16 entries, that code needs to be updated.
On 2009/02/03 19:35:06, M-A wrote:
> LAST_IPC_MESSAGE_START,
> 
> COMPILED_ASSERT(LAST_IPC_MESSAGE_START <= 16);
> 
> Maybe overkill but still.

Done.

http://codereview.chromium.org/20015/diff/1/21
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/20015/diff/1/21#newcode77
Line 77: struct PluginHostMsg_URLRequest_Params {
On 2009/02/03 20:08:59, Matt Perry wrote:
> Any reason why these Params structs shouldn't be in the IPC namespace?

yes, so when you send a message that uses them (which isn't in the IPC
namespace), you don't have to create its params from the IPC namespace.

http://codereview.chromium.org/20015/diff/1/6
File chrome/test/automation/automation_messages.h (right):

http://codereview.chromium.org/20015/diff/1/6#newcode19
Line 19: #define MESSAGES_INTERNAL_FILE
"chrome/test/automation/automation_messages_internal.h"
On 2009/02/03 19:35:06, M-A wrote:
> 80 cols. Use \
> 

Done.

http://codereview.chromium.org/20015/diff/1/5
File chrome/test/automation/tab_proxy.cc (right):

http://codereview.chromium.org/20015/diff/1/5#newcode121
Line 121: return matches;
On 2009/02/03 18:53:06, amit wrote:
> should set active_ordinal before returning

oops, I had added the extra ordinal variable before I updated the macros and
forgot to take it out.  fixed.

http://codereview.chromium.org/20015/diff/1/5#newcode237
Line 237: int TabProxy::Reload() {
On 2009/02/03 18:53:06, amit wrote:
> Most of the above functions have very similar code. So perhaps a template
> function to reduce redundancy?

The proper thing is to switch this code to using SyncChannel.  It would clean
things up a lot more, the caller wouldn't have to declare the response message,
and the input/output parameters would be together in one message class...

http://codereview.chromium.org/20015/diff/1/5#newcode259
Line 259: if (succeeded) {
On 2009/02/03 18:53:06, amit wrote:
> Would be better use the same pattern to club this check together with  the
> SendAndWaitForResponse.

like above, this code really needs to switch to using SyncChannel..

http://codereview.chromium.org/20015/diff/1/5#newcode606
Line 606: AutomationMsg_CloseTabResponse::Read(response, &succeeded);
On 2009/02/03 18:53:06, amit wrote:
> passing 'succeeded' to AutomationMsg_CloseTabResponse::Read would overwrite
the
> reference value returned from Read with the return value of Read. Please check
> other places in this file with similar issue.
> 
> I think adding && succeeded at the end would correct this but would be
confusing
> so its better to have a separate bool 'result'. 

good catch, I did your suggestion for now.  I hate to sound like a broken record
but really all this code should go away once SyncChannel is used.

http://codereview.chromium.org/20015/diff/1/5#newcode681
Line 681: bool succeeded;
On 2009/02/03 18:53:06, amit wrote:
> why separate declaration?

value is separate from succeeded...

http://codereview.chromium.org/20015/diff/1/5#newcode702
Line 702: AutomationMsg_ActionOnSSLBlockingPageResponse::ID, INFINITE,
&is_timeout) &&
On 2009/02/03 18:53:06, amit wrote:
> overflowing 80 char margin

Done.

[Matt Perry, 2009-02-03 21:31:30]

LG

http://codereview.chromium.org/20015/diff/1/12
File chrome/common/ipc_message_macros.h (right):

http://codereview.chromium.org/20015/diff/1/12#newcode134
Line 134: label##Start = label##MsgStart << 12, \
On 2009/02/03 21:16:40, John Abd-El-Malek wrote:
> On 2009/02/03 20:08:59, Matt Perry wrote:
> > nit: add
> >   _YouForgotToAddThisMessageTypeTo_ipc_message_utils_h = label##MsgStart
> > 
> > or some such, so that a bogus label will generate a somewhat understandable
> > error message.
> 
> yeah I wanted to have something like this, but I couldn't figure out how to do
> it.  We'd really need something like 
> 
> int _YouForgotToAddThisMessageTypeTo_ipc_message_utils_h = label##MsgStart; \
> 
> but this file gets included in a lot of places so we'd get multiple defined
> symbols linker errors..

Wouldn't adding it as an enum value achieve this effect?

[amit, 2009-02-03 21:44:01]

OK

http://codereview.chromium.org/20015/diff/1/5
File chrome/test/automation/tab_proxy.cc (right):

http://codereview.chromium.org/20015/diff/1/5#newcode259
Line 259: if (succeeded) {
On 2009/02/03 21:16:40, John Abd-El-Malek wrote:
> On 2009/02/03 18:53:06, amit wrote:
> > Would be better use the same pattern to club this check together with  the
> > SendAndWaitForResponse.
> 
> like above, this code really needs to switch to using SyncChannel..
> 

Well, in some cases of external tabs of we don't control the message loop, so
switching to SyncChannel is not easy. :(

[M-A Ruel, 2009-02-03 21:48:00]

Some small nits, no need to reupload for these.

About ViewHostMsg_ImeControl, maybe you should extract it in its own header?
That's what have been done for many enums so we don't need to use a int. As you
prefer.

http://codereview.chromium.org/20015/diff/1061/1090
File chrome/browser/tab_contents/ipc_status_view.cc (right):

http://codereview.chromium.org/20015/diff/1061/1090#newcode69
Line 69: logger->RegisterMessageLogger(PluginProcessHostStart,
PluginProcessHostMsgLog);
80 cols, please visit
http://dev.chromium.org/developers/how-tos/visualstudio-tricks :)

http://codereview.chromium.org/20015/diff/1061/1071
File chrome/common/ipc_fuzzing_tests.cc (right):

http://codereview.chromium.org/20015/diff/1061/1071#newcode99
Line 99: // We don't actually use the messages defined in this fiel, but we do
this
uh?

http://codereview.chromium.org/20015/diff/1061/1101
File chrome/common/ipc_logging.cc (right):

http://codereview.chromium.org/20015/diff/1061/1101#newcode269
Line 269: }
}  // namespace IPC