Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
939753004:
device/bluetooth: Implement Unregister() of BlueotoothAudioSinkChromeOS and disconnection-related c… (Closed)
|
Created:
5 years, 10 months ago by Miao Modified:
5 years, 10 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondevice/bluetooth: Implement Unregister() of BlueotoothAudioSinkChromeOS and disconnection-related clean-up.
This CL implements all functions related to unregistering
BluetoothAudioSinkChromeOS and cleaning up the resources, such as
media, media endpoint, media transport and transport-specific
properties. These functions will be called when a connect between
a remote device and an A2DP sink closes. The corresponding tests
are included in BluetoothAudioSinkChromeOSTest.
BUG=441581
TEST=device_unittests --gtest_filter=*AudioSink*
Committed: https://crrev.com/4a0cac258fddaafd18cdcd0cea53121826ab4c3a
Cr-Commit-Position: refs/heads/master@{#318266}
Patch Set 1 #
Total comments: 39
Patch Set 2 : Revised helper functions in FakeBluetoothMediaClient and FakeBluetoothMediaTransportClient and modi… #
Total comments: 25
Patch Set 3 : Changed volume_ back to uint16_t with a defined range. #
Total comments: 10
Patch Set 4 : Moved kInvalidVolume to BluetoothAudioSink. #
Total comments: 4
Patch Set 5 : Defined Transport structure, added two setters for state/volume, and modified GetProperties(). #
Total comments: 11
Patch Set 6 : Fixed nits. #
Total comments: 6
Patch Set 7 : #Messages
Total messages: 22 (5 generated)
mcchou@chromium.org changed reviewers: + armansito@chromium.org, benchan@chromium.org
This CL is ready for review.
https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:85: if (endpoints_.find(endpoint_path) == endpoints_.end()) { see ContainsKey in src/base/stl_util.h https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:86: // since user applications can be notified of an error returned by the call. is this comment still valid? "since user applications can be notified of an error returned by the call" https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:39: volume_(nullptr), no need to initialize scoped_ptr to nullptr what's the purpose of this change? https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:142: StateChanged(BluetoothAudioSink::STATE_DISCONNECTED); this may be simpler: if (state_ != BluetoothAudioSink::STATE_INVALID) StateChanged(BluetoothAudioSink::STATE_DISCONNECTED); https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:207: volume_.reset(new uint16_t()); this looks kinda weird as VolumeChanged does nothing if |volume_| is nullptr perhaps VolumeChanged should take a pointer as well https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:385: volume_ = 0; volume_.reset()? https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:42: void Unregister(const base::Closure& callback) override; comment needs updated
First round of comments, haven't looked at the unit tests yet. https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:119: endpoints_[endpoint_path] = registered; Do an early return here. https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right): https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:62: transport->SetValid(object_path_, false); It's a bit strange that the endpoint service provider invalidates the transport rather than the other way around. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:87: virtual void Unregister(const base::Closure& callback) = 0; Why did you remove the error callback? Also, without the error callback, the comment above doesn't make much sense, since you can't return an error. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:66: media_endpoint_.get() != nullptr) { nit: No need to compare against nullptr, the pointer itself will evaluate to true of false based on its value. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:83: void BluetoothAudioSinkChromeOS::Unregister(const base::Closure& callback) { Note that if BluetoothAudioSinkChromeOS outlives DBusThreadManager, the code below will crash. You should add a TODO here saying that we should provide this guarantee in the future. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:115: return volume_.get() != nullptr ? *volume_ : 0; nit: no need for '!= nullptr' https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:206: if (properties.volume.get() != nullptr) { nit: no need for '!= nullptr' https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:207: volume_.reset(new uint16_t()); Why not make this assignment from VolumeChanged instead of here? https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:287: case BluetoothAudioSink::STATE_INVALID: { All of these added scopes are unnecessary. I would remove them. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:292: if (state_ != BluetoothAudioSink::STATE_DISCONNECTED) Kind of unnecessary I think, I'd just make ResetTransport not do anything if it doesn't have anything to clean up. So, remove the break and the if statement and just fall through to the next case. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:327: if (volume_.get() == nullptr) nit: if (!volume_.get()) That said, your code looks a bit odd. You're initializing volume_ both here and outside, in addition you have this weird check, I'd change this so that you just initialize it here if volume_ is NULL: if (!volume_.get()) volume_.reset(new...); https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:384: VolumeChanged(0); So did volume just become 0 or are you cleaning up the volume in general?
https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:85: if (endpoints_.find(endpoint_path) == endpoints_.end()) { On 2015/02/19 00:21:45, Ben Chan wrote: > see ContainsKey in src/base/stl_util.h Done. https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_client.cc:119: endpoints_[endpoint_path] = registered; On 2015/02/19 00:37:46, armansito wrote: > Do an early return here. Done. https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right): https://codereview.chromium.org/939753004/diff/1/chromeos/dbus/fake_bluetooth... chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:62: transport->SetValid(object_path_, false); On 2015/02/19 00:37:46, armansito wrote: > It's a bit strange that the endpoint service provider invalidates the transport > rather than the other way around. Right. Removed the test for this function, since ClearConfiguration will be called in one of the following cases: (1) Directly call SetValid on the transport object. (2) Set the media object to be invisible. This will invalidate all transport objects. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:86: // since user applications can be notified of an error returned by the call. On 2015/02/19 00:21:45, Ben Chan wrote: > is this comment still valid? > > "since user applications can be notified of an error returned by the call" Recovered the error callback, so the the comment stays valid. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:87: virtual void Unregister(const base::Closure& callback) = 0; On 2015/02/19 00:37:46, armansito wrote: > Why did you remove the error callback? Also, without the error callback, the > comment above doesn't make much sense, since you can't return an error. Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:39: volume_(nullptr), On 2015/02/19 00:21:45, Ben Chan wrote: > no need to initialize scoped_ptr to nullptr > > what's the purpose of this change? Removed. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:66: media_endpoint_.get() != nullptr) { On 2015/02/19 00:37:46, armansito wrote: > nit: No need to compare against nullptr, the pointer itself will evaluate to > true of false based on its value. Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:83: void BluetoothAudioSinkChromeOS::Unregister(const base::Closure& callback) { On 2015/02/19 00:37:46, armansito wrote: > Note that if BluetoothAudioSinkChromeOS outlives DBusThreadManager, the code > below will crash. You should add a TODO here saying that we should provide this > guarantee in the future. Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:115: return volume_.get() != nullptr ? *volume_ : 0; On 2015/02/19 00:37:46, armansito wrote: > nit: no need for '!= nullptr' Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:142: StateChanged(BluetoothAudioSink::STATE_DISCONNECTED); On 2015/02/19 00:21:45, Ben Chan wrote: > this may be simpler: > > if (state_ != BluetoothAudioSink::STATE_INVALID) > StateChanged(BluetoothAudioSink::STATE_DISCONNECTED); Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:206: if (properties.volume.get() != nullptr) { On 2015/02/19 00:37:46, armansito wrote: > nit: no need for '!= nullptr' Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:207: volume_.reset(new uint16_t()); On 2015/02/19 00:37:46, armansito wrote: > Why not make this assignment from VolumeChanged instead of here? Removed the allocation of memory here. Let VolumeChanged deal with it. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:287: case BluetoothAudioSink::STATE_INVALID: { On 2015/02/19 00:37:46, armansito wrote: > All of these added scopes are unnecessary. I would remove them. Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:292: if (state_ != BluetoothAudioSink::STATE_DISCONNECTED) On 2015/02/19 00:37:46, armansito wrote: > Kind of unnecessary I think, I'd just make ResetTransport not do anything if it > doesn't have anything to clean up. > > So, remove the break and the if statement and just fall through to the next > case. Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:327: if (volume_.get() == nullptr) On 2015/02/19 00:37:46, armansito wrote: > nit: if (!volume_.get()) > > That said, your code looks a bit odd. You're initializing volume_ both here and > outside, in addition you have this weird check, I'd change this so that you just > initialize it here if volume_ is NULL: > > if (!volume_.get()) > volume_.reset(new...); > Removed. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:331: FOR_EACH_OBSERVER(BluetoothAudioSink::Observer, observers_, Check whether the new volume before notifying observers. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:384: VolumeChanged(0); On 2015/02/19 00:37:46, armansito wrote: > So did volume just become 0 or are you cleaning up the volume in general? Once the transport is invalid, user applications should get notified first, and then we reset the |volume_|. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:385: volume_ = 0; On 2015/02/19 00:21:45, Ben Chan wrote: > volume_.reset()? Done. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:42: void Unregister(const base::Closure& callback) override; On 2015/02/19 00:21:45, Ben Chan wrote: > comment needs updated Recovered the error callback, so the the comment stays valid. https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/939753004/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:517: TEST_F(BluetoothAudioSinkChromeOSTest, EndpointReleased) { Removed this test. This can be included in UnregisterAudioSinkDuringIdleState test. Released() will be called in the following cases: (1) UnregisterEndpoint gets called. (2) Set the media object to be invisible.
https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:102: while (endpoints_.begin() != endpoints_.end()) { nit: no need for curly braces. https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:102: while (endpoints_.begin() != endpoints_.end()) { Please describe in a comment why this code works. It may not be clear to the reader that SetEndpointRegistered removes the endpoint entry from |endpoints_|. https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (left): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:28: "/fake/hci0/dev_00_00_00_00_00_00"; nit: How about "*/fake_audio_source"? Also, can this be something other than hci0? https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:28: path << "fake/hci0/dev_00_00_00_00_00_00/fd" << sequence_number; nit: if you want to truly simulate bluez's behavior then you could actually pass the adapter path as the prefix (as in if the adapter is hci1, this should also start with hci1). I guess the dev_00* part doesn't really matter but I'd put something like "*/fake_audio_source/fd" as the device path instead of all 0s for the address. https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.h (right): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:74: dbus::ObjectPath GetTransportPath(dbus::ObjectPath endpoint_path); nit: Change argument type to "const dbus::ObjectPath&". Here and everywhere else where you pass an object path as argument. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:43: ERROR_INVALID_ADAPTER, // BluetoothAdapter not presented/powered. nit: s/presented/present/ https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:67: base::Unretained(this))); You wouldn't need to pass an unretained pointer if you just made UnregisterErrorCallback a file-local function (i.e. in an anonymous namespace). https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:88: // Gets Media object exported by D-Bus and unregisters the endpoint. nit: probably file a bug for this and refer to the bug number from here. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:119: return volume_.get() ? *volume_ : 0; Document at the API definition that GetVolume() returns 0 if the volume is unknown. Now, that you're actually returning 0 if the volume is unknown, do you need to store it as a scoped_ptr? https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:142: // If true, the previous transport has been deprecated, so the |state_| has to "deprecated" is not the correct word here, maybe "torn down"? https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:319: if (volume_.get() && volume == *volume_) You could probably check "if (GetVolume() == volume)" here, since from a public interface standpoint, you're returning 0 anyway. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:322: volume_.reset(new uint16_t(volume)); You should only reset here if volume_ is NULL, to avoid unnecessary realloc. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:117: device::BluetoothAudioSink::ErrorCode error_code); if this is a dummy then why does it need to be a class method?
https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_client.cc (right): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_client.cc:102: while (endpoints_.begin() != endpoints_.end()) { On 2015/02/23 22:07:06, armansito wrote: > Please describe in a comment why this code works. It may not be clear to the > reader that SetEndpointRegistered removes the endpoint entry from |endpoints_|. Done. https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (left): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:28: "/fake/hci0/dev_00_00_00_00_00_00"; On 2015/02/23 22:07:06, armansito wrote: > nit: How about "*/fake_audio_source"? Also, can this be something other than > hci0? Done. https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:28: path << "fake/hci0/dev_00_00_00_00_00_00/fd" << sequence_number; On 2015/02/23 22:07:06, armansito wrote: > nit: if you want to truly simulate bluez's behavior then you could actually pass > the adapter path as the prefix (as in if the adapter is hci1, this should also > start with hci1). I guess the dev_00* part doesn't really matter but I'd put > something like "*/fake_audio_source/fd" as the device path instead of all 0s for > the address. Referred to chromeos::FakeBluetoothAdapterClient::kAdapterPath for now. https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.h (right): https://codereview.chromium.org/939753004/diff/20001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:74: dbus::ObjectPath GetTransportPath(dbus::ObjectPath endpoint_path); On 2015/02/23 22:07:06, armansito wrote: > nit: Change argument type to "const dbus::ObjectPath&". Here and everywhere else > where you pass an object path as argument. Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:43: ERROR_INVALID_ADAPTER, // BluetoothAdapter not presented/powered. On 2015/02/23 22:07:06, armansito wrote: > nit: s/presented/present/ Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:67: base::Unretained(this))); On 2015/02/23 22:07:06, armansito wrote: > You wouldn't need to pass an unretained pointer if you just made > UnregisterErrorCallback a file-local function (i.e. in an anonymous namespace). Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:88: // Gets Media object exported by D-Bus and unregisters the endpoint. On 2015/02/23 22:07:06, armansito wrote: > nit: probably file a bug for this and refer to the bug number from here. Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:119: return volume_.get() ? *volume_ : 0; On 2015/02/23 22:07:06, armansito wrote: > Document at the API definition that GetVolume() returns 0 if the volume is > unknown. > > Now, that you're actually returning 0 if the volume is unknown, do you need to > store it as a scoped_ptr? Changed |volume_| back to uint16_t. Possible values of a volume are 0-127, so I use 128 to represent invalid volume. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:142: // If true, the previous transport has been deprecated, so the |state_| has to On 2015/02/23 22:07:06, armansito wrote: > "deprecated" is not the correct word here, maybe "torn down"? Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:319: if (volume_.get() && volume == *volume_) On 2015/02/23 22:07:06, armansito wrote: > You could probably check "if (GetVolume() == volume)" here, since from a public > interface standpoint, you're returning 0 anyway. Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:322: volume_.reset(new uint16_t(volume)); On 2015/02/23 22:07:06, armansito wrote: > You should only reset here if volume_ is NULL, to avoid unnecessary realloc. Done. https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/939753004/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:117: device::BluetoothAudioSink::ErrorCode error_code); On 2015/02/23 22:07:06, armansito wrote: > if this is a dummy then why does it need to be a class method? Moved it to anonymous namespace.
https://codereview.chromium.org/939753004/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:30: << "/fake_audio_source/fd" << sequence_number; nit: use chromeos::FakeBluetoothAdapterClient::kTransportDevicePath here. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:24: const uint16_t kInvalidVolume = 128; nit: I would move this to BluetoothAudioSink. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:95: error_callback.Run(BluetoothAudioSink::ERROR_NOT_UNREGISTERED); Did this appear after a rebase? You have to return here after invoking |error_callback|. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:331: volume_ = (volume >= 0 && volume <= 127) ? volume : 128; nit: s/128/kInvalidVolume/. Also |volume| is unsigned so it can't ever be less than 0. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:50: // are 0-128, where 0-127 are valid volumes and 128 represents invalid volume. nit: I would define a constant for invalid volume in bluetooth_audio_sink then and return that. Also, this comment needs to go to bluetooth_audio_sink.h where the API is defined.
https://codereview.chromium.org/939753004/diff/40001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/40001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:30: << "/fake_audio_source/fd" << sequence_number; On 2015/02/24 01:38:05, armansito wrote: > nit: use chromeos::FakeBluetoothAdapterClient::kTransportDevicePath here. Done. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:24: const uint16_t kInvalidVolume = 128; On 2015/02/24 01:38:06, armansito wrote: > nit: I would move this to BluetoothAudioSink. Moved this to BluetoothAudioSink as a static const. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:95: error_callback.Run(BluetoothAudioSink::ERROR_NOT_UNREGISTERED); On 2015/02/24 01:38:06, armansito wrote: > Did this appear after a rebase? You have to return here after invoking > |error_callback|. Done. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:331: volume_ = (volume >= 0 && volume <= 127) ? volume : 128; On 2015/02/24 01:38:06, armansito wrote: > nit: s/128/kInvalidVolume/. Also |volume| is unsigned so it can't ever be less > than 0. Done. https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/939753004/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:50: // are 0-128, where 0-127 are valid volumes and 128 represents invalid volume. On 2015/02/24 01:38:06, armansito wrote: > nit: I would define a constant for invalid volume in bluetooth_audio_sink then > and return that. Also, this comment needs to go to bluetooth_audio_sink.h where > the API is defined. Moved kInvalidVolume to BluetoothAudioSink and updated the comment of GetVolume().
lgtm with nits https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:105: // are 0-128, where 0-127 are valid volumes and 128 represents invalid volume. nit: Say that if volume is unknown, "kInvalidVolume" is returned instead of "128" so that callers compare against the constant and not directly against 128. https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:329: volume_ = volume < BluetoothAudioSink::kInvalidVolume Can |volume| ever be larger than 128? I would just directly assign add this check right before: DCHECK(volume <= BluetoothAudioSink::kInvalidVolume). Of course it makes sense to guard against the invalid behavior of an external process (i.e. BlueZ), so if you want to keep your conditional and not have the DCHECK then just use std::min, which reads a bit better: volume_ = std::min(volume, BluetoothAudioSink::kInvalidVolume);
https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:105: // are 0-128, where 0-127 are valid volumes and 128 represents invalid volume. On 2015/02/24 23:42:09, armansito wrote: > nit: Say that if volume is unknown, "kInvalidVolume" is returned instead of > "128" so that callers compare against the constant and not directly against 128. Done. https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/939753004/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:329: volume_ = volume < BluetoothAudioSink::kInvalidVolume On 2015/02/24 23:42:09, armansito wrote: > Can |volume| ever be larger than 128? I would just directly assign add this > check right before: > > DCHECK(volume <= BluetoothAudioSink::kInvalidVolume). > > Of course it makes sense to guard against the invalid behavior of an external > process (i.e. BlueZ), so if you want to keep your conditional and not have the > DCHECK then just use std::min, which reads a bit better: > > volume_ = std::min(volume, BluetoothAudioSink::kInvalidVolume); Done.
not lgtm, please address my new set of comments, they are mostly nits. https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:91: FakeBluetoothMediaTransportClient::~FakeBluetoothMediaTransportClient() { You have to clean up the data in |endpoints_| here. https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.h (right): https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:9: #include <memory> why is this needed? https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:37: struct Transport { Make this a private nested-class. https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:102: std::map<dbus::ObjectPath, Transport*> endpoints_; These variable names don't exactly reflect their contents. |endpoints_| contains a mapping to transports and |transports_| contains a mapping to endpoint paths. I would rename this to make these more clear: endpoints_ -> endpoint_to_transport_map_ transports_ -> transport_to_endpoint_map_ https://codereview.chromium.org/939753004/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/939753004/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:632: // |kTransportVolumeInitial| is the initial volume of the transport, and this nit: did you mean |kTransportVolume|?
https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.cc:91: FakeBluetoothMediaTransportClient::~FakeBluetoothMediaTransportClient() { On 2015/02/26 02:27:34, armansito wrote: > You have to clean up the data in |endpoints_| here. Done. https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... File chromeos/dbus/fake_bluetooth_media_transport_client.h (right): https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:9: #include <memory> On 2015/02/26 02:27:34, armansito wrote: > why is this needed? Removed. https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:37: struct Transport { On 2015/02/26 02:27:34, armansito wrote: > Make this a private nested-class. Done. https://codereview.chromium.org/939753004/diff/80001/chromeos/dbus/fake_bluet... chromeos/dbus/fake_bluetooth_media_transport_client.h:102: std::map<dbus::ObjectPath, Transport*> endpoints_; On 2015/02/26 02:27:34, armansito wrote: > These variable names don't exactly reflect their contents. |endpoints_| contains > a mapping to transports and |transports_| contains a mapping to endpoint paths. > I would rename this to make these more clear: > > endpoints_ -> endpoint_to_transport_map_ > transports_ -> transport_to_endpoint_map_ Done. https://codereview.chromium.org/939753004/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc (right): https://codereview.chromium.org/939753004/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:600: // Changes volume to a valid level. Update the comment. https://codereview.chromium.org/939753004/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:632: // |kTransportVolumeInitial| is the initial volume of the transport, and this On 2015/02/26 02:27:34, armansito wrote: > nit: did you mean |kTransportVolume|? Yes.
lgtm with nits https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_media_transport_client.cc:95: delete it->second; for (auto& it : endpoint_to_transport_map_) delete it.second; https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_media_transport_client.cc:117: if (endpoint_path.value() == "" || Replace 'endpoint_path.value() == ""' with endpoint_path.value().empty() or even better: !endpoint_path.IsValid() https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_media_transport_client.h (right): https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_media_transport_client.h:36: class Transport { Make this class private. As in, move it below the "private:" directive within the parent class: class FakeBluetoothMediaTransportClient { public: ... private: ... class Transport { ... }; ... }; Also add a description of what this class does right above it.
https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right): https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_media_transport_client.cc:95: delete it->second; On 2015/02/26 04:23:08, armansito wrote: > for (auto& it : endpoint_to_transport_map_) > delete it.second; That's a neat one. https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_media_transport_client.cc:117: if (endpoint_path.value() == "" || On 2015/02/26 04:23:08, armansito wrote: > Replace 'endpoint_path.value() == ""' with > > endpoint_path.value().empty() > > or even better: > > !endpoint_path.IsValid() Done. https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... File chromeos/dbus/fake_bluetooth_media_transport_client.h (right): https://codereview.chromium.org/939753004/diff/100001/chromeos/dbus/fake_blue... chromeos/dbus/fake_bluetooth_media_transport_client.h:36: class Transport { On 2015/02/26 04:23:08, armansito wrote: > Make this class private. As in, move it below the "private:" directive within > the parent class: > > class FakeBluetoothMediaTransportClient { > public: > ... > private: > ... > class Transport { > ... > }; > ... > }; > > Also add a description of what this class does right above it. Done.
The CQ bit was checked by mcchou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from armansito@chromium.org Link to the patchset: https://codereview.chromium.org/939753004/#ps120001 (title: " ")
The CQ bit was unchecked by mcchou@chromium.org
The CQ bit was checked by mcchou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/939753004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4a0cac258fddaafd18cdcd0cea53121826ab4c3a Cr-Commit-Position: refs/heads/master@{#318266} |
