Supporting callback for Disconnected signal.

Supporting callback for Disconnected signal.

If the connection with dbus-daemon is closed, callback function set with
SetDisconnectedCallback will be called.

BUG=None
TEST=ran dbus_unittests


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

[satorux1, 2013-02-13 08:06:53]

https://codereview.chromium.org/12224139/diff/1/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/12224139/diff/1/dbus/bus.h#newcode208
dbus/bus.h:208: typedef base::Callback<void ()> OnDisconnectedCallback;
I think you can just use base::Closure because this callback does not take
anything

https://codereview.chromium.org/12224139/diff/1/dbus/bus.h#newcode501
dbus/bus.h:501: friend class DisconnectableBusForTesting;
This worries me because

1) 'friend' should be avoided in general
2) inheritance should also be avoided in general.

Seems to me that  DisconnectableBusForTesting just adds CloseConnection()
function.

I think it'd be simpler to add CloseConnection() to the Bus class.

https://codereview.chromium.org/12224139/diff/1/dbus/end_to_end_async_unittes...
File dbus/end_to_end_async_unittest.cc (right):

https://codereview.chromium.org/12224139/diff/1/dbus/end_to_end_async_unittes...
dbus/end_to_end_async_unittest.cc:55: EndToEndAsyncTest() :
on_disconnected_callcount_(0) {}
callcount -> call_count

[Seigo Nonaka, 2013-02-13 09:45:29]

https://codereview.chromium.org/12224139/diff/1/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/12224139/diff/1/dbus/bus.h#newcode208
dbus/bus.h:208: typedef base::Callback<void ()> OnDisconnectedCallback;
On 2013/02/13 08:06:53, satorux1 wrote:
> I think you can just use base::Closure because this callback does not take
> anything

Done.

https://codereview.chromium.org/12224139/diff/1/dbus/bus.h#newcode501
dbus/bus.h:501: friend class DisconnectableBusForTesting;
On 2013/02/13 08:06:53, satorux1 wrote:
> This worries me because
> 
> 1) 'friend' should be avoided in general
> 2) inheritance should also be avoided in general.
> 
> Seems to me that  DisconnectableBusForTesting just adds CloseConnection()
> function.
> 
> I think it'd be simpler to add CloseConnection() to the Bus class.

Sure, adding CloseConnection.
Thanks.

https://codereview.chromium.org/12224139/diff/1/dbus/end_to_end_async_unittes...
File dbus/end_to_end_async_unittest.cc (right):

https://codereview.chromium.org/12224139/diff/1/dbus/end_to_end_async_unittes...
dbus/end_to_end_async_unittest.cc:55: EndToEndAsyncTest() :
on_disconnected_callcount_(0) {}
On 2013/02/13 08:06:53, satorux1 wrote:
> callcount -> call_count

Done.

[satorux1, 2013-02-14 02:30:40]

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

https://codereview.chromium.org/12224139/diff/12001/dbus/bus.h#newcode208
dbus/bus.h:208: void SetDisconnectedCallback(const base::Closure& closure);
What about adding the closure parameter to CloseConnection() instead?

[Seigo Nonaka, 2013-02-14 03:33:52]

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

https://codereview.chromium.org/12224139/diff/12001/dbus/bus.h#newcode208
dbus/bus.h:208: void SetDisconnectedCallback(const base::Closure& closure);
I think your suggestion doesn't help my original motivation. 
The disconnection is not only from CloseConnection, such as dbus-daemon
crashing.

One way to remove this function is adding closure parameter into dbus::Options
and setting it in ctor.
WDYT?

On 2013/02/14 02:30:41, satorux1 wrote:
> What about adding the closure parameter to CloseConnection() instead?

[satorux1, 2013-02-14 04:26:40]

On 2013/02/14 03:33:52, Seigo Nonaka wrote:
> https://codereview.chromium.org/12224139/diff/12001/dbus/bus.h
> File dbus/bus.h (right):
> 
> https://codereview.chromium.org/12224139/diff/12001/dbus/bus.h#newcode208
> dbus/bus.h:208: void SetDisconnectedCallback(const base::Closure& closure);
> I think your suggestion doesn't help my original motivation. 
> The disconnection is not only from CloseConnection, such as dbus-daemon
> crashing.
> 
> One way to remove this function is adding closure parameter into dbus::Options
> and setting it in ctor.
> WDYT?

Sounds good!


> 
> On 2013/02/14 02:30:41, satorux1 wrote:
> > What about adding the closure parameter to CloseConnection() instead?

