Adding support for sending/receiving proto bufs to dbus library.

Adding support for sending/receiving proto bufs to dbus library.

This CL add in support to dbus wrapping classes to send and receive protocol
buffers. They take in a generic MessageLite, which all protocol buffers inherit
from. It will then serialize the buffer and send it out as an array of bytes. On
receiving the array of bytes will be parsed back into a protocol buffer. The
operations for doing this are very canned and these methods are designed to
allow devs to skip writing boilerplate. The methods are just wrappers around the
appopriate byte array methods.



BUG=chromium:112127
TEST=Ran new unittests

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

[satorux1, 2012-01-31 20:27:49]

http://codereview.chromium.org/9315006/diff/1/dbus/message.cc
File dbus/message.cc (right):

http://codereview.chromium.org/9315006/diff/1/dbus/message.cc#newcode601
dbus/message.cc:601: char* serialized_proto =
reinterpret_cast<char*>(malloc(buf_size));
Cannot we just use std::string? We rarely use malloc/free in Chrome's code.

http://codereview.chromium.org/9315006/diff/1/dbus/message.h
File dbus/message.h (right):

http://codereview.chromium.org/9315006/diff/1/dbus/message.h#newcode304
dbus/message.h:304: bool AppendProtocolBuffer(google::protobuf::MessageLite*
protobuf);
Can you make it a const reference?

[rharrison, 2012-01-31 21:05:16]

http://codereview.chromium.org/9315006/diff/1/dbus/message.cc
File dbus/message.cc (right):

http://codereview.chromium.org/9315006/diff/1/dbus/message.cc#newcode601
dbus/message.cc:601: char* serialized_proto =
reinterpret_cast<char*>(malloc(buf_size));
On 2012/01/31 20:27:49, satorux1 wrote:
> Cannot we just use std::string? We rarely use malloc/free in Chrome's code.

Done.

http://codereview.chromium.org/9315006/diff/1/dbus/message.h
File dbus/message.h (right):

http://codereview.chromium.org/9315006/diff/1/dbus/message.h#newcode304
dbus/message.h:304: bool AppendProtocolBuffer(google::protobuf::MessageLite*
protobuf);
On 2012/01/31 20:27:49, satorux1 wrote:
> Can you make it a const reference?

Done.

[satorux1, 2012-01-31 21:42:31]

Could you write unit tests for the new functions? Other functions have unit
tests in message_unittest.cc. Would be great if you can write unit tests without
code generation.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.cc
File dbus/message.cc (right):

http://codereview.chromium.org/9315006/diff/4001/dbus/message.cc#newcode819
dbus/message.cc:819: size_t buf_size;
Please initialize them.

char* serialized_buf = NULL;
size_t buf_size = 0;

PopArrayOfBytes() should do the right thing, but it's good to be defensive.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h
File dbus/message.h (right):

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode14
dbus/message.h:14: #include
"third_party/protobuf/src/google/protobuf/message_lite.h"
I think you can remove the include from .h file, by forward-declaring the class:

namespace google {
namespace protobuf {
class MessageLite;
}
}

Then have the #include line in .cc file.

This will speed up the build time for clients of dbus/message.h which don't use
protobuf

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode298
dbus/message.h:298: // Appends the protocol buffer. The buffer is serialized
into an array of
Appends the protocol buffer as an array of bytes.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode304
dbus/message.h:304: bool AppendProtocolBuffer(const
google::protobuf::MessageLite& protobuf);
AppendProtoAsArrayOfBytes() would make it clear that the protocol buffer message
is appended as an array of bytes.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode410
dbus/message.h:410: bool PopProtocolBuffer(google::protobuf::MessageLite*
protobuf);
PopArrayOfBytesAsProto() ?

[rharrison, 2012-02-01 19:03:20]

I have resolved most of the outstanding issues. I have one bug I would like some
assistance with.

For the requested unit test I have added in a test protobuf to use in it. When
building dbus_unittests I am getting linking errors that look like the build
system does not know where the protocol buffer library is. I am not sure why
this is occurring and from my inspection of the gyp rules I have the same thing
as other areas that build protobufs. Could you please take a look and see if
there is something that I am missing?

http://codereview.chromium.org/9315006/diff/4001/dbus/message.cc
File dbus/message.cc (right):

