Add support for sending protobuf messages over IPC.

Add support for sending protobuf messages over IPC.

BUG=462584
Committed: https://crrev.com/07840d895caf8b511ffd2209d3ce2ffb1ae4efe7
Cr-Commit-Position: refs/heads/master@{#321237}

[Tom Sepez, 2015-03-13 22:28:10]

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

[grt (UTC plus 2), 2015-03-14 11:38:06]

On 2015/03/13 22:28:10, Tom Sepez wrote:
> mailto:tsepez@chromium.org changed reviewers:
> + mailto:jam@chromium.org, mailto:tsepez@chromium.org

Just to be clear: this is a proof of concept at this point. I haven't
sanity-checked the proto-to-ipc code to make sure it covers all corner cases,
and (clearly) it's missing tests.

That said, thanks for taking a look.

[grt (UTC plus 2), 2015-03-15 23:10:19]

IPC only

[grt (UTC plus 2), 2015-03-15 23:11:45]

I've moved the use of this new IPC stuff into
https://codereview.chromium.org/999003003/ so that the two can be reviewed
independently.

[Tom Sepez, 2015-03-16 16:45:44]

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/protobuf_message_read_macros.h (right):

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/protobuf_message_read_macros.h:37:
p->clear_##name##();                                          \
I'm not sure you can token splice onto (), p->clear_##name() should do the
trick.

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/protobuf_message_read_macros.h:38: else if
(!ReadParamF(m, iter, p, &param_type::set_##name##))    \
ditto

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/protobuf_message_read_macros.h:48: else if
(!ReadParam(m, iter, p->mutable_##name##()))            \
ditto

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/protobuf_message_write_macros.h (right):

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/protobuf_message_write_macros.h:23: if
(p.has_##name##()) {                                             \
ditto

[grt (UTC plus 2), 2015-03-16 17:48:13]

Comments addressed, thanks. I've also added a unittest. Please take another
look.

I'll point out that none of this is specific to Chrome or Safe Browsing, and
could potentially be in src/ipc/ itself. Given that the only consumer of
protobuf-over-IPC at the moment is code destined to live in
chrome/common/safe_browsing, I figure it's good for the code to live there and
to move outward only when other needs arise. Let me know if you prefer a
different location for this.

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
File chrome/common/safe_browsing/protobuf_message_read_macros.h (right):

https://codereview.chromium.org/1004323003/diff/40001/chrome/common/safe_brow...
chrome/common/safe_browsing/protobuf_message_read_macros.h:37:
p->clear_##name##();                                          \
On 2015/03/16 16:45:43, Tom Sepez wrote:
> I'm not sure you can token splice onto (), p->clear_##name() should do the
> trick.

Done here and elsewhere.

[Tom Sepez, 2015-03-16 18:01:30]

On 2015/03/16 17:48:13, grt wrote:
> Comments addressed, thanks. I've also added a unittest. Please take another
> look.
> 
> I'll point out that none of this is specific to Chrome or Safe Browsing, and
> could potentially be in src/ipc/ itself. Given that the only consumer of
> protobuf-over-IPC at the moment is code destined to live in
> chrome/common/safe_browsing, I figure it's good for the code to live there and
> to move outward only when other needs arise. Let me know if you prefer a
> different location for this.
> 
> 
I think your location is fine.  Otherwise, IPC would have to depend on
protobufs.

[Tom Sepez, 2015-03-16 18:05:10]

LGTM once you fix the compilation noise.

[grt (UTC plus 2), 2015-03-16 19:14:24]

compile

[grt (UTC plus 2), 2015-03-16 19:18:04]

Patchset #5 (id:80001) has been deleted

[grt (UTC plus 2), 2015-03-16 20:24:02]

I've fixed the compile failures (adding support for the GN build in the
process). Please take another look. Let me know if I should wait for a review
from anyone else before landing. Thanks.

[Tom Sepez, 2015-03-16 20:41:16]

Depends on how big a rush you are in.  I'd like jam@ to take a peek, but I think
he's out for a little bit.  Otherwise LGTM.

[grt (UTC plus 2), 2015-03-17 13:08:43]

On 2015/03/16 20:41:16, Tom Sepez wrote:
> Depends on how big a rush you are in.  I'd like jam@ to take a peek, but I
think
> he's out for a little bit.  Otherwise LGTM.

It would be nice to get it in this week so I can continue to make progress on
its raison d'etre. Unless I hear from you otherwise, I'll land this as-is later
today. Thanks.

[Tom Sepez, 2015-03-17 20:26:31]

On 2015/03/17 13:08:43, grt wrote:
> On 2015/03/16 20:41:16, Tom Sepez wrote:
> > Depends on how big a rush you are in.  I'd like jam@ to take a peek, but I
> think
> > he's out for a little bit.  Otherwise LGTM.
> 
> It would be nice to get it in this week so I can continue to make progress on
> its raison d'etre. Unless I hear from you otherwise, I'll land this as-is
later
> today. Thanks.

Ok. Go ahead.

[grt (UTC plus 2), 2015-03-17 20:27:01]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-03-17 20:27:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004323003/120001

[grt (UTC plus 2), 2015-03-17 20:45:27]

grt@chromium.org changed reviewers:
+ mattm@chromium.org, sky@chromium.org

[grt (UTC plus 2), 2015-03-17 20:46:14]

grt@chromium.org changed reviewers:
+ scottmg@chromium.org

[grt (UTC plus 2), 2015-03-17 20:47:46]

In search of OWNERS reviews as follows:
mattm: chrome/*/safe_browsing/*
sky: chrome/common (except safe_browsing) and chrome/test
scottmg: +third_party/protobuf in safe_browsing DEPS

Thanks

[commit-bot: I haz the power, 2015-03-17 20:55:49]

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

[commit-bot: I haz the power, 2015-03-17 20:55:50]

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...)

[mattm, 2015-03-17 22:15:50]

https://codereview.chromium.org/1004323003/diff/120001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1004323003/diff/120001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2686: # GN version: TODO(grt)
still todo?

https://codereview.chromium.org/1004323003/diff/120001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1004323003/diff/120001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1468:
"../common/safe_browsing/ipc_protobuf_message_unittest.cc",
Hm, weird. I don't think these should be disabled on chromeos. I've created
https://codereview.chromium.org/1012263002/ for the unittest.

[scottmg, 2015-03-17 23:04:42]

deps lgtm

[grt (UTC plus 2), 2015-03-18 16:48:01]

mattm: PTAL

https://codereview.chromium.org/1004323003/diff/120001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1004323003/diff/120001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2686: # GN version: TODO(grt)
On 2015/03/17 22:15:50, mattm wrote:
> still todo?

Done.

https://codereview.chromium.org/1004323003/diff/120001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1004323003/diff/120001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1468:
"../common/safe_browsing/ipc_protobuf_message_unittest.cc",
On 2015/03/17 22:15:50, mattm wrote:
> Hm, weird. I don't think these should be disabled on chromeos. I've created
> https://codereview.chromium.org/1012263002/ for the unittest.

I wondered about that. I've removed the extra stuff here so that the new tests
should run on chromeos as well.

[mattm, 2015-03-18 19:22:27]

lgtm (IPC stuff I'm just rubber stamping)

[grt (UTC plus 2), 2015-03-18 19:24:13]

sky: ping for OWNERS review for chrome/common (except safe_browsing) and
chrome/test

[sky, 2015-03-18 21:01:13]

LGTM with the following change

https://codereview.chromium.org/1004323003/diff/140001/chrome/common/common_m...
File chrome/common/common_message_generator.cc (right):

https://codereview.chromium.org/1004323003/diff/140001/chrome/common/common_m...
chrome/common/common_message_generator.cc:16: #include
"chrome/common/safe_browsing/ipc_protobuf_message_null_macros.h"
Why do you have this include twice?

[grt (UTC plus 2), 2015-03-18 21:14:35]

Thanks for taking a look.

https://codereview.chromium.org/1004323003/diff/140001/chrome/common/common_m...
File chrome/common/common_message_generator.cc (right):

https://codereview.chromium.org/1004323003/diff/140001/chrome/common/common_m...
chrome/common/common_message_generator.cc:16: #include
"chrome/common/safe_browsing/ipc_protobuf_message_null_macros.h"
On 2015/03/18 21:01:13, sky wrote:
> Why do you have this include twice?

ipc_protobuf_message_macros.h will eventually be included transitively by
common_message_generator.h (this is coming in a subsequent change that depends
on this one). since there are no protobuf-like analogs to constructor_macros.h
and destructor_macros.h, the various IPC_PROTOBUF_MESSAGE_TRAITS_* macros need
to be nulled out here to avoid multiple definition errors.

[grt (UTC plus 2), 2015-03-18 21:46:10]

On 2015/03/18 21:14:35, grt wrote:
> Thanks for taking a look.
> 
>
https://codereview.chromium.org/1004323003/diff/140001/chrome/common/common_m...
> File chrome/common/common_message_generator.cc (right):
> 
>
https://codereview.chromium.org/1004323003/diff/140001/chrome/common/common_m...
> chrome/common/common_message_generator.cc:16: #include
> "chrome/common/safe_browsing/ipc_protobuf_message_null_macros.h"
> On 2015/03/18 21:01:13, sky wrote:
> > Why do you have this include twice?
> 
> ipc_protobuf_message_macros.h will eventually be included transitively by
> common_message_generator.h (this is coming in a subsequent change that depends
> on this one). since there are no protobuf-like analogs to constructor_macros.h
> and destructor_macros.h, the various IPC_PROTOBUF_MESSAGE_TRAITS_* macros need
> to be nulled out here to avoid multiple definition errors.

sky: Please hit the CQ box if you're okay with this. Otherwise, I'm happy to
follow any suggestion you may have. Thanks.

[sky, 2015-03-18 23:04:28]

Ok, LGTM

[sky, 2015-03-18 23:04:34]

The CQ bit was checked by sky@chromium.org

[sky, 2015-03-18 23:04:34]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1004323003/#ps140001
(title: "remove todo and enable for cros")

[commit-bot: I haz the power, 2015-03-18 23:05:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004323003/140001

[commit-bot: I haz the power, 2015-03-18 23:57:26]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-03-18 23:58:01]

Patchset 7 (id:??) landed as
https://crrev.com/07840d895caf8b511ffd2209d3ce2ffb1ae4efe7
Cr-Commit-Position: refs/heads/master@{#321237}