Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
768493006:
device/bluetooth: Add Bluetooth Audio Sink base class. (Closed)
Descriptiondevice/bluetooth: Add Bluetooth Audio Sink base class.
This CL adds the base class for Bluetooth Audio Sink.
BUG=429016
Committed: https://crrev.com/f9c8af1a6d4b00b558649095d5589765224bc466
Cr-Commit-Position: refs/heads/master@{#307142}
Patch Set 1 #
Total comments: 44
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #
Total comments: 14
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 34 (11 generated)
mcchou@chromium.org changed reviewers: + armansito@chromium.org, benchan@chromium.org
https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.cc (left): https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.cc:13: } You'll need to provide definitions for AudioSinkOptions ctor/dtor here as well. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:14: #include "dbus/file_descriptor.h" We can't have dbus includes here. The code wouldn't compile on windows/mac. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:20: // BluetoothAudioSink represents a local audio sink where a remote device can In general you shouldn't refer to any BlueZ concepts here. What your class represents is really a "Bluetooth A2DP Sink implementation". You should document this class with that high-level description and perhaps go into a little more detail but refrain from mentioning endpoints, media transports, etc that are BlueZ API concepts. Though it's OK to use A2DP terminology in general. Basically present this class to your users as a way to act as an A2DP sink. How it internally does it is not something that your users should need to know. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:23: // which attaches to that endpoint. I wouldn't go into BlueZ API details here. This is meant to be a cross-platform API, so these concepts won't apply to Windows/Mac (even if we don't ever provide an implementation for those platforms, we should keep this generic). https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:24: // An user applications can obtain a pointer to a BluetoothAudioSink object via "An user applications" -> "User applications" ? https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:26: // BluetoothAudioSink's endopint object depends on whether BluetoothAdapter is s/endopint/endpoint/ Though I wouldn't mention endpoints here. Just say "The validity of a BluetoothAudioSink depends on ..." https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:29: // the remote device. Get rid of the last sentence. These are implementation details that aren't relevant to the user. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:36: // BluetoothAdapter object's changes. You don't need the second sentence. Just saying "Possible Audio Sink states" is enough for this enum. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:45: // Used to configure an BT Media Endpoint. Don't mention anything about BlueZ APIs. Just say "Options to configure an A2DP audio sink". https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:46: struct AudioSinkOptions { I'd just call this "struct Options". BluetoothAudioSink::AudioSinkOptions is a bit wordy. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:51: scoped_ptr<std::vector<uint8_t>> capabilities; Aren't codec and capabilities mandatory? I would remove the scoped_ptr's here and set them to a meaningful default value in the constructor. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:54: // Interface for observing changes from Bluetooth Audio Sinks. from a BluetoothAudioSink https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:64: const BluetoothAudioSink::AudioSinkState& audio_sink_state) = 0; Pass enum by value. No need for a const reference since enum is really just an int. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:77: const dbus::FileDescriptor& fd, Don't expose dbus objects here since the whole point of this base class is to abstract away platform details. Find a more suitable type to represent the file descriptor, probably your own socket abstraction that works with base::IOBuffer. You should probably sort this out with Ben. If you have your own socket abstraction then you could also encapsulate the MTUs as part of its public interface. Just a thought. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:82: // The EndpointAcuiredCallback is used to pass the scoped_refptr of Acuired -> Acquired Also you called it EndpointRegisteredCallback. So you should probably correct the comment. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:83: // a BluetoothAudioSink object while a BT Media Endpoint is successfully Don't mention BlueZ concepts. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:86: scoped_refptr<BluetoothAudioSink>)> EndpointRegisteredCallback; I would call this "AudioSinkRegisteredCallback" since you're returning a BluetoothAudioSink. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:88: // The EndpointDeletedCallback is used to handle the clean-up while a BT Media EndpointUnregisteredCallback? https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:90: typedef base::Callback<void()> EndpointUnregisteredCallback; This typedef is unnecessary since you're method can just accept a base::Closure instead. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:93: // can fail. "in which case it can fail" -> "in which case it is called" https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:110: const ErrorCallback& error_callback) = 0; I thought we were going to have this method in BluetoothAdapter (which is what you say in the top-level comment above). It also doesn't make sense to have a RegisterEndpoint instance method in BluetoothAudioSink that returns a new instance of a BluetoothAudioSink... https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:115: virtual void UnregisterEndpoint(const EndpointUnregisteredCallback& callback, I think just base::Closure is sufficient. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:115: virtual void UnregisterEndpoint(const EndpointUnregisteredCallback& callback, Should we call this method just "Unregister"? I don't think we should mention BlueZ concepts here.
https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.cc (left): https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.cc:13: } On 2014/12/02 19:20:19, armansito wrote: > You'll need to provide definitions for AudioSinkOptions ctor/dtor here as well. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:20: // BluetoothAudioSink represents a local audio sink where a remote device can On 2014/12/02 19:20:19, armansito wrote: > In general you shouldn't refer to any BlueZ concepts here. What your class > represents is really a "Bluetooth A2DP Sink implementation". You should document > this class with that high-level description and perhaps go into a little more > detail but refrain from mentioning endpoints, media transports, etc that are > BlueZ API concepts. > > Though it's OK to use A2DP terminology in general. Basically present this class > to your users as a way to act as an A2DP sink. How it internally does it is not > something that your users should need to know. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:23: // which attaches to that endpoint. On 2014/12/02 19:20:19, armansito wrote: > I wouldn't go into BlueZ API details here. This is meant to be a cross-platform > API, so these concepts won't apply to Windows/Mac (even if we don't ever provide > an implementation for those platforms, we should keep this generic). Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:24: // An user applications can obtain a pointer to a BluetoothAudioSink object via On 2014/12/02 19:20:19, armansito wrote: > "An user applications" -> "User applications" ? Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:26: // BluetoothAudioSink's endopint object depends on whether BluetoothAdapter is On 2014/12/02 19:20:20, armansito wrote: > s/endopint/endpoint/ > > Though I wouldn't mention endpoints here. Just say "The validity of a > BluetoothAudioSink depends on ..." Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:29: // the remote device. On 2014/12/02 19:20:19, armansito wrote: > Get rid of the last sentence. These are implementation details that aren't > relevant to the user. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:36: // BluetoothAdapter object's changes. On 2014/12/02 19:20:19, armansito wrote: > You don't need the second sentence. Just saying "Possible Audio Sink states" is > enough for this enum. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:45: // Used to configure an BT Media Endpoint. On 2014/12/02 19:20:20, armansito wrote: > Don't mention anything about BlueZ APIs. Just say "Options to configure an A2DP > audio sink". Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:46: struct AudioSinkOptions { On 2014/12/02 19:20:20, armansito wrote: > I'd just call this "struct Options". BluetoothAudioSink::AudioSinkOptions is a > bit wordy. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:51: scoped_ptr<std::vector<uint8_t>> capabilities; On 2014/12/02 19:20:20, armansito wrote: > Aren't codec and capabilities mandatory? I would remove the scoped_ptr's here > and set them to a meaningful default value in the constructor. Done. Codec and capabilities are mandatory. In the case where applications may use codecs other than SBC, these options allow applications to provide their own configurations. I was thinking about making codec and capabilities NULL, and while registering an audio sink, we need to check whether codec and capabilities are NULL or not. But what you suggest is better, it is obvious that the default values should be in the constructor of Options:) https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:54: // Interface for observing changes from Bluetooth Audio Sinks. On 2014/12/02 19:20:20, armansito wrote: > from a BluetoothAudioSink Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:64: const BluetoothAudioSink::AudioSinkState& audio_sink_state) = 0; On 2014/12/02 19:20:20, armansito wrote: > Pass enum by value. No need for a const reference since enum is really just an > int. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:82: // The EndpointAcuiredCallback is used to pass the scoped_refptr of On 2014/12/02 19:20:19, armansito wrote: > Acuired -> Acquired > > Also you called it EndpointRegisteredCallback. So you should probably correct > the comment. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:83: // a BluetoothAudioSink object while a BT Media Endpoint is successfully On 2014/12/02 19:20:19, armansito wrote: > Don't mention BlueZ concepts. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:86: scoped_refptr<BluetoothAudioSink>)> EndpointRegisteredCallback; On 2014/12/02 19:20:20, armansito wrote: > I would call this "AudioSinkRegisteredCallback" since you're returning a > BluetoothAudioSink. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:88: // The EndpointDeletedCallback is used to handle the clean-up while a BT Media On 2014/12/02 19:20:20, armansito wrote: > EndpointUnregisteredCallback? Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:90: typedef base::Callback<void()> EndpointUnregisteredCallback; On 2014/12/02 19:20:20, armansito wrote: > This typedef is unnecessary since you're method can just accept a base::Closure > instead. Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:93: // can fail. On 2014/12/02 19:20:19, armansito wrote: > "in which case it can fail" -> "in which case it is called" Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:93: // can fail. On 2014/12/02 19:20:19, armansito wrote: > "in which case it can fail" -> "in which case it is called" Done. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:110: const ErrorCallback& error_callback) = 0; On 2014/12/02 19:20:19, armansito wrote: > I thought we were going to have this method in BluetoothAdapter (which is what > you say in the top-level comment above). It also doesn't make sense to have a > RegisterEndpoint instance method in BluetoothAudioSink that returns a new > instance of a BluetoothAudioSink... Sorry about the confusion, there is no doubt that RegisterEndpoint/UnregisterEndpoint will be placed in BluetoothAdapter. RegisterEndpoint and UnregisterEndpoint should be renamed, and they handle the actual works to register/unregister an audio sink. https://codereview.chromium.org/768493006/diff/1/device/bluetooth/bluetooth_a... device/bluetooth/bluetooth_audio_sink.h:115: virtual void UnregisterEndpoint(const EndpointUnregisteredCallback& callback, On 2014/12/02 19:20:20, armansito wrote: > Should we call this method just "Unregister"? I don't think we should mention > BlueZ concepts here. Done.
https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.cc (right): https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.cc:10: : codec(0x00), capabilities({0x3f, 0xff, 0x12, 0x35}) { Why this default value for capabilities specifically? You should document what these values mean here, both for the codec and capabilities. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:20: // stream audio data. Once a BluetoothAudioSink is successfully registered. Please mention A2DP here. BluetoothAudioSink specifically represents an A2DP sink. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:22: // the i[MaÂnterface provided by BluetoothAdapter. The validity of a interface? https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:28: // BluetoothAu[MaÂdioSink and the remote device. I keep seeing "[MaÂ" everywhere. Please correct them. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:29: enum AudioSinkState { nit: Like Options, I would simply call this "State" (plus, if you call it AudioSinkState, then your enums would have to look like AUDIO_SINK_STATE_INVALID etc according to the style guide). https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:70: // The AudioSinkAcquiredCallback is used to pass the scoped_refptr of nit: "...used to return a BluetoothAudioSink object..." https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:90: virtual void Register(const Options& options, Remove this method? Won't this be in BluetoothAdapter?
https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.cc (right): https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.cc:10: : codec(0x00), capabilities({0x3f, 0xff, 0x12, 0x35}) { On 2014/12/03 20:14:12, armansito wrote: > Why this default value for capabilities specifically? You should document what > these values mean here, both for the codec and capabilities. Done. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:20: // stream audio data. Once a BluetoothAudioSink is successfully registered. On 2014/12/03 20:14:12, armansito wrote: > Please mention A2DP here. BluetoothAudioSink specifically represents an A2DP > sink. Done. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:22: // the i[MaÂnterface provided by BluetoothAdapter. The validity of a On 2014/12/03 20:14:12, armansito wrote: > interface? I was using secure shell yesterday, and there were some weird bindings with up and down keys:p https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:28: // BluetoothAu[MaÂdioSink and the remote device. On 2014/12/03 20:14:12, armansito wrote: > I keep seeing "[MaÂ" everywhere. Please correct them. Done. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:29: enum AudioSinkState { On 2014/12/03 20:14:12, armansito wrote: > nit: Like Options, I would simply call this "State" (plus, if you call it > AudioSinkState, then your enums would have to look like AUDIO_SINK_STATE_INVALID > etc according to the style guide). Done. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:70: // The AudioSinkAcquiredCallback is used to pass the scoped_refptr of On 2014/12/03 20:14:12, armansito wrote: > nit: "...used to return a BluetoothAudioSink object..." Done. https://codereview.chromium.org/768493006/diff/20001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:90: virtual void Register(const Options& options, On 2014/12/03 20:14:12, armansito wrote: > Remove this method? Won't this be in BluetoothAdapter? Done.
Mostly nits. Almost there :) https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.cc (right): https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.cc:9: BluetoothAudioSink::Options::Options() { nit: Use the initializer list as you had before (codec_(0x00), ..) but maybe put the comments above the constructor? https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:20: // can stream audio data. Once a BluetoothAudioSink is successfully registered. nit: "Once a BluetoothAudioSink is successfully registered, user applications can..." https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:52: // |audio_sink| indicates the object being changed, and |audio_sink_state| nit: s/audio_sink_state/state/ https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:56: BluetoothAudioSink::State audio_sink_state) = 0; nit: s/audio_sink_state/state/ https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:59: // |audio_sink| indicates the object being changed, and |audio_sink_volume| nit: s/audio_sink_volume/volume/ https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:63: uint16_t audio_sink_volume) = 0; nit: s/audio_sink_volume/volume/ https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:86: I would add getters for volume and state: virtual State GetState() const = 0; virtual uint16_t GetVolume() const = 0;
https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.cc (right): https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.cc:9: BluetoothAudioSink::Options::Options() { On 2014/12/03 23:34:52, armansito wrote: > nit: Use the initializer list as you had before (codec_(0x00), ..) but maybe put > the comments above the constructor? Done. https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:20: // can stream audio data. Once a BluetoothAudioSink is successfully registered. On 2014/12/03 23:34:52, armansito wrote: > nit: "Once a BluetoothAudioSink is successfully registered, user applications > can..." Done. https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:52: // |audio_sink| indicates the object being changed, and |audio_sink_state| On 2014/12/03 23:34:53, armansito wrote: > nit: s/audio_sink_state/state/ Done. https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:56: BluetoothAudioSink::State audio_sink_state) = 0; On 2014/12/03 23:34:52, armansito wrote: > nit: s/audio_sink_state/state/ Done. https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:59: // |audio_sink| indicates the object being changed, and |audio_sink_volume| On 2014/12/03 23:34:53, armansito wrote: > nit: s/audio_sink_volume/volume/ Done. https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:63: uint16_t audio_sink_volume) = 0; On 2014/12/03 23:34:53, armansito wrote: > nit: s/audio_sink_volume/volume/ Done. https://codereview.chromium.org/768493006/diff/40001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:86: On 2014/12/03 23:34:53, armansito wrote: > I would add getters for volume and state: > > virtual State GetState() const = 0; > virtual uint16_t GetVolume() const = 0; Done.
lgtm with one nit. Also, you'll need to add the DEVICE_BLUETOOTH_EXPORT macro to your class declaration once this CL lands: https://codereview.chromium.org/778443002/. You may want to give scheib@ a heads up in case your CL lands before his. https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.cc (right): https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.cc:24: : codec(0x00), capabilities({0x3f, 0xff, 0x12, 0x35}) { nit: Put each member in its own line.
https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.cc (right): https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.cc:24: : codec(0x00), capabilities({0x3f, 0xff, 0x12, 0x35}) { On 2014/12/04 06:24:00, armansito wrote: > nit: Put each member in its own line. Done.
https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:17: // TODO(mcchou): Define a BluetoothAudioSink specific socket stream abstraction. socket or just I/O? you mention IOBuffer below https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:34: STATE_ACTIVE, // Co[MaÂnnected, streaming and acquired. weird characters here https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:43: std::vector<uint8_t> capabilities; you should probably include <stdint.h> for uint8_t instead of relying on the fact that it gets indirectly included by other headers.
https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:17: // TODO(mcchou): Define a BluetoothAudioSink specific socket stream abstraction. On 2014/12/05 00:24:48, Ben Chan wrote: > socket or just I/O? you mention IOBuffer below I mean I/O. https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:34: STATE_ACTIVE, // Co[MaÂnnected, streaming and acquired. On 2014/12/05 00:24:48, Ben Chan wrote: > weird characters here Done. https://codereview.chromium.org/768493006/diff/60001/device/bluetooth/bluetoo... device/bluetooth/bluetooth_audio_sink.h:43: std::vector<uint8_t> capabilities; On 2014/12/05 00:24:49, Ben Chan wrote: > you should probably include <stdint.h> for uint8_t instead of relying on the > fact that it gets indirectly included by other headers. Done.
lgtm
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/768493006/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/768493006/120001
The CQ bit was unchecked by armansito@chromium.org
https://codereview.chromium.org/768493006/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink.h:26: class BluetoothAudioSink : public base::RefCounted<BluetoothAudioSink> { Add the DEVICE_BLUETOOTH_EXPORT macro here.
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/768493006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by mcchou@chromium.org
The CQ bit was unchecked by mcchou@chromium.org
https://codereview.chromium.org/768493006/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_audio_sink.h (right): https://codereview.chromium.org/768493006/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_audio_sink.h:26: class BluetoothAudioSink : public base::RefCounted<BluetoothAudioSink> { On 2014/12/05 20:04:27, armansito wrote: > Add the DEVICE_BLUETOOTH_EXPORT macro here. Done.
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/768493006/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f9c8af1a6d4b00b558649095d5589765224bc466 Cr-Commit-Position: refs/heads/master@{#307142} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
