Call get_dispatch_status function to handle Disconnected signal.

Call get_dispatch_status function to handle Disconnected signal.

It turns out that dbus_connection_get_dispatch_status should be called even if
the connection is lost, otherwise we miss "Disconnected" signal.

BUG=174431
TEST=Manually done on alex and ran dbus_unittests.


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

[satorux1, 2013-02-06 03:15:38]

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc#newcode6
dbus/bus.cc:6: // - Handle "disconnected" signal.
please remove this.

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc#newcode866
dbus/bus.cc:866: void Bus::OnConnectionDisconnected(DBusConnection* connection)
{
on what thread this function is called? Please annotate with AssertOn*

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc#newcode916
dbus/bus.cc:916: void *data) {
ditto. please annotate the thread.

[Seigo Nonaka, 2013-02-06 04:57:22]

Sorry, there are few DCHECK failure.
Latest patch fixes them and checked this works on Debug configuration.

I talked with you and decided not calling ShutdownAndBlock on Disconnected
signal handler, but I found the safe way to call ShutdownAndBlock.
Please take an another look?

Thank you.
Seigo

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc#newcode6
dbus/bus.cc:6: // - Handle "disconnected" signal.
On 2013/02/06 03:15:39, satorux1 wrote:
> please remove this.

Done.

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc#newcode866
dbus/bus.cc:866: void Bus::OnConnectionDisconnected(DBusConnection* connection)
{
On 2013/02/06 03:15:39, satorux1 wrote:
> on what thread this function is called? Please annotate with AssertOn*

Done.

https://codereview.chromium.org/12211022/diff/3001/dbus/bus.cc#newcode916
dbus/bus.cc:916: void *data) {
On 2013/02/06 03:15:39, satorux1 wrote:
> ditto. please annotate the thread.

Done.

[Seigo Nonaka, 2013-02-08 07:58:30]

ping, just in case.

[satorux1, 2013-02-08 08:11:10]

https://codereview.chromium.org/12211022/diff/1003/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12211022/diff/1003/dbus/bus.cc#newcode872
dbus/bus.cc:872: AssertOnDBusThread();
nit: add a blank line here.

https://codereview.chromium.org/12211022/diff/1003/dbus/bus.cc#newcode878
dbus/bus.cc:878: // Triggered by ShutdownAndBlock. Just do nothing.
I'm confused. Why we can tell that this is triggered by ShutdownAndBlock()? 

Would it be better to check |shutdown_completed_| instead?

[Seigo Nonaka, 2013-02-08 08:25:41]

https://codereview.chromium.org/12211022/diff/1003/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12211022/diff/1003/dbus/bus.cc#newcode872
dbus/bus.cc:872: AssertOnDBusThread();
On 2013/02/08 08:11:10, satorux1 wrote:
> nit: add a blank line here.

Done.

https://codereview.chromium.org/12211022/diff/1003/dbus/bus.cc#newcode878
dbus/bus.cc:878: // Triggered by ShutdownAndBlock. Just do nothing.
Right!
We should use |shutodown_completed_|.

Done.
On 2013/02/08 08:11:10, satorux1 wrote:
> I'm confused. Why we can tell that this is triggered by ShutdownAndBlock()? 
> 
> Would it be better to check |shutdown_completed_| instead?

[satorux1, 2013-02-08 08:29:42]

LGTM

[Seigo Nonaka, 2013-02-08 08:31:55]

Thank you for your review!
Submitting...

[commit-bot: I haz the power, 2013-02-08 08:32:04]

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

[commit-bot: I haz the power, 2013-02-08 10:53:44]

Change committed as 181479

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

https://codereview.chromium.org/12211022/diff/2004/dbus/bus.cc
File dbus/bus.cc (right):

https://codereview.chromium.org/12211022/diff/2004/dbus/bus.cc#newcode380
dbus/bus.cc:380: return;  // Already shutdowned, just return.
Do we need this? Looks unnecessary.