Chromium Code Reviews
Help | Chromium Project | Sign in
(205)

Issue 9315006: Adding support for sending/receiving proto bufs to dbus library. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years ago by rharrison
Modified:
2 years, 11 months ago
Reviewers:
Steve Block, satorux1
CC:
chromium-reviews
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Made changes requested by satorux #

Total comments: 10

Patch Set 3 : Resolved most of outstanding issues #

Total comments: 9

Patch Set 4 : Fixed protobuf build issues #

Patch Set 5 : Added a missing dependency to gyp #

Patch Set 6 : Added in the DEPS file as suggested #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+102 lines, -0 lines) Patch
A dbus/DEPS View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M dbus/dbus.gyp View 1 2 3 4 3 chunks +12 lines, -0 lines 0 comments Download
M dbus/message.h View 1 2 3 chunks +25 lines, -0 lines 0 comments Download
M dbus/message.cc View 1 2 3 chunks +31 lines, -0 lines 0 comments Download
M dbus/message_unittest.cc View 1 2 3 2 chunks +17 lines, -0 lines 0 comments Download
A dbus/test_proto.proto View 1 2 3 1 chunk +13 lines, -0 lines 2 comments Download
Commit:

Messages

Total messages: 25 (0 generated)
rharrison
3 years ago (2012-01-31 20:17:52 UTC) #1
satorux1
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? ...
3 years ago (2012-01-31 20:27:49 UTC) #2
rharrison
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: ...
3 years ago (2012-01-31 21:05:16 UTC) #3
satorux1
Could you write unit tests for the new functions? Other functions have unit tests in ...
3 years ago (2012-01-31 21:42:31 UTC) #4
rharrison
I have resolved most of the outstanding issues. I have one bug I would like ...
3 years ago (2012-02-01 19:03:20 UTC) #5
satorux1
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 ...
3 years ago (2012-02-01 19:14:57 UTC) #6
rharrison
Resolved my linking issue by adding in the explicit statement to optimize for Message Lite. ...
3 years ago (2012-02-01 21:33:21 UTC) #7
satorux1
LGTM Re protobuf build issue, I don't know who would be the right person to ...
3 years ago (2012-02-02 06:40:29 UTC) #8
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/8001
3 years ago (2012-02-02 14:47:28 UTC) #9
I haz the power (commit-bot)
Try job failure for 9315006-8001 (retry) on linux_rel for step "compile" (clobber build). It's a ...
3 years ago (2012-02-02 15:09:53 UTC) #10
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/10001
3 years ago (2012-02-06 20:22:45 UTC) #11
I haz the power (commit-bot)
Try job failure for 9315006-10001 (retry) on linux_rel for step "check_deps". It's a second try, ...
3 years ago (2012-02-06 21:29:11 UTC) #12
satorux1
You should probably add dbus/DEPS and put something like include_rules = [ "+base", "+third_party/protobuf", } ...
3 years ago (2012-02-06 22:34:32 UTC) #13
rharrison
Looks like adding the DEPS as suggested fixed the issues. Thanks for the help, I ...
3 years ago (2012-02-07 15:31:35 UTC) #14
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001
3 years ago (2012-02-07 15:55:01 UTC) #15
I haz the power (commit-bot)
Try job failure for 9315006-19001 (previous was lost) (retry) on mac_rel for step "ui_tests" (clobber ...
3 years ago (2012-02-07 21:07:57 UTC) #16
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001
3 years ago (2012-02-07 21:13:51 UTC) #17
I haz the power (commit-bot)
Try job failure for 9315006-19001 (retry) on mac_rel for steps "base_unittests, net_unittests". It's a second ...
3 years ago (2012-02-07 23:48:40 UTC) #18
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001
3 years ago (2012-02-08 15:17:27 UTC) #19
I haz the power (commit-bot)
Try job failure for 9315006-19001 (retry) on win_rel for step "compile" (clobber build). It's a ...
3 years ago (2012-02-08 15:47:47 UTC) #20
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/9315006/19001
3 years ago (2012-02-09 14:44:16 UTC) #21
I haz the power (commit-bot)
Change committed as 121248
3 years ago (2012-02-09 18:14:09 UTC) #22
Steve Block
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 ...
2 years, 11 months ago (2012-03-22 13:31:02 UTC) #23
rharrison
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 ...
2 years, 11 months ago (2012-03-22 14:24:52 UTC) #24
Steve Block
2 years, 11 months ago (2012-03-22 15:08:57 UTC) #25
Sounds great, thanks!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87e6a26