device/bluetooth:Implement BluetoothMediaEndpointServiceProvider delegate and media-related overrid…

device/bluetooth:Implement BluetoothMediaEndpointServiceProvider delegate and media-related overrides.

This CL implements delegate overrides for
BluetoothMediaEndpointServiceProvider and observer overrides for
BluetoothMediaClient and BluetoothMediaTransportClient. These
overrides will be called once a remote device connects to a local
A2DP audio sink, and the state of the audio sink should become idle.
The behaviors of FakeBluetoothMediaTransportClient,
FakeBluetoothMediaEndpointServiceProvider and
FakeMediaEndpointServiceProvider are also defined for
connection-related tests in this CL.

BUG=441581
TEST=device_unittests --gtest_filter=*AudioSink*

Committed: https://crrev.com/016980f10044b64144293f9e1ba0ce4c5fddeda3
Cr-Commit-Position: refs/heads/master@{#316709}

[Miao, 2015-02-09 20:33:30]

mcchou@chromium.org changed reviewers:
+ armansito@chromium.org, benchan@chromium.org

[Miao, 2015-02-09 20:34:15]

This CL is ready for review.

[armansito, 2015-02-10 00:17:02]

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:129: //
Delegate::TransportProperties structure to |delegate_|.
Some of the properties below are mandatory and some are optional. For the
mandatory ones, it might make sense to log an error if any of them is missing
and perhaps not call the delegate method at all?

Either way you need unit tests for all of these cases.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:138: } else {
nit: you can just continue with "else-if" here instead of "else { if ...}"

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:157:
dict_entry_reader.PopVariantOfUint16(properties.delay.get());
These two won't work! You're passing a NULL pointer to PopVariantOfUint16, which
will dereference the NULL pointer to assign the result and thus crash. You have
to initialize the memory here:

  properties.delay.reset(new uint16_t());
  dict_entry_reader.PopVariantOfUint16(properties.delay.get());

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:280:
TransportProperties() : delay(nullptr), volume(nullptr) {
You should assign a default value for codec and state here.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
File chromeos/dbus/bluetooth_media_transport_client.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_transport_client.cc:38: // static
Why do these need to be class-level constants?

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
File chromeos/dbus/bluetooth_media_transport_client.h (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_transport_client.h:76: // Constants used to
indicate exceptional error conditions.
All of these should go to service_constants.h. though I'm not sure why you need
to declare all of the Property constants here in the header?

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_client.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_client.cc:88: // If the media object becomes
invisible, a update chain will remove all pairs
nit: s/a update/an update/

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_client.cc:89: // in |endpoints_| and update
endpoints' visibility to the transport object.
Maybe rephrase the second part of the sentence here. Do you mean simply that it
will unregister the endpoint?

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_client.h (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_client.h:56: // Pairs of endpoint paths and
bool values indicating their visibility.
I find this confusing since you're using "visible" to mean two different things.
Doesn't this basically indicate whether or not an endpoint is registered?

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:33: if
(visible_) {
nit: Turn this around and early-return if !visible_

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:38: void
SetVisible(bool visible);
I think this is a bit strange, since this is an exported object it should always
be visible. I have a faint idea of why this might be needed but I'm getting a
bit confused by the dozen SetVisible methods that all call each other.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right):

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:160: if
(properties.state.compare(BluetoothMediaTransportClient::kStateIdle) ==
nit: You can just compare the strings with operator==

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:169: << properties.state;
All of these nested if statements are getting a bit unreadable. Turn this around
and return early.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:183: // Closes IOBuffer.
Well, currently there's no IOBuffer anywhere so this comment is somewhat
invalid.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:197: // Gets Media object,
initiates an Media Endpoint with options, and returns
nit: s/an Media/a Media/

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:199: // Media.
Note: You should avoid comments like this if all they do is explain verbatim
what the following code does and the code itself is self explanatory, unless you
have some additional insight that you think is valuable, such as side-effects,
etc.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_audio_sink_chromeos.h (right):

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.h:107: // Called while the
|state_| changes to either STATE_INVAILD or
nit: s/Called while/Called when/
nit: s/STATE_INVAILD/STATE_INVALID/

[Miao, 2015-02-10 22:15:22]

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:129: //
Delegate::TransportProperties structure to |delegate_|.
On 2015/02/10 00:17:01, armansito wrote:
> Some of the properties below are mandatory and some are optional. For the
> mandatory ones, it might make sense to log an error if any of them is missing
> and perhaps not call the delegate method at all?
> 
> Either way you need unit tests for all of these cases.

Done.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:138: } else {
On 2015/02/10 00:17:01, armansito wrote:
> nit: you can just continue with "else-if" here instead of "else { if ...}"

Done.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:157:
dict_entry_reader.PopVariantOfUint16(properties.delay.get());
On 2015/02/10 00:17:01, armansito wrote:
> These two won't work! You're passing a NULL pointer to PopVariantOfUint16,
which
> will dereference the NULL pointer to assign the result and thus crash. You
have
> to initialize the memory here:
> 
>   properties.delay.reset(new uint16_t());
>   dict_entry_reader.PopVariantOfUint16(properties.delay.get());

Done.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:280:
TransportProperties() : delay(nullptr), volume(nullptr) {
On 2015/02/10 00:17:01, armansito wrote:
> You should assign a default value for codec and state here.

Done. These default value can be used to check whether the transport property
set is initiated correctly or not.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
File chromeos/dbus/bluetooth_media_transport_client.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_transport_client.cc:38: // static
On 2015/02/10 00:17:01, armansito wrote:
> Why do these need to be class-level constants?

Moved kNoResponseError and kUnexpectedResponse back to anonymous namespace.
Since property names are used in endpoint service provider, it makes sense to me
to put property constants under transport namespace,

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
File chromeos/dbus/bluetooth_media_transport_client.h (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_transport_client.h:76: // Constants used to
indicate exceptional error conditions.
On 2015/02/10 00:17:01, armansito wrote:
> All of these should go to service_constants.h. though I'm not sure why you
need
> to declare all of the Property constants here in the header?

Comment added! Right, these will go to service_constants.h eventually, but
before that, I think it might be more centralized to put them here. In this
case, I can ensure that the reference in endpoint service provider will be
updated as well. And it makes sense to me that all property constants are under
transport namespace.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_client.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_client.cc:88: // If the media object becomes
invisible, a update chain will remove all pairs
On 2015/02/10 00:17:01, armansito wrote:
> nit: s/a update/an update/

Done.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_client.cc:89: // in |endpoints_| and update
endpoints' visibility to the transport object.
On 2015/02/10 00:17:01, armansito wrote:
> Maybe rephrase the second part of the sentence here. Do you mean simply that
it
> will unregister the endpoint?

Done.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_client.h (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_client.h:56: // Pairs of endpoint paths and
bool values indicating their visibility.
On 2015/02/10 00:17:01, armansito wrote:
> I find this confusing since you're using "visible" to mean two different
things.
> Doesn't this basically indicate whether or not an endpoint is registered?

I shouldn't use visibility here, and yes, this indicates whether an endpoint is
registered.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:33: if
(visible_) {
On 2015/02/10 00:17:01, armansito wrote:
> nit: Turn this around and early-return if !visible_

|visible_| removed.

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h (right):

https://codereview.chromium.org/910023002/diff/1/chromeos/dbus/fake_bluetooth...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:38: void
SetVisible(bool visible);
On 2015/02/10 00:17:01, armansito wrote:
> I think this is a bit strange, since this is an exported object it should
always
> be visible. I have a faint idea of why this might be needed but I'm getting a
> bit confused by the dozen SetVisible methods that all call each other.

Indeed, an endpoint is an exported object, so this method should be removed. I
was wondering if it is possible for an endpoint to be released
accidentally...but I guess testing Released() will cover this case.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_audio_sink_chromeos.cc (right):

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:160: if
(properties.state.compare(BluetoothMediaTransportClient::kStateIdle) ==
On 2015/02/10 00:17:01, armansito wrote:
> nit: You can just compare the strings with operator==

Done.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:169: << properties.state;
On 2015/02/10 00:17:02, armansito wrote:
> All of these nested if statements are getting a bit unreadable. Turn this
around
> and return early.

Done.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:183: // Closes IOBuffer.
On 2015/02/10 00:17:02, armansito wrote:
> Well, currently there's no IOBuffer anywhere so this comment is somewhat
> invalid.

Removed.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:197: // Gets Media object,
initiates an Media Endpoint with options, and returns
On 2015/02/10 00:17:02, armansito wrote:
> nit: s/an Media/a Media/

Done.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.cc:199: // Media.
On 2015/02/10 00:17:01, armansito wrote:
> Note: You should avoid comments like this if all they do is explain verbatim
> what the following code does and the code itself is self explanatory, unless
you
> have some additional insight that you think is valuable, such as side-effects,
> etc.

Done.

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_audio_sink_chromeos.h (right):

https://codereview.chromium.org/910023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_audio_sink_chromeos.h:107: // Called while the
|state_| changes to either STATE_INVAILD or
On 2015/02/10 00:17:02, armansito wrote:
> nit: s/Called while/Called when/
> nit: s/STATE_INVAILD/STATE_INVALID/

Done.

[Miao, 2015-02-10 22:49:11]

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:33: // Sets the
visibility to the transport object to true if the media object
Update comment in patch 3.

[Ben Chan, 2015-02-11 20:36:24]

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:153:
properties.configuration = std::vector<uint8_t>(bytes, bytes + length);
perhaps simply:

  properties.configuration.assign(bytes, bytes + length);

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:166:
properties.state.compare(kInvalidState) != 0) {
perhaps simply (string implements operator != (const char* ) correctly) :

  properties.state != kInvalidState

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:287: delay(nullptr),
scoped_ptr is default constructed to nullptr, so these two lines are not
necessary.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.h (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:65:
scoped_ptr<uint16_t> volume;
You probably want to make this struct non-copyable as it owns some scoped_ptrs.
That avoids mistakes due to unintended copying of the struct.

scoped_ptr, which now mimics C++11 unique_ptr, is movable not copyable. So this
struct is effectively not copyable.

scoped_ptr could also be implemented in a way that a copy transfers the
ownership of the bare pointer, so the scoped_ptrs in the struct being copied
will be reset to nullptr.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:107: virtual void
Released() = 0;
The "Released" seems confusing as it looks like a callback when the media
endpoint is "released".

How about ReleaseMediaEndpoint?

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:38:
dbus::ObjectPath object_path() const { return object_path_; }
This is more efficient:

const dbus::ObjectPath& object_path() const { return object_path_; }

[Miao, 2015-02-11 23:17:18]

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:153:
properties.configuration = std::vector<uint8_t>(bytes, bytes + length);
On 2015/02/11 20:36:23, Ben Chan wrote:
> perhaps simply:
> 
>   properties.configuration.assign(bytes, bytes + length);

Done.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:166:
properties.state.compare(kInvalidState) != 0) {
On 2015/02/11 20:36:23, Ben Chan wrote:
> perhaps simply (string implements operator != (const char* ) correctly) :
> 
>   properties.state != kInvalidState

Done.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:287: delay(nullptr),
On 2015/02/11 20:36:23, Ben Chan wrote:
> scoped_ptr is default constructed to nullptr, so these two lines are not
> necessary.

Removed, and also removed those in bluetooth_audio_sink_chromeos.cc and
bluetooth_audio_sink_chromeos_unittest.cc.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.h (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:65:
scoped_ptr<uint16_t> volume;
On 2015/02/11 20:36:23, Ben Chan wrote:
> You probably want to make this struct non-copyable as it owns some
scoped_ptrs.
> That avoids mistakes due to unintended copying of the struct.
> 
> scoped_ptr, which now mimics C++11 unique_ptr, is movable not copyable. So
this
> struct is effectively not copyable.
> 
> scoped_ptr could also be implemented in a way that a copy transfers the
> ownership of the bare pointer, so the scoped_ptrs in the struct being copied
> will be reset to nullptr.

Done.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:107: virtual void
Released() = 0;
On 2015/02/11 20:36:23, Ben Chan wrote:
> The "Released" seems confusing as it looks like a callback when the media
> endpoint is "released".
> 
> How about ReleaseMediaEndpoint?

In BluetoothProfileServiceProvider, Release() is renamed as Released() as well.
I think Released is good enough in this case, as this function will be called
after an endpoint has been released.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:33: // Sets the
visibility to the transport object to true if the media object
On 2015/02/10 22:49:11, Miao wrote:
> Update comment in patch 3.

Done.

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h (right):

https://codereview.chromium.org/910023002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:38:
dbus::ObjectPath object_path() const { return object_path_; }
On 2015/02/11 20:36:24, Ben Chan wrote:
> This is more efficient:
> 
> const dbus::ObjectPath& object_path() const { return object_path_; }

Done.

[armansito, 2015-02-12 05:20:44]

lgtm with nits. Also wait for Ben's approval.

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:32: const char
kInvalidState[] = "unknown";
I would move these two to the header as static class members, since other code
has no way to compare against these otherwise, unless you think that other code
won't ever see these.

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:166: properties.state
!= kInvalidState) {
nit: You should log something here in the case of an error.

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:170:
response_sender.Run(dbus::Response::FromMethodCall(method_call));
I thought this method had no response?

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_client.h (right):

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_client.h:47: void SetEndpointState(const
dbus::ObjectPath& endpoint_path, bool registered);
Rename this to SetEndpointRegistered?

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right):

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_transport_client.cc:138:
endpoints_[endpoint_path] = valid;
nit: early return.

[Miao, 2015-02-12 19:08:04]

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:32: const char
kInvalidState[] = "unknown";
On 2015/02/12 05:20:44, armansito wrote:
> I would move these two to the header as static class members, since other code
> has no way to compare against these otherwise, unless you think that other
code
> won't ever see these.

In SetConfiguration, we compare the values of state and codec with these two
constants. If they match, the delegate won't be invoked anyway, so other code
won't need these.

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:166: properties.state
!= kInvalidState) {
On 2015/02/12 05:20:44, armansito wrote:
> nit: You should log something here in the case of an error.

Done.

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:170:
response_sender.Run(dbus::Response::FromMethodCall(method_call));
On 2015/02/12 05:20:44, armansito wrote:
> I thought this method had no response?

It is an empty response, just like the response of Release().

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_client.h (right):

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_client.h:47: void SetEndpointState(const
dbus::ObjectPath& endpoint_path, bool registered);
On 2015/02/12 05:20:44, armansito wrote:
> Rename this to SetEndpointRegistered?

Done.

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right):

https://codereview.chromium.org/910023002/diff/40001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_transport_client.cc:138:
endpoints_[endpoint_path] = valid;
On 2015/02/12 05:20:44, armansito wrote:
> nit: early return.

Done.

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

https://codereview.chromium.org/910023002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:141:
ASSERT_EQ(callback_count_, 1);
Changed some ASSERT to EXPECT according to the contexts.

[Miao, 2015-02-12 19:08:58]

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

[Ben Chan, 2015-02-12 21:17:44]

lgtm with nits

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_client.cc (right):

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_client.cc:91: if (!visible_) {
nit:  you can do the following to save one indentation level

if (visible)
  return;

...

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:16:
FakeBluetoothMediaEndpointServiceProvider(const ObjectPath object_path,
should be passed by reference

const ObjectPath&

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right):

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_transport_client.cc:28: // static
there is already a // static above

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

https://codereview.chromium.org/910023002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:237:
TestAudioSinkObserver observer(audio_sink_);
perhaps a TODO in a follow-up CL, refactor common test set up code in these
tests as they look very similar in each test.

[Miao, 2015-02-12 22:53:52]

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

[Miao, 2015-02-12 22:54:06]

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_client.cc (right):

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_client.cc:91: if (!visible_) {
On 2015/02/12 21:17:44, Ben Chan wrote:
> nit:  you can do the following to save one indentation level
> 
> if (visible)
>   return;
> 
> ...

Done.

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc (right):

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.cc:16:
FakeBluetoothMediaEndpointServiceProvider(const ObjectPath object_path,
On 2015/02/12 21:17:44, Ben Chan wrote:
> should be passed by reference
> 
> const ObjectPath&

Done.

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_media_transport_client.cc (right):

https://codereview.chromium.org/910023002/diff/60001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_transport_client.cc:28: // static
On 2015/02/12 21:17:44, Ben Chan wrote:
> there is already a // static above

Done.

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

https://codereview.chromium.org/910023002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_audio_sink_chromeos_unittest.cc:237:
TestAudioSinkObserver observer(audio_sink_);
On 2015/02/12 21:17:44, Ben Chan wrote:
> perhaps a TODO in a follow-up CL, refactor common test set up code in these
> tests as they look very similar in each test.

Will refactor common tests in the next CL.

[Miao, 2015-02-12 22:54:18]

The CQ bit was checked by mcchou@chromium.org

[commit-bot: I haz the power, 2015-02-12 22:55:23]

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

[commit-bot: I haz the power, 2015-02-12 23:55:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-12 23:55:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Miao, 2015-02-13 20:02:24]

Patchset #6 (id:100001) has been deleted

[Miao, 2015-02-17 22:13:30]

Patchset #7 (id:140001) has been deleted

[Miao, 2015-02-17 22:13:43]

Patchset #6 (id:120001) has been deleted

[mcchou, 2015-02-17 23:59:15]

The CQ bit was checked by mcchou@google.com

[commit-bot: I haz the power, 2015-02-17 23:59:36]

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

[commit-bot: I haz the power, 2015-02-18 00:21:26]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-02-18 00:22:14]

Patchset 6 (id:??) landed as
https://crrev.com/016980f10044b64144293f9e1ba0ce4c5fddeda3
Cr-Commit-Position: refs/heads/master@{#316709}