Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
787743002:
device/bluetooth: Add BluetoothAudioSinkChromeOS. (Closed)
Descriptiondevice/bluetooth: Add BluetoothAudioSinkChromeOS.
This CL implements Bluetooth Audio Sink API for ChromeOS platform,
where user applications can register/unregister a BT audio sink
via BlueAdapter interface. This API acts as a middle layer hiding
the components of BlueZ/D-Bus.
BUG=441581
Committed: https://crrev.com/6ad5832b96d497beb736e521b24f698ee74c592c
Cr-Commit-Position: refs/heads/master@{#312562}
Patch Set 1 #
Total comments: 40
Patch Set 2 : #
Total comments: 6
Patch Set 3 : Change copyright year and add some functions. #
Total comments: 8
Patch Set 4 : Fix nits. #
Total comments: 14
Patch Set 5 : Remove all OnAction callbacks and fill in GeneratePath function. #
Total comments: 12
Patch Set 6 : Make GenerateEndpointPath() static. #
Total comments: 6
Patch Set 7 : Move the declaration/definition ofsequence_number into GenerateEndpointPath(). #
Total comments: 2
Patch Set 8 : Remove GenerateEndpointPath(). #
Total comments: 2
Patch Set 9 : #Patch Set 10 : Remove unused variable. #
Total comments: 8
Patch Set 11 : Fix nits. #
Messages
Total messages: 39 (7 generated)
mcchou@chromium.org changed reviewers: + armansito@chromium.org, benchan@chromium.org
https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:5: #include "device/bluetooth/bluetooth_audio_sink.h" You don't need to include this, bluetooth_audio_sink_chromeos.h already does. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:33: // If |persent| is true, change state to |STATE_DISCONNECTED| and call Precede all of your comments below with a TODO and refer to the bug number that handles filling in these details. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:113: VLOG(1) << "Bluetooth audio sink state changed"; Log the state? https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:114: state_ = state; I'd notify the observers only if the state actually changed. So do an early return if the new state == old state. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:116: BluetoothAudioSinkStateChanged(this, state_)); I'd notify the observers only if the state actually changed. So do an early return if the new state == old state. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:122: VLOG(1) << "Bluetooth audio sink volume changed"; Log the volume? https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:123: volume_ = volume; I'd notify the observers only if the volume actually changed. So do an early return if the new volume == old volume. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:129: : state_(device::BluetoothAudioSink::STATE_INVALID), volume_(0) { nit: each variable in its own line. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:133: } Move the declarations of ctor/dtor before all the methods. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:61: protected: Do these need to be protected? I'd just make Register public. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:67: const device::BluetoothAudioSink::AudioSinkAcquiredCallback& callback, This probably doesn't need to use this callback. I would instead define |callback| as base::Closure. When you call this in BluetoothAdapterChromeOS, you can easily bind your BluetoothAudioSinkChromeOS pointer as an argument to the callback. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:73: virtual void Unregister( Will this be an override of BluetoothAudioSink::Unregister? I'd just define it correctly here. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:84: uint16_t volume); Make these two methods private (StateChanged & VolumeChanged). https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:87: friend class BluetoothAdapterChromeOS; Is the friendship necessary here? You could probably just make the constructor public since these are all internal classes anyway.
https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:89: BluetoothAudioSinkChromeOS(); The constructor should probably take in a pointer to BluetoothAdapterChromeOS, since this object will need to observe events from it and needs to obtain its object path.
https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:67: const device::BluetoothAudioSink::AudioSinkAcquiredCallback& callback, The name of this callback is too verbose, I would change it to BluetoothAudioSink::AcquiredCallback.
https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:10: void BluetoothAudioSinkChromeOS::AddObserver( nit: try to keep the method definitions and declarations in the same order https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:65: virtual void Register( these methods don't need to be virtual https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:90: virtual ~BluetoothAudioSinkChromeOS(); ~BluetoothAudioSinkChromeOS() override; https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:93: // device; nit: ;->. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:99: // Object Path of the media object being used. nit: Path -> path
In your commit, change: BT -> Bluetooth BlueAdapter -> BluetoothAdapter
https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:5: #include "device/bluetooth/bluetooth_audio_sink.h" On 2014/12/09 00:30:28, armansito wrote: > You don't need to include this, bluetooth_audio_sink_chromeos.h already does. Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:33: // If |persent| is true, change state to |STATE_DISCONNECTED| and call On 2014/12/09 00:30:28, armansito wrote: > Precede all of your comments below with a TODO and refer to the bug number that > handles filling in these details. Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:113: VLOG(1) << "Bluetooth audio sink state changed"; On 2014/12/09 00:30:28, armansito wrote: > Log the state? Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:114: state_ = state; On 2014/12/09 00:30:28, armansito wrote: > I'd notify the observers only if the state actually changed. So do an early > return if the new state == old state. Will put a DCHECK_NQ here before notifying observers. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:122: VLOG(1) << "Bluetooth audio sink volume changed"; On 2014/12/09 00:30:28, armansito wrote: > Log the volume? Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:123: volume_ = volume; On 2014/12/09 00:30:28, armansito wrote: > I'd notify the observers only if the volume actually changed. So do an early > return if the new volume == old volume. The same as StateChanged. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:129: : state_(device::BluetoothAudioSink::STATE_INVALID), volume_(0) { On 2014/12/09 00:30:28, armansito wrote: > nit: each variable in its own line. Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.cc:133: } On 2014/12/09 00:30:28, armansito wrote: > Move the declarations of ctor/dtor before all the methods. Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:61: protected: On 2014/12/09 00:30:28, armansito wrote: > Do these need to be protected? I'd just make Register public. Done. User applications should have no access to BluetoothAudioSinkChromeOS::Register. As long as user applications don't downcast the pointer, we shall be fine. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:65: virtual void Register( On 2014/12/09 15:28:37, Ben Chan wrote: > these methods don't need to be virtual Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:67: const device::BluetoothAudioSink::AudioSinkAcquiredCallback& callback, On 2014/12/09 13:30:18, armansito wrote: > The name of this callback is too verbose, I would change it to > BluetoothAudioSink::AcquiredCallback. Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:67: const device::BluetoothAudioSink::AudioSinkAcquiredCallback& callback, On 2014/12/09 00:30:28, armansito wrote: > This probably doesn't need to use this callback. I would instead define > |callback| as base::Closure. When you call this in BluetoothAdapterChromeOS, you > can easily bind your BluetoothAudioSinkChromeOS pointer as an argument to the > callback. Added a TODO in BluetoothAudioChromeOS. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:73: virtual void Unregister( On 2014/12/09 00:30:28, armansito wrote: > Will this be an override of BluetoothAudioSink::Unregister? I'd just define it > correctly here. It's not an override of BluetoothAudioSink::Unregister which has been removed. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:84: uint16_t volume); On 2014/12/09 00:30:28, armansito wrote: > Make these two methods private (StateChanged & VolumeChanged). Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:87: friend class BluetoothAdapterChromeOS; On 2014/12/09 00:30:28, armansito wrote: > Is the friendship necessary here? You could probably just make the constructor > public since these are all internal classes anyway. Removed the friendship and made constructor public. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:89: BluetoothAudioSinkChromeOS(); On 2014/12/09 00:31:20, armansito wrote: > The constructor should probably take in a pointer to BluetoothAdapterChromeOS, > since this object will need to observe events from it and needs to obtain its > object path. Done. The original idea is that BluetoothAdapterChromeOS will call BluetoothAudioSinkChromeOS::AddObserver to add itself as an observer, but adding/removing it from the observer list won't be clear in this case. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:90: virtual ~BluetoothAudioSinkChromeOS(); On 2014/12/09 15:28:37, Ben Chan wrote: > ~BluetoothAudioSinkChromeOS() override; Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:93: // device; On 2014/12/09 15:28:38, Ben Chan wrote: > nit: ;->. Done. https://codereview.chromium.org/787743002/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink_chromeos.h:99: // Object Path of the media object being used. On 2014/12/09 15:28:38, Ben Chan wrote: > nit: Path -> path Done.
I would change the commit message and title to say something like "Add BluetoothAudioSinkChromeOS" since most of the methods are unimplemented. https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:77: const device::BluetoothAudioSink::ErrorCallback& error_callback); This will need to be "override"
https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: This should be 2015 now ;) https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 now.
https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/07 01:13:34, armansito wrote: > nit: This should be 2015 now ;) Done. https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/07 01:13:34, armansito wrote: > nit: 2015 now. Done. https://codereview.chromium.org/787743002/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:77: const device::BluetoothAudioSink::ErrorCallback& error_callback); On 2014/12/16 02:01:41, armansito wrote: > This will need to be "override" Done.
https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:73: // successfully registered, otherwise |error_callback| will be called. Can you add a note here at the end saying "Called from BluetoothAdapterChromeOS." https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:74: virtual void Register( nit: This probably doesn't need to be virtual. https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:112: const std::string& error_message); nit: Remove all of the On* methods, since these aren't being used right now. You generally shouldn't include any unused methods unless they are part of the class' public interface.
https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/17 02:09:06, armansito wrote: > nit: 2015 Done. https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:73: // successfully registered, otherwise |error_callback| will be called. On 2015/01/17 02:09:06, armansito wrote: > Can you add a note here at the end saying "Called from > BluetoothAdapterChromeOS." Done. https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:74: virtual void Register( On 2015/01/17 02:09:06, armansito wrote: > nit: This probably doesn't need to be virtual. Done. https://codereview.chromium.org/787743002/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:112: const std::string& error_message); On 2015/01/17 02:09:06, armansito wrote: > nit: Remove all of the On* methods, since these aren't being used right now. You > generally shouldn't include any unused methods unless they are part of the > class' public interface. Done.
lgtm
The CQ bit was checked by armansito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787743002/60001
https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:15: adapter_(adapter) { initialize all POD member variables, e.g. |read_mtu_|, |write_mtu_|, |present_|. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:176: const uint16_t read_mtu, 'const' isn't necessary for |read_mtu| and |write_mtu|. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:179: // Set |fd_|, |read_mtu_| and |write_mtu_| if media transport is acquired when you later assign |fd| to |fd_|, you should probably transfer the ownership to this class so that no one else is going to close the |fd| while this class is using it. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:198: DCHECK_NE(fd_.value(), -1); Note: If dbus::FileDescriptor defines a kInvalidFD value, you should probably use that instead of -1. But since dbus::FileDescriptor doesn't, it's probably fine to use -1 here. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:36: static uint16_t sequence_number; this should be initialized properly https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:136: BluetoothAdapterChromeOS* adapter_; I believe you expect |adapter_| outlives this object (as otherwise that would be an issue in the destructor of this class), you should mention this assumption in the comment. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:146: ObserverList<BluetoothAudioSink::Observer> observers_; like |adapter|, mention that objects in |observers_| are expected to outlive this object.
The CQ bit was unchecked by benchan@chromium.org
The CQ bit was unchecked by mcchou@chromium.org
https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:15: adapter_(adapter) { On 2015/01/20 19:29:08, Ben Chan wrote: > initialize all POD member variables, e.g. |read_mtu_|, |write_mtu_|, |present_|. Done. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:176: const uint16_t read_mtu, On 2015/01/20 19:29:08, Ben Chan wrote: > 'const' isn't necessary for |read_mtu| and |write_mtu|. Removed. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:179: // Set |fd_|, |read_mtu_| and |write_mtu_| if media transport is acquired On 2015/01/20 19:29:08, Ben Chan wrote: > when you later assign |fd| to |fd_|, you should probably transfer the ownership > to this class so that no one else is going to close the |fd| while this class is > using it. Done. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:198: DCHECK_NE(fd_.value(), -1); On 2015/01/20 19:29:07, Ben Chan wrote: > Note: If dbus::FileDescriptor defines a kInvalidFD value, you should probably > use that instead of -1. But since dbus::FileDescriptor doesn't, it's probably > fine to use -1 here. It looks like there is no constant value defined for invalid value, so we keep -1 for now. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:36: static uint16_t sequence_number; On 2015/01/20 19:29:08, Ben Chan wrote: > this should be initialized properly Done. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:136: BluetoothAdapterChromeOS* adapter_; On 2015/01/20 19:29:08, Ben Chan wrote: > I believe you expect |adapter_| outlives this object (as otherwise that would be > an issue in the destructor of this class), you should mention this assumption in > the comment. Done. https://codereview.chromium.org/787743002/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:146: ObserverList<BluetoothAudioSink::Observer> observers_; On 2015/01/20 19:29:08, Ben Chan wrote: > like |adapter|, mention that objects in |observers_| are expected to outlive > this object. Done.
https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:177: if (sequence_number == std::numeric_limits<uint16_t>::max()) { this function should be static. also move the declaration and definition of |sequence_number| here. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:180: ++sequence_number; ++sequence_number is sufficient as it will wrap around when it hits the max. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:146: // List of observers interested in event notifications from us. |observers_| objects in |observers_|
https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:181: } This is unnecessary, since unsigned integer overflow has a defined behavior. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:36: static uint16_t sequence_number; Why 16-bit? I'd just make this unsigned int, which will be large enough. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:98: dbus::ObjectPath GenerateEndpointPath(); this looks like this should be a static method.
https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:177: if (sequence_number == std::numeric_limits<uint16_t>::max()) { On 2015/01/20 22:46:51, Ben Chan wrote: > this function should be static. > > also move the declaration and definition of |sequence_number| here. Done. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:180: ++sequence_number; On 2015/01/20 22:46:51, Ben Chan wrote: > ++sequence_number is sufficient as it will wrap around when it hits the max. Done. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.cc:181: } On 2015/01/20 22:47:49, armansito wrote: > This is unnecessary, since unsigned integer overflow has a defined behavior. Done. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:36: static uint16_t sequence_number; On 2015/01/20 22:47:49, armansito wrote: > Why 16-bit? I'd just make this unsigned int, which will be large enough. Done. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:98: dbus::ObjectPath GenerateEndpointPath(); On 2015/01/20 22:47:49, armansito wrote: > this looks like this should be a static method. Done. https://codereview.chromium.org/787743002/diff/80001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink_chromeos.h:146: // List of observers interested in event notifications from us. |observers_| On 2015/01/20 22:46:51, Ben Chan wrote: > objects in |observers_| Done.
https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:44: unsigned int BluetoothAudioSinkChromeOS::sequence_number = 0; nit: Add "// static" before assignment. https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:46: dbus::ObjectPath BluetoothAudioSinkChromeOS::GenerateEndpointPath() { nit: Add "// static" before function definition. https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:47: ++sequence_number; Paths start at 1 then? Just making sure that this will never use "0" as a number unless it overflows. https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:47: ++sequence_number; Also add a note here saying why it's OK for this to overflow. Basically, exporting an object with an existing path will fail at that time and that we don't care about finding unused ones since the maximum number is sufficiently large. https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.h:37: static dbus::ObjectPath GenerateEndpointPath(); nit: I'd make this private, since this will only be used internally. https://codereview.chromium.org/787743002/diff/100001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.h:106: static unsigned int sequence_number; nit: Underscore at the end (i.e. |sequence_number_|) according to style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names
https://codereview.chromium.org/787743002/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:45: unsigned int sequence_number = 0; Won't this method now only generate /blabla/endpoint1 as the path?
not lgtm (might as well), since the code broke after my first approval
https://codereview.chromium.org/787743002/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:45: unsigned int sequence_number = 0; On 2015/01/21 00:27:20, armansito wrote: > Won't this method now only generate /blabla/endpoint1 as the path? Will remove this method for now and add it as a helper function in anonymous namespace in the next CL.
https://codereview.chromium.org/787743002/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:139: void Register( BluetoothAudioSinkChromeOS:: https://codereview.chromium.org/787743002/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:149: void Unregister( BluetoothAudioSinkChromeOS::
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/787743002/180001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: armansito@chromium.org. Please make sure to get positive review before another CQ attempt.
lgtm with nits https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:78: } nit: If this is not doing anything, you can probably just not override it at all. https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:90: } nit: If this is not doing anything, you can probably just not override it at all. https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:167: DCHECK_NE(fd_.value(), -1); nit: A more robust check would be DCHECK_GE(fd_.value(), 0);
https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right): https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:78: } On 2015/01/22 03:37:42, armansito wrote: > nit: If this is not doing anything, you can probably just not override it at > all. Removed. https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:90: } On 2015/01/22 03:37:42, armansito wrote: > nit: If this is not doing anything, you can probably just not override it at > all. Removed. https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.cc:167: DCHECK_NE(fd_.value(), -1); On 2015/01/22 03:37:42, armansito wrote: > nit: A more robust check would be DCHECK_GE(fd_.value(), 0); Done. https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink_chromeos.h (right): https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.h:49: void MediaAdded(const dbus::ObjectPath& object_path) override; Removed. https://codereview.chromium.org/787743002/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink_chromeos.h:53: void MediaTransportAdded(const dbus::ObjectPath& object_path) override; Removed.
The CQ bit was checked by benchan@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/787743002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6ad5832b96d497beb736e521b24f698ee74c592c Cr-Commit-Position: refs/heads/master@{#312562} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