http://codereview.chromium.org/9315006/diff/4001/dbus/message.cc#newcode819
dbus/message.cc:819: size_t buf_size;
On 2012/01/31 21:42:31, satorux1 wrote:
> Please initialize them.
> 
> char* serialized_buf = NULL;
> size_t buf_size = 0;
> 
> PopArrayOfBytes() should do the right thing, but it's good to be defensive.
> 

Done.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h
File dbus/message.h (right):

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode14
dbus/message.h:14: #include
"third_party/protobuf/src/google/protobuf/message_lite.h"
On 2012/01/31 21:42:31, satorux1 wrote:
> I think you can remove the include from .h file, by forward-declaring the
class:
> 
> namespace google {
> namespace protobuf {
> class MessageLite;
> }
> }
> 
> Then have the #include line in .cc file.
> 
> This will speed up the build time for clients of dbus/message.h which don't
use
> protobuf

Done.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode298
dbus/message.h:298: // Appends the protocol buffer. The buffer is serialized
into an array of
On 2012/01/31 21:42:31, satorux1 wrote:
> Appends the protocol buffer as an array of bytes.

Done.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode304
dbus/message.h:304: bool AppendProtocolBuffer(const
google::protobuf::MessageLite& protobuf);
On 2012/01/31 21:42:31, satorux1 wrote:
> AppendProtoAsArrayOfBytes() would make it clear that the protocol buffer
message
> is appended as an array of bytes.

Done.

http://codereview.chromium.org/9315006/diff/4001/dbus/message.h#newcode410
dbus/message.h:410: bool PopProtocolBuffer(google::protobuf::MessageLite*
protobuf);
On 2012/01/31 21:42:31, satorux1 wrote:
> PopArrayOfBytesAsProto() ?

Done.

[satorux1, 2012-02-01 19:14:57]

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp
File dbus/dbus.gyp (right):

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp#newcode36
dbus/dbus.gyp:36: 'sources': [ 'test_proto.proto' ],
I don't see it in this patch. Did you forget to include this file?

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp#newcode49
dbus/dbus.gyp:49: '../testing/gmock.gyp:gmock',
Please add a dependency to the protobuf libe here.

'../third_party/protobuf/protobuf.gyp:protobuf_lite'

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp#newcode71
dbus/dbus.gyp:71: '../testing/gtest.gyp:gtest',
You probably need to add a dependency to the protobuf libe here as well.

'../third_party/protobuf/protobuf.gyp:protobuf_lite'

http://codereview.chromium.org/9315006/diff/2003/dbus/message_unittest.cc
File dbus/message_unittest.cc (right):

http://codereview.chromium.org/9315006/diff/2003/dbus/message_unittest.cc#new...
dbus/message_unittest.cc:234: send_message.set_text("testing");
To make test a little more interesting, could you add one more field, like
set_int(123) ?

http://codereview.chromium.org/9315006/diff/2003/dbus/message_unittest.cc#new...
dbus/message_unittest.cc:240: EXPECT_EQ(receive_message.text(),
send_message.text());
Rather than comparing a field, it'be better to compare the whole message, like

EXPECT_EQ(receive_message.DebugString(), send_message.DebugString())

[rharrison, 2012-02-01 21:33:21]

Resolved my linking issue by adding in the explicit statement to optimize for
Message Lite. This is correct to my understanding, since we are not using the
full proto buf library.

This suggests that there are bugs in the main Chrome build code, since I have
built heavy proto bufs in it unintentionally without there being any linking
issues, so code referenced in chrome_browser.gypi is getting linked against the
non-optimized version.

satorux, are you the correct person to look into the issue with heavy v. lite
protobufs or is there someone more familiar with how we build them? I would
prefer to not take it on as a bug since it is a substantial deviation from what
I am trying to accomplish, but I think it is something that warrants
investigation.

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp
File dbus/dbus.gyp (right):

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp#newcode36
dbus/dbus.gyp:36: 'sources': [ 'test_proto.proto' ],
On 2012/02/01 19:14:57, satorux1 wrote:
> I don't see it in this patch. Did you forget to include this file?
Yes, yes I did

