bluetooth: Modify DCHECK to conditionals in several BluetoothAdapterChromeOS methods.

bluetooth: Modify DCHECK to conditionals in several BluetoothAdapterChromeOS methods.

Initial patch in https://codereview.chromium.org/848613003 used
DCHECK in too many methods that should function even when
the adapter is not present.

BUG=452705
Committed: https://crrev.com/8b2a039c029653d4be508f6acce4cb162c68aea0
Cr-Commit-Position: refs/heads/master@{#317332}

[scheib, 2015-02-17 19:36:28]

scheib@chromium.org changed reviewers:
+ armansito@chromium.org

[scheib, 2015-02-17 19:38:03]

In BluetoothAdapterChromeOS::ProcessQueuedDiscoveryRequests I believe the
correct thing there is to not have any conditional on IsPresent .. call the
error callback and ProcessQueuedDiscoveryRequests.

[armansito, 2015-02-17 21:10:29]

On 2015/02/17 19:38:03, scheib wrote:
> In BluetoothAdapterChromeOS::ProcessQueuedDiscoveryRequests I believe the
> correct thing there is to not have any conditional on IsPresent .. call the
> error callback and ProcessQueuedDiscoveryRequests.

Can you add unit tests that fail without fix but succeed with it? That would be
the best way to document the expected behavior and avoid future regressions.

[scheib, 2015-02-19 01:10:12]

Patchset #2 (id:20001) has been deleted

[scheib, 2015-02-19 01:13:57]

Thanks, armansito PTAL at the expectations I've come up with for methods in the
tests.

The following I probably need to still come up with tests for, but thoughts
welcome if you agree we can skip those or have thoughts on all the others.
  // OnRegisterProfile 
  // SetProfileDelegate
  // OnRegisterProfileError

[armansito, 2015-02-19 01:44:47]

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:484: return;
Since we don't really do anything, here I would just omit this. It's nice to at
least do the logging below.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:663: if (!IsPresent())
This really shouldn't happen, since this method gets called on the adapter. If
the adapter disappears during the call, then we won't get success here. If we
get success and the adapter disappears in the mean time, then the audio sink
will process it via the observer event.

In either case, we should notify the upper layer via |callback| or
|error_callback| here. I would bind the error_callback to the original call and
invoke it here if the adapter isn't present, and log an error message.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:1023: }
nit: newline after '}'.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:1209: }
Wouldn't we want to clear the pending discovery requests here and invoke their
error callback?

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:45: // Methods tolerate the dbus
system being before the BluetoothAdapterChromeOS
nit: '...dbus system being _shutdown_ before the...'?

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:156: friend class
BluetoothChromeOSTest_Shutdown_Test;
Is this necessary? I thought the friendship above would be enough, though I may
be wrong.

[scheib, 2015-02-19 21:34:15]

PTAL

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:484: return;
On 2015/02/19 01:44:47, armansito wrote:
> Since we don't really do anything, here I would just omit this. It's nice to
at
> least do the logging below.

Logging moved above return. The conditional return kept as the DCHECK for agent_
will fail after Shutdown().

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:663: if (!IsPresent())
On 2015/02/19 01:44:47, armansito wrote:
> This really shouldn't happen, since this method gets called on the adapter. If
> the adapter disappears during the call, then we won't get success here. If we
> get success and the adapter disappears in the mean time, then the audio sink
> will process it via the observer event.
> 
> In either case, we should notify the upper layer via |callback| or
> |error_callback| here. I would bind the error_callback to the original call
and
> invoke it here if the adapter isn't present, and log an error message.

Done.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:1023: }
On 2015/02/19 01:44:47, armansito wrote:
> nit: newline after '}'.

Done.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:1209: }
On 2015/02/19 01:44:47, armansito wrote:
> Wouldn't we want to clear the pending discovery requests here and invoke their
> error callback?

Done.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:45: // Methods tolerate the dbus
system being before the BluetoothAdapterChromeOS
On 2015/02/19 01:44:47, armansito wrote:
> nit: '...dbus system being _shutdown_ before the...'?

Done.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:156: friend class
BluetoothChromeOSTest_Shutdown_Test;
On 2015/02/19 01:44:47, armansito wrote:
> Is this necessary? I thought the friendship above would be enough, though I
may
> be wrong.

Yes, the previous friend only applies to the fixture. A subclass of the fixture
is made for each test, and it doesn't have friend status.

It is possible to have a single friend listed here by adding wrappers for every
call to the test fixture and calling those from a test instead of directly on
the adapter, but for so many methods that's too much plumbing vs just adding
another friend line per test that needs it.

[armansito, 2015-02-19 21:40:49]

lgtm with some final comments.

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/933823003/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:156: friend class
BluetoothChromeOSTest_Shutdown_Test;
On 2015/02/19 21:34:15, scheib wrote:
> On 2015/02/19 01:44:47, armansito wrote:
> > Is this necessary? I thought the friendship above would be enough, though I
> may
> > be wrong.
> 
> Yes, the previous friend only applies to the fixture. A subclass of the
fixture
> is made for each test, and it doesn't have friend status.
> 
> It is possible to have a single friend listed here by adding wrappers for
every
> call to the test fixture and calling those from a test instead of directly on
> the adapter, but for so many methods that's too much plumbing vs just adding
> another friend line per test that needs it.

sgtm

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:1218: // the queue.
Sorry for the push back, can you add a unit test for this case if you don't have
one already?

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:45: // Methods tolerate a shutdown
senario where BluetoothAdapterChromeOS::Shutdown
nit: s/senario/scenario/

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:46: // caues IsPresent to return
false just before the dbus system is shutdown but
nit: s/caues/causes/

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:50: // for all new methods.
nit: Is this last sentence a TODO?

[scheib, 2015-02-20 00:37:19]

New patchsets have been uploaded after l-g-t-m from armansito@chromium.org

[scheib, 2015-02-20 00:41:17]

PTAL

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:1218: // the queue.
On 2015/02/19 21:40:49, armansito wrote:
> Sorry for the push back, can you add a unit test for this case if you don't
have
> one already?

Done. I've added 4 more unit tests to verify that queued up sessions are all
responded to.

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:45: // Methods tolerate a shutdown
senario where BluetoothAdapterChromeOS::Shutdown
On 2015/02/19 21:40:49, armansito wrote:
> nit: s/senario/scenario/

Done.

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:46: // caues IsPresent to return
false just before the dbus system is shutdown but
On 2015/02/19 21:40:49, armansito wrote:
> nit: s/caues/causes/

Done.

https://codereview.chromium.org/933823003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:50: // for all new methods.
On 2015/02/19 21:40:49, armansito wrote:
> nit: Is this last sentence a TODO?

Reworded as maintenance instructions.

[armansito, 2015-02-20 02:17:52]

lgtm

[scheib, 2015-02-20 16:22:21]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2015-02-20 16:23:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933823003/80001

[commit-bot: I haz the power, 2015-02-20 16:26:54]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-20 16:27:24]

Patchset 4 (id:??) landed as
https://crrev.com/8b2a039c029653d4be508f6acce4cb162c68aea0
Cr-Commit-Position: refs/heads/master@{#317332}