[Seigo Nonaka, 2013-02-14 05:18:16]

Added closure to bus::Option instead of setter function.
PTAL?

Thank you.
On 2013/02/14 04:26:40, satorux1 wrote:
> On 2013/02/14 03:33:52, Seigo Nonaka wrote:
> > https://codereview.chromium.org/12224139/diff/12001/dbus/bus.h
> > File dbus/bus.h (right):
> > 
> > https://codereview.chromium.org/12224139/diff/12001/dbus/bus.h#newcode208
> > dbus/bus.h:208: void SetDisconnectedCallback(const base::Closure& closure);
> > I think your suggestion doesn't help my original motivation. 
> > The disconnection is not only from CloseConnection, such as dbus-daemon
> > crashing.
> > 
> > One way to remove this function is adding closure parameter into
dbus::Options
> > and setting it in ctor.
> > WDYT?
> 
> Sounds good!
> 
> 
> > 
> > On 2013/02/14 02:30:41, satorux1 wrote:
> > > What about adding the closure parameter to CloseConnection() instead?

[satorux1, 2013-02-15 03:55:20]

Sorry for the belated response.

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc#newcode381
dbus/bus.cc:381: if (connection_type_ == PRIVATE)
why do we need to check the type?

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.h#newcode195
dbus/bus.h:195: // will be called on the original thread. This is also called
when the
original -> origin

[Seigo Nonaka, 2013-02-15 04:18:36]

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc#newcode381
dbus/bus.cc:381: if (connection_type_ == PRIVATE)
Because dbus_connection_close is only for private connection.
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2522ac5...

But certainly, this function name is confusing.
Changing to CHECK and also changing comment.

On 2013/02/15 03:55:20, satorux1 wrote:
> why do we need to check the type?

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.h
File dbus/bus.h (right):

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.h#newcode195
dbus/bus.h:195: // will be called on the original thread. This is also called
when the
On 2013/02/15 03:55:20, satorux1 wrote:
> original -> origin

Done.

[satorux1, 2013-02-15 04:25:11]

LGTM with nits

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc#newcode381
dbus/bus.cc:381: if (connection_type_ == PRIVATE)
On 2013/02/15 04:18:36, Seigo Nonaka wrote:
> Because dbus_connection_close is only for private connection.
>
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2522ac5...
> 
> But certainly, this function name is confusing.
> Changing to CHECK and also changing comment.
> 
> On 2013/02/15 03:55:20, satorux1 wrote:
> > why do we need to check the type?
> 


Then let's rename this function to ClosePrivateConnection().

https://codereview.chromium.org/12224139/diff/16001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12224139/diff/16001/dbus/bus.cc#newcode380
dbus/bus.cc:380: CHECK_EQ(PRIVATE, connection_type_) << "Do not close non
private connection.";
Let's make it a DCHECK_EQ(). We rarely use CHECKs.

"Do not close non private connection." -> "non-private connection should not be
closed"

[Seigo Nonaka, 2013-02-15 04:36:10]

Thank you for your review!
Submitting...

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12224139/diff/9007/dbus/bus.cc#newcode381
dbus/bus.cc:381: if (connection_type_ == PRIVATE)
On 2013/02/15 04:25:11, satorux1 wrote:
> On 2013/02/15 04:18:36, Seigo Nonaka wrote:
> > Because dbus_connection_close is only for private connection.
> >
>
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga2522ac5...
> > 
> > But certainly, this function name is confusing.
> > Changing to CHECK and also changing comment.
> > 
> > On 2013/02/15 03:55:20, satorux1 wrote:
> > > why do we need to check the type?
> > 
> 
> 
> Then let's rename this function to ClosePrivateConnection().

Done.

https://codereview.chromium.org/12224139/diff/16001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12224139/diff/16001/dbus/bus.cc#newcode380
dbus/bus.cc:380: CHECK_EQ(PRIVATE, connection_type_) << "Do not close non
private connection.";
On 2013/02/15 04:25:12, satorux1 wrote:
> Let's make it a DCHECK_EQ(). We rarely use CHECKs.
> 
> "Do not close non private connection." -> "non-private connection should not
be
> closed"

Done.

[commit-bot: I haz the power, 2013-02-15 04:37:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/12224139/5009

[commit-bot: I haz the power, 2013-02-15 16:14:55]

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

[commit-bot: I haz the power, 2013-02-15 16:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/12224139/5009

[commit-bot: I haz the power, 2013-02-15 16:44:41]

Change committed as 182736