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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 2 months ago by rharrison
Modified:
2 years, 1 month ago
Reviewers:
Steve Block, satorux1
CC:
chromium-reviews_chromium.org
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) Lint Patch
A dbus/DEPS View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M dbus/dbus.gyp View 1 2 3 4 3 chunks +12 lines, -0 lines 0 comments ? errors Download
M dbus/message.h View 1 2 3 chunks +25 lines, -0 lines 0 comments ? errors Download
M dbus/message.cc View 1 2 3 chunks +31 lines, -0 lines 0 comments ? errors Download
M dbus/message_unittest.cc View 1 2 3 2 chunks +17 lines, -0 lines 0 comments ? errors Download
A dbus/test_proto.proto View 1 2 3 1 chunk +13 lines, -0 lines 2 comments ? errors Download
Commit:

Messages

Total messages: 25
rharrison
2 years, 2 months ago #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? ...
2 years, 2 months ago #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: ...
2 years, 2 months ago #3
satorux1
Could you write unit tests for the new functions? Other functions have unit tests in ...
2 years, 2 months ago #4
rharrison
I have resolved most of the outstanding issues. I have one bug I would like ...
2 years, 2 months ago #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 ...
2 years, 2 months ago #6
rharrison
Resolved my linking issue by adding in the explicit statement to optimize for Message Lite. ...
2 years, 2 months ago #7
satorux1
LGTM Re protobuf build issue, I don't know who would be the right person to ...
2 years, 2 months ago #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
2 years, 2 months ago #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 ...
2 years, 2 months ago #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
2 years, 2 months ago #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, ...
2 years, 2 months ago #12
satorux1
You should probably add dbus/DEPS and put something like include_rules = [ "+base", "+third_party/protobuf", } ...
2 years, 2 months ago #13
rharrison
Looks like adding the DEPS as suggested fixed the issues. Thanks for the help, I ...
2 years, 2 months ago #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
2 years, 2 months ago #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 ...
2 years, 2 months ago #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
2 years, 2 months ago #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 ...
2 years, 2 months ago #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
2 years, 2 months ago #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 ...
2 years, 2 months ago #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
2 years, 2 months ago #21
I haz the power (commit-bot)
Change committed as 121248
2 years, 2 months ago #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, 1 month ago #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, 1 month ago #24
Steve Block
2 years, 1 month ago #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 1280:2d3e6564b7b6