Add a method to check if a D-Bus service has an owner. Use it for mtpd.

Add a method to check if a D-Bus service has an owner. Use it for mtpd.

BUG=181064
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198328

[Lei Zhang, 2013-04-30 00:55:39]

Is this what you had in mind when you filed bug 181064?

[satorux1, 2013-04-30 01:53:10]

yes! thank you for adding this, but i think we can make it better.

https://codereview.chromium.org/14568005/diff/1/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

https://codereview.chromium.org/14568005/diff/1/dbus/object_proxy.cc#newcode249
dbus/object_proxy.cc:249: const std::string& service_name) {
thread checking is missing. bus_->AssertOnDBusThread() ?

https://codereview.chromium.org/14568005/diff/1/dbus/object_proxy.h
File dbus/object_proxy.h (right):

https://codereview.chromium.org/14568005/diff/1/dbus/object_proxy.h#newcode161
dbus/object_proxy.h:161: static bool ServiceHasOwner(dbus::Bus* bus, const
std::string& service_name);
Looks rather awkward to have this in ObjectProxy as a static member function.
Maybe move this to bus.h as a non-static member function?

https://codereview.chromium.org/14568005/diff/1/device/media_transfer_protoco...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/14568005/diff/1/device/media_transfer_protoco...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:81:
bus->PostTaskToDBusThread(FROM_HERE, dbus_task);
This is awkward. I think we should provide an asynchronous utility function in
'dbus', so clients don't have to deal with posting something to the D-Bus
thread.

[Lei Zhang, 2013-04-30 03:57:43]

I hope patch set 2 addresses all your concerns.

[satorux1, 2013-04-30 04:50:00]

LGTM with nits. Thanks!

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc#newcode843
dbus/bus.cc:843: std::string service_owner;
please move this to right before PopString

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc#newcode855
dbus/bus.cc:855: return service_owner;
nit: return "" is a bit clearer.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc#newcode868
dbus/bus.cc:868: return service_owner;
ditto.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h#newcode544
dbus/bus.h:544: virtual std::string GetServiceOwnerAndBlock(const std::string&
service_name,
Seems to me that we we don't need to expose the blocking version. Please move
this to private: section.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h#newcode545
dbus/bus.h:545: bool suppress_errors);
so far, this is always true, right? please remove this parameter.

https://codereview.chromium.org/14568005/diff/12001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/object_proxy.cc#newc...
dbus/object_proxy.cc:52: void EmptyResponseCallbackBody(Response*
unused_response) {
unused_response -> /* response */

thank you for cleaning up this file.

https://codereview.chromium.org/14568005/diff/12001/dbus/object_proxy.cc#newc...
dbus/object_proxy.cc:613: void ObjectProxy::UpdateNameOwnerAndBlock() {
Please keep bus_->AssertOnDBusThread();

https://codereview.chromium.org/14568005/diff/12001/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/14568005/diff/12001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:389:
dbus::Bus* GetBus() {
function comment missing

https://codereview.chromium.org/14568005/diff/12001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:398: void
FinishSetupOnOriginThread(const std::string& service_owner) {
ditto.

[Lei Zhang, 2013-04-30 20:49:23]

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc#newcode843
dbus/bus.cc:843: std::string service_owner;
On 2013/04/30 04:50:00, satorux1 wrote:
> please move this to right before PopString

Done.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc#newcode855
dbus/bus.cc:855: return service_owner;
On 2013/04/30 04:50:00, satorux1 wrote:
> nit: return "" is a bit clearer.

Done.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.cc#newcode868
dbus/bus.cc:868: return service_owner;
On 2013/04/30 04:50:00, satorux1 wrote:
> ditto.

Done.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h#newcode544
dbus/bus.h:544: virtual std::string GetServiceOwnerAndBlock(const std::string&
service_name,
On 2013/04/30 04:50:00, satorux1 wrote:
> Seems to me that we we don't need to expose the blocking version. Please move
> this to private: section.

Used in object_proxy.cc.

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h#newcode545
dbus/bus.h:545: bool suppress_errors);
On 2013/04/30 04:50:00, satorux1 wrote:
> so far, this is always true, right? please remove this parameter.

It's false in object_proxy.cc.

https://codereview.chromium.org/14568005/diff/12001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/object_proxy.cc#newc...
dbus/object_proxy.cc:52: void EmptyResponseCallbackBody(Response*
unused_response) {
On 2013/04/30 04:50:00, satorux1 wrote:
> unused_response -> /* response */
> 
> thank you for cleaning up this file.

Done.

https://codereview.chromium.org/14568005/diff/12001/dbus/object_proxy.cc#newc...
dbus/object_proxy.cc:613: void ObjectProxy::UpdateNameOwnerAndBlock() {
On 2013/04/30 04:50:00, satorux1 wrote:
> Please keep bus_->AssertOnDBusThread();

Done.

https://codereview.chromium.org/14568005/diff/12001/device/media_transfer_pro...
File device/media_transfer_protocol/media_transfer_protocol_manager.cc (right):

https://codereview.chromium.org/14568005/diff/12001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:389:
dbus::Bus* GetBus() {
On 2013/04/30 04:50:00, satorux1 wrote:
> function comment missing

Done.

https://codereview.chromium.org/14568005/diff/12001/device/media_transfer_pro...
device/media_transfer_protocol/media_transfer_protocol_manager.cc:398: void
FinishSetupOnOriginThread(const std::string& service_owner) {
On 2013/04/30 04:50:00, satorux1 wrote:
> ditto.

Done.

[satorux1, 2013-05-01 01:19:27]

LGTM with a request:

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h#newcode545
dbus/bus.h:545: bool suppress_errors);
On 2013/04/30 20:49:23, Lei Zhang wrote:
> On 2013/04/30 04:50:00, satorux1 wrote:
> > so far, this is always true, right? please remove this parameter.
> 
> It's false in object_proxy.cc.

I see. Then could you make it an enum as boolean is hard to read in client code
(ex. false of what?) like

enum GetServiceOwnerOption {
  REPORT_ERRORS,
  SUPPRESS_ERRORS,
}

https://codereview.chromium.org/14568005/diff/20001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

https://codereview.chromium.org/14568005/diff/20001/dbus/object_proxy.cc#newc...
dbus/object_proxy.cc:615: service_name_owner_ =
bus_->GetServiceOwnerAndBlock(service_name_, false);
false of what? :)

[Lei Zhang, 2013-05-01 01:40:50]

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/14568005/diff/12001/dbus/bus.h#newcode545
dbus/bus.h:545: bool suppress_errors);
On 2013/05/01 01:19:27, satorux1 wrote:
> On 2013/04/30 20:49:23, Lei Zhang wrote:
> > On 2013/04/30 04:50:00, satorux1 wrote:
> > > so far, this is always true, right? please remove this parameter.
> > 
> > It's false in object_proxy.cc.
> 
> I see. Then could you make it an enum as boolean is hard to read in client
code
> (ex. false of what?) like
> 
> enum GetServiceOwnerOption {
>   REPORT_ERRORS,
>   SUPPRESS_ERRORS,
> }

Done.

https://codereview.chromium.org/14568005/diff/20001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

https://codereview.chromium.org/14568005/diff/20001/dbus/object_proxy.cc#newc...
dbus/object_proxy.cc:615: service_name_owner_ =
bus_->GetServiceOwnerAndBlock(service_name_, false);
On 2013/05/01 01:19:27, satorux1 wrote:
> false of what? :)

Done.

[commit-bot: I haz the power, 2013-05-01 01:41:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14568005/27001

[commit-bot: I haz the power, 2013-05-01 02:09:03]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-01 03:22:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14568005/27001

[commit-bot: I haz the power, 2013-05-01 03:29:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-01 19:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14568005/48001

[commit-bot: I haz the power, 2013-05-01 19:34:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-02 03:23:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14568005/48001

[commit-bot: I haz the power, 2013-05-02 05:27:21]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-04 00:26:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/14568005/48001

[commit-bot: I haz the power, 2013-05-04 15:48:39]

Change committed as 198328