chrome: dbus: support asynchronous method replies

chrome: dbus: support asynchronous method replies

BUG=chromium-os:23241
TEST=Unit tests and manual testing on device.

Change-Id: I4d665897687030f4ab2379e4f6ddb9b3ebe02af4


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

[satorux1, 2011-11-22 06:20:02]

Looks nice overall. Minor comments only:

http://codereview.chromium.org/8637002/diff/9/chrome/browser/chromeos/dbus/pr...
File chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h (right):

http://codereview.chromium.org/8637002/diff/9/chrome/browser/chromeos/dbus/pr...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h:106:
dbus::ExportedObject::SendResponseCallback send_response_cb);
cb -> callback, as we generally eschew abbreviation like this. 

If we rename the type to ResponseSender, the parameter name should be
response_sender.

http://codereview.chromium.org/8637002/diff/9/chrome/browser/chromeos/dbus/pr...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h:113:
dbus::ExportedObject::SendResponseCallback send_response_cb);
ditto

http://codereview.chromium.org/8637002/diff/9/dbus/exported_object.h
File dbus/exported_object.h (right):

http://codereview.chromium.org/8637002/diff/9/dbus/exported_object.h#newcode42
dbus/exported_object.h:42: // response message.
Please describe that the export method should call this with NULL, if something
went wrong.

http://codereview.chromium.org/8637002/diff/9/dbus/exported_object.h#newcode43
dbus/exported_object.h:43: typedef base::Callback<void (Response*)>
SendResponseCallback;
I suggested SendResponseCallback, but I'm now thinking that ResponseSender would
be a better name. We don't have to have Callback suffix all the time.

[Vince Laviano, 2011-11-22 22:10:17]

http://codereview.chromium.org/8637002/diff/9/chrome/browser/chromeos/dbus/pr...
File chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h (right):

http://codereview.chromium.org/8637002/diff/9/chrome/browser/chromeos/dbus/pr...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h:106:
dbus::ExportedObject::SendResponseCallback send_response_cb);
On 2011/11/22 06:20:05, satorux1 wrote:
> cb -> callback, as we generally eschew abbreviation like this. 
> 
> If we rename the type to ResponseSender, the parameter name should be
> response_sender.

Done.

http://codereview.chromium.org/8637002/diff/9/chrome/browser/chromeos/dbus/pr...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h:113:
dbus::ExportedObject::SendResponseCallback send_response_cb);
On 2011/11/22 06:20:05, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/8637002/diff/9/dbus/exported_object.h
File dbus/exported_object.h (right):

http://codereview.chromium.org/8637002/diff/9/dbus/exported_object.h#newcode42
dbus/exported_object.h:42: // response message.
On 2011/11/22 06:20:05, satorux1 wrote:
> Please describe that the export method should call this with NULL, if
something
> went wrong.

Done.

http://codereview.chromium.org/8637002/diff/9/dbus/exported_object.h#newcode43
dbus/exported_object.h:43: typedef base::Callback<void (Response*)>
SendResponseCallback;
On 2011/11/22 06:20:05, satorux1 wrote:
> I suggested SendResponseCallback, but I'm now thinking that ResponseSender
would
> be a better name. We don't have to have Callback suffix all the time.

Done.

[satorux1, 2011-11-22 22:38:17]

LGTM with style nits

http://codereview.chromium.org/8637002/diff/5001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/8637002/diff/5001/dbus/bus.h#newcode104
dbus/bus.h:104: //       dbus::ExportedObject::ResponseSender response_sender) {
not aligned vertically?

// void Echo(dbus::MethodCall* method_call,
//           dbus::ExportedObject::ResponseSender response_sender) {

http://codereview.chromium.org/8637002/diff/5001/dbus/bus.h#newcode109
dbus/bus.h:109: //     // or store the send_response_cb and send a response
later to implement
send_response_cb  -> response_sender.

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc#newcode167
dbus/test_service.cc:167: dbus::ExportedObject::ResponseSender response_sender)
{
vertical align.

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc#newcode182
dbus/test_service.cc:182: dbus::ExportedObject::ResponseSender response_sender)
{
ditto.

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc#newcode188
dbus/test_service.cc:188: dbus::ExportedObject::ResponseSender response_sender)
{
ditto

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.h
File dbus/test_service.h (right):

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.h#newcode93
dbus/test_service.h:93: dbus::ExportedObject::ResponseSender response_sender);
please align parameters vertically.

void Echo(MethodCall* method_call,
          dbus::ExportedObject::ResponseSender response_sender);

[Vince Laviano, 2011-11-22 23:16:22]

http://codereview.chromium.org/8637002/diff/5001/dbus/bus.h
File dbus/bus.h (right):

http://codereview.chromium.org/8637002/diff/5001/dbus/bus.h#newcode104
dbus/bus.h:104: //       dbus::ExportedObject::ResponseSender response_sender) {
On 2011/11/22 22:38:17, satorux1 wrote:
> not aligned vertically?
> 
> // void Echo(dbus::MethodCall* method_call,
> //           dbus::ExportedObject::ResponseSender response_sender) {

Done. With the previous names, SenderResponseCallback and sender_response_cb,
vertical alignment caused the line to exceed 80 chars, hence the 4 space indent
instead.

