Add simple trace logging of received IPC messages

Add simple trace logging of received IPC messages

Adds an entry to chrome://tracing for processing IPC messages that includes the IPC message type ID (which can be matched to a name with the ipclist tool), and in DEBUG builds (or other builds where IPC_MESSAGE_LOG_ENABLED has been set), the message name.

Also adds shell_messages.h to the message generator headers (per the rules in ipc_message_macros.h) to fix broken IPC message logging.

BUG=79942
TEST=

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

[Rick Byers, 2012-02-13 23:09:58]

John,
Can you please review this tiny IPC tracing tweak for me?

Thanks,
   Rick

[jbates, 2012-02-13 23:26:25]

https://chromiumcodereview.appspot.com/9389020/diff/1/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://chromiumcodereview.appspot.com/9389020/diff/1/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:242: std::string name = "N/A (IPC Logging disabled)";
To avoid the overhead of this string, I would move it inside the
IPC_MESSAGE_LOG_ENABLED ifdef, and have two forms of TRACE_EVENTs: one with
"name" in the #ifdef and one without in the #else. (std::string arguments are
expensive with tracing since they require an allocation and copy for every event
even though it's the same string every time in this case.)

[Rick Byers, 2012-02-13 23:43:53]

On 2012/02/13 23:26:25, jbates wrote:
> https://chromiumcodereview.appspot.com/9389020/diff/1/ipc/ipc_channel_proxy.cc
> File ipc/ipc_channel_proxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9389020/diff/1/ipc/ipc_channel_proxy.c...
> ipc/ipc_channel_proxy.cc:242: std::string name = "N/A (IPC Logging disabled)";
> To avoid the overhead of this string, I would move it inside the
> IPC_MESSAGE_LOG_ENABLED ifdef, and have two forms of TRACE_EVENTs: one with
> "name" in the #ifdef and one without in the #else. (std::string arguments are
> expensive with tracing since they require an allocation and copy for every
event
> even though it's the same string every time in this case.)

Thanks, done - I just show type OR name depending what's available.

[jbates, 2012-02-13 23:53:43]

LGTM

[jam, 2012-02-14 01:37:38]

http://codereview.chromium.org/9389020/diff/6/chrome/common/all_messages.h
File chrome/common/all_messages.h (right):

http://codereview.chromium.org/9389020/diff/6/chrome/common/all_messages.h#ne...
chrome/common/all_messages.h:18: #include "content/shell/shell_messages.h"
i think this is supposed to be all files used in chrome. content_shell isn't
used in it, why are you adding it here?

[Rick Byers, 2012-02-14 01:54:56]

On 2012/02/14 01:37:38, John Abd-El-Malek wrote:
> http://codereview.chromium.org/9389020/diff/6/chrome/common/all_messages.h
> File chrome/common/all_messages.h (right):
> 
>
http://codereview.chromium.org/9389020/diff/6/chrome/common/all_messages.h#ne...
> chrome/common/all_messages.h:18: #include "content/shell/shell_messages.h"
> i think this is supposed to be all files used in chrome. content_shell isn't
> used in it, why are you adding it here?

Ah - that makes sense.  The comments in the headers imply it's always needed,
and the ipclist tool fails without this - complaining of a gap in
IPCMessageStart (if I recall).  I assumed the runtime IPC logging code might
have a problem with it being missing too, but I suppose that's probably not the
case.

I can look at alternate ways of fixing ipclist (eg. maybe we need another marker
in addition to LastIPCMsgStart to separate out the chrome ones), but all this
IPC code is new to me so I'd appreciate any advice you have.  Or I could just
ignore ipclist - I was using it to map IPC #s to names on Release builds that
don't have logging, but quickly switched to just enabling IPC logging on my
Release builds.

[Rick Byers, 2012-02-14 02:27:11]

Ah, I see ipclist already has an 'excemptions' pattern for cases like this where
a message class is used only outside of chrome itself.  I've instead used that
and updated the code to handle the case where the last class in the list
requires an exemption.  I've also updated comments to make this more clear in
the future.

[jam, 2012-02-14 02:32:06]

lgtm

[commit-bot: I haz the power, 2012-02-14 14:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/9389020/1007

[commit-bot: I haz the power, 2012-02-14 15:51:49]

Change committed as 121889