http://codereview.chromium.org/9315006/diff/2003/dbus/dbus.gyp#newcode71
dbus/dbus.gyp:71: '../testing/gtest.gyp:gtest',
On 2012/02/01 19:14:57, satorux1 wrote:
> You probably need to add a dependency to the protobuf libe here as well.
> 
> '../third_party/protobuf/protobuf.gyp:protobuf_lite'

Done.

http://codereview.chromium.org/9315006/diff/2003/dbus/message_unittest.cc
File dbus/message_unittest.cc (right):

http://codereview.chromium.org/9315006/diff/2003/dbus/message_unittest.cc#new...
dbus/message_unittest.cc:234: send_message.set_text("testing");
On 2012/02/01 19:14:57, satorux1 wrote:
> To make test a little more interesting, could you add one more field, like
> set_int(123) ?

Done.

http://codereview.chromium.org/9315006/diff/2003/dbus/message_unittest.cc#new...
dbus/message_unittest.cc:240: EXPECT_EQ(receive_message.text(),
send_message.text());
On 2012/02/01 19:14:57, satorux1 wrote:
> Rather than comparing a field, it'be better to compare the whole message, like
> 
> EXPECT_EQ(receive_message.DebugString(), send_message.DebugString())

This is not available in MessageLite objects.

[satorux1, 2012-02-02 06:40:29]

LGTM

Re protobuf build issue, I don't know who would be the right person to talk to.
You could file a bug on crbug.com and ask around on #chromium IRC channel.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/8001

[commit-bot: I haz the power, 2012-02-02 15:09:53]

Try job failure for 9315006-8001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-06 20:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/10001

[commit-bot: I haz the power, 2012-02-06 21:29:11]

Try job failure for 9315006-10001 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[satorux1, 2012-02-06 22:34:32]

You should probably add dbus/DEPS and put something like

include_rules = [
  "+base",
  "+third_party/protobuf",
}

You can run checkdeps.py locally:
 python tools/checkdeps/checkdeps.py

[rharrison, 2012-02-07 15:31:35]

Looks like adding the DEPS as suggested fixed the issues. Thanks for the help, I
hadn't worked up with the DEPS system before. I will try landing this yet again
once maruel brings the commit queue back online.

[commit-bot: I haz the power, 2012-02-07 15:55:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001

[commit-bot: I haz the power, 2012-02-07 21:07:57]

Try job failure for 9315006-19001 (previous was lost) (retry) on mac_rel for
step "ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-07 21:13:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001

[commit-bot: I haz the power, 2012-02-07 23:48:40]

Try job failure for 9315006-19001 (retry) on mac_rel for steps "base_unittests,
net_unittests".
It's a second try, previously, steps "base_unittests, net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-08 15:17:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001

[commit-bot: I haz the power, 2012-02-08 15:47:47]

Try job failure for 9315006-19001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-09 14:44:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001

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

Change committed as 121248

[Steve Block, 2012-03-22 13:31:02]

http://codereview.chromium.org/9315006/diff/19001/dbus/test_proto.proto
File dbus/test_proto.proto (right):

http://codereview.chromium.org/9315006/diff/19001/dbus/test_proto.proto#newcode1
dbus/test_proto.proto:1: // Copyright (c) 2012 The Chromium OS Authors. All
rights reserved.
Is this intentionally copyright Chromium OS, rather than Chromium? All
third-party code in Chromium should be in directories named 'third_party'. See
http://www.chromium.org/developers/adding-3rd-party-libraries.

[rharrison, 2012-03-22 14:24:52]

http://codereview.chromium.org/9315006/diff/19001/dbus/test_proto.proto
File dbus/test_proto.proto (right):

http://codereview.chromium.org/9315006/diff/19001/dbus/test_proto.proto#newcode1
dbus/test_proto.proto:1: // Copyright (c) 2012 The Chromium OS Authors. All
rights reserved.
On 2012/03/22 13:31:03, Steve Block wrote:
> Is this intentionally copyright Chromium OS, rather than Chromium? All
> third-party code in Chromium should be in directories named 'third_party'. See
> http://www.chromium.org/developers/adding-3rd-party-libraries.

I think this was an oversight on my part since I likely used one of the
protobufs from ChromeOS as a template for this. Since this already landed I can
write a small follow on CL to correct this, would that be sufficient?

[Steve Block, 2012-03-22 15:08:57]

Sounds great, thanks!