http://codereview.chromium.org/8637002/diff/5001/dbus/bus.h#newcode109
dbus/bus.h:109: //     // or store the send_response_cb and send a response
later to implement
On 2011/11/22 22:38:17, satorux1 wrote:
> send_response_cb  -> response_sender.

Done.

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc#newcode167
dbus/test_service.cc:167: dbus::ExportedObject::ResponseSender response_sender)
{
On 2011/11/22 22:38:17, satorux1 wrote:
> vertical align.

Done.

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc#newcode182
dbus/test_service.cc:182: dbus::ExportedObject::ResponseSender response_sender)
{
On 2011/11/22 22:38:17, satorux1 wrote:
> ditto.

This one is 82 chars long when vertically aligned. Right-aligned it at 80. 

Some alternatives are: 
- 4 space indent
- break param type and param name onto separate lines
- put "void" return type on a line of its own above the method name to reduce
line length

What do you think?

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.cc#newcode188
dbus/test_service.cc:188: dbus::ExportedObject::ResponseSender response_sender)
{
On 2011/11/22 22:38:17, satorux1 wrote:
> ditto

This one is 86. Right aligned at 80. See above.

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.h
File dbus/test_service.h (right):

http://codereview.chromium.org/8637002/diff/5001/dbus/test_service.h#newcode93
dbus/test_service.h:93: dbus::ExportedObject::ResponseSender response_sender);
On 2011/11/22 22:38:17, satorux1 wrote:
> please align parameters vertically.
> 
> void Echo(MethodCall* method_call,
>           dbus::ExportedObject::ResponseSender response_sender);

Done.

[satorux1, 2011-11-22 23:55:38]

LGTM with style nits.

http://codereview.chromium.org/8637002/diff/12001/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/8637002/diff/12001/dbus/test_service.cc#newcod...
dbus/test_service.cc:182: dbus::ExportedObject::ResponseSender response_sender)
{
In this case, the following is preferable:

void TestService::SlowEcho(
    MethodCall* method_call,
    dbus::ExportedObject::ResponseSender response_sender) {

http://codereview.chromium.org/8637002/diff/12001/dbus/test_service.cc#newcod...
dbus/test_service.cc:188: dbus::ExportedObject::ResponseSender response_sender)
{
ditto.

[Vince Laviano, 2011-11-23 00:04:24]

http://codereview.chromium.org/8637002/diff/12001/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/8637002/diff/12001/dbus/test_service.cc#newcod...
dbus/test_service.cc:182: dbus::ExportedObject::ResponseSender response_sender)
{
On 2011/11/22 23:55:39, satorux1 wrote:
> In this case, the following is preferable:
> 
> void TestService::SlowEcho(
>     MethodCall* method_call,
>     dbus::ExportedObject::ResponseSender response_sender) {
> 
> 

Done.

http://codereview.chromium.org/8637002/diff/12001/dbus/test_service.cc#newcod...
dbus/test_service.cc:188: dbus::ExportedObject::ResponseSender response_sender)
{
On 2011/11/22 23:55:39, satorux1 wrote:
> ditto.

Done.

[satorux1, 2011-11-23 00:05:48]

LGTM again just in case.

[Vince Laviano, 2011-11-23 22:42:07]

Uploaded a new patchset that fixes a memory management issue in
ExportedObject::HandleMessage that I discovered during testing.

On the !bus_->HasDBusThread() execution path, we were calling:

iter->second.Run(method_call.get(),
                 base::Bind(&ExportedObject::SendResponse,
                 this,
                 start_time,
                 method_call.release())

However, the order of evaluation of function arguments isn't defined by C++, and
method_call.release() was being called before method_call.get(). This caused a
NULL method_call pointer to be passed to the service provider callback.

PTAL

[satorux1, 2011-11-23 23:31:45]

LGTM. wow, this was a subtle bug.

http://codereview.chromium.org/8637002/diff/18001/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/8637002/diff/18001/dbus/exported_object.cc#new...
dbus/exported_object.cc:228: MethodCall* method_call_ptr =
method_call.release();
method_call_ptr -> released_method_call may be a bit more readable.

[Vince Laviano, 2011-11-24 00:14:12]

Fixed style nit and added the unit test that we discussed offline.

http://codereview.chromium.org/8637002/diff/18001/dbus/exported_object.cc
File dbus/exported_object.cc (right):

http://codereview.chromium.org/8637002/diff/18001/dbus/exported_object.cc#new...
dbus/exported_object.cc:228: MethodCall* method_call_ptr =
method_call.release();
On 2011/11/23 23:31:46, satorux1 wrote:
> method_call_ptr -> released_method_call may be a bit more readable.

Done.

[satorux1, 2011-11-24 00:17:41]

LGTM++. Awesome.

http://codereview.chromium.org/8637002/diff/22001/dbus/test_service.cc
File dbus/test_service.cc (right):

http://codereview.chromium.org/8637002/diff/22001/dbus/test_service.cc#newcod...
dbus/test_service.cc:202: base::Bind(&TestService::Echo,
Oh nice, you could reuse this. :)

[commit-bot: I haz the power, 2011-11-24 00:20:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vlaviano@chromium.org/8637002/22001

[commit-bot: I haz the power, 2011-11-24 02:23:35]

Change committed as 111479