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

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, 2 months ago by rharrison
Modified:
3 years 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

Messages

Total messages: 25 (0 generated)
rharrison
3 years, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month 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, 1 month ago (2012-02-09 14:44:16 UTC) #21
I haz the power (commit-bot)
Change committed as 121248
3 years, 1 month 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 ...
3 years 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 ...
3 years ago (2012-03-22 14:24:52 UTC) #24
Steve Block
3 years 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 cf4c24d