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: Iab009ddbd12dea1e12299ae0ddccd4e430d9cf97


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

[Vince Laviano, 2011-11-29 21:40:23]

This is a resubmission of a reverted CL.  The previous code review is here:
http://codereview.chromium.org/8637002/

I had neglected to update proxy_resolution_service_provider_unittest.cc to take
the changes to the dbus exported object API into account.  

Previously, dbus::ExportedObject::MethodCallCallbacks took a MethodCall* and
returned a Response*. With this CL, they now return void and take both a
MethodCall* and a ResponseSender. Instead of having to immediately return a
response, exported methods can now either send an immediate response or store
the ResponseSender and send an asynchronous response later. 

I've updated the proxy resolution service provider unit test to be compatible
with this new approach to sending method replies.

[stevenjb, 2011-11-29 22:06:53]

LGTM

http://codereview.chromium.org/8728020/diff/1/chrome/browser/chromeos/dbus/pr...
File chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc (right):

http://codereview.chromium.org/8728020/diff/1/chrome/browser/chromeos/dbus/pr...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc:250: // Return
an empty response for now. We'll send a signal once the
nit: fix comment (s/Return/Send/ ?)

[Vince Laviano, 2011-11-29 22:11:47]

http://codereview.chromium.org/8728020/diff/1/chrome/browser/chromeos/dbus/pr...
File chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc (right):

http://codereview.chromium.org/8728020/diff/1/chrome/browser/chromeos/dbus/pr...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc:250: // Return
an empty response for now. We'll send a signal once the
On 2011/11/29 22:06:53, Steven Bennetts wrote:
> nit: fix comment (s/Return/Send/ ?)

Done.

[commit-bot: I haz the power, 2011-11-29 22:11:57]

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

[commit-bot: I haz the power, 2011-11-30 00:12:46]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 112050, job name
was 8728020-4001 (retry) (previous was lost) (previous was lost) (previous was
lost).

[commit-bot: I haz the power, 2011-11-30 00:15:14]

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

[commit-bot: I haz the power, 2011-11-30 02:01:40]

Try job failure for 8728020-4001 (retry) on mac_rel for step "sql_unittests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-30 02:36:58]

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

[commit-bot: I haz the power, 2011-11-30 04:57:44]

Change committed as 112131

[Paweł Hajdan Jr., 2011-11-30 08:46:10]

Drive-by, it's even quite possible that the "while" loop can be just removed.

Could you do a follow-up CL please?

http://codereview.chromium.org/8728020/diff/4001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc
(right):

http://codereview.chromium.org/8728020/diff/4001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc:266:
message_loop_.Run();
This is generally an anti-pattern. Could you only Run the MessageLoop once and
quit it once? Running ML in a loop is harder to debug, and suggests you're not
waiting for an event. Instead of checking the condition in the loop, you should
Quit the loop in the place where you currently set response_received_ to true.

[Vince Laviano, 2011-11-30 23:47:47]

http://codereview.chromium.org/8728020/diff/4001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc
(right):

http://codereview.chromium.org/8728020/diff/4001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc:266:
message_loop_.Run();
On 2011/11/30 08:46:11, Paweł Hajdan Jr. wrote:
> This is generally an anti-pattern. Could you only Run the MessageLoop once and
> quit it once? Running ML in a loop is harder to debug, and suggests you're not
> waiting for an event. Instead of checking the condition in the loop, you
should
> Quit the loop in the place where you currently set response_received_ to true.

http://codereview.chromium.org/8764006/