chromeos/dbus: Add BlueZ Media Endpoint Service Provider API.

chromeos/dbus: Add BlueZ Media Endpoint Service Provider API.

This CL adds the BlueZ Media Endpoint service provider class and its
fake class, and add the files in chromeos/chromeos.gyp.

BUG=429016
Committed: https://crrev.com/ac03835cf1a407799ed2baddf350b663527e9977
Cr-Commit-Position: refs/heads/master@{#304976}

[Miao, 2014-11-15 03:52:36]

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

[Ben Chan, 2014-11-16 18:57:56]

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

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:48: VLOG(1) <<
"Createing Bluetooth Media Endpoint: " << object_path_.value();
nit: perhaps adding CHECK()s for any assumption you make on the arguments, e.g.

CHECK_NE(nullptr, bus);
CHECK_NE(nullptr, delegate);
CHECK(!object_path.empty());

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

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:19: // the bluetooth
daemon can commuicate with to serve as media source/sink.
bluetooth -> Bluetooth

as media -> as a media

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:22: // chromsthe D-Bus
object path as |endpoint_path| argument to the
chromsthe -> the

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:25: // After
initiateing a connection between an audio source and an audio sink, the
initiateing -> initiating

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:28: // using the
callbacks supplied to this methods.
this methods?  these methods or this method?  actually, which method(s)?

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:54: // streaing. Audio
Sources provide |capabilities| as a reference, where
streaing -> streaming

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:55: // a user
application can use this |capabilities| to figure out
this |capabilities| -> these |capabilities|

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:63: // This method
will be called when an Audio Source disconnects to an Audio
disconnects to -> disconnects from

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:64: // Sink. A user
applications is supposed to clear any of its resources which
applications -> application

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:65: // it keeps for
that partiular connection. |transport_path| is the Media
partiular -> particular

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:72: // tasks. These is
no need to unregister the endpoint, since when this
These -> There

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:79: // Create the
instance where |bus| is the D-Bus bus connection to export the
Create -> Creates (for consistency with comments in other classes)

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:80: // object noto,
|object_path| is the object path that it should have and
noto?

[Miao, 2014-11-17 20:51:02]

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

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:48: VLOG(1) <<
"Createing Bluetooth Media Endpoint: " << object_path_.value();
On 2014/11/16 18:57:56, Ben Chan wrote:
> nit: perhaps adding CHECK()s for any assumption you make on the arguments,
e.g.
> 
> CHECK_NE(nullptr, bus);
> CHECK_NE(nullptr, delegate);
> CHECK(!object_path.empty());

Done.

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

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:19: // the bluetooth
daemon can commuicate with to serve as media source/sink.
On 2014/11/16 18:57:56, Ben Chan wrote:
> bluetooth -> Bluetooth
> 
> as media -> as a media

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:22: // chromsthe D-Bus
object path as |endpoint_path| argument to the
On 2014/11/16 18:57:56, Ben Chan wrote:
> chromsthe -> the

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:25: // After
initiateing a connection between an audio source and an audio sink, the
On 2014/11/16 18:57:56, Ben Chan wrote:
> initiateing -> initiating

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:28: // using the
callbacks supplied to this methods.
On 2014/11/16 18:57:56, Ben Chan wrote:
> this methods?  these methods or this method?  actually, which method(s)?

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:54: // streaing. Audio
Sources provide |capabilities| as a reference, where
On 2014/11/16 18:57:56, Ben Chan wrote:
> streaing -> streaming

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:55: // a user
application can use this |capabilities| to figure out
On 2014/11/16 18:57:56, Ben Chan wrote:
> this |capabilities| -> these |capabilities| 

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:63: // This method
will be called when an Audio Source disconnects to an Audio
On 2014/11/16 18:57:56, Ben Chan wrote:
> disconnects to -> disconnects from

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:64: // Sink. A user
applications is supposed to clear any of its resources which
On 2014/11/16 18:57:56, Ben Chan wrote:
> applications -> application

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:65: // it keeps for
that partiular connection. |transport_path| is the Media
On 2014/11/16 18:57:56, Ben Chan wrote:
> partiular -> particular

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:72: // tasks. These is
no need to unregister the endpoint, since when this
On 2014/11/16 18:57:56, Ben Chan wrote:
> These -> There

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:79: // Create the
instance where |bus| is the D-Bus bus connection to export the
On 2014/11/16 18:57:56, Ben Chan wrote:
> Create -> Creates (for consistency with comments in other classes)

Done.

https://codereview.chromium.org/725383003/diff/1/chromeos/dbus/bluetooth_medi...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:80: // object noto,
|object_path| is the object path that it should have and
On 2014/11/16 18:57:56, Ben Chan wrote:
> noto?

Done.

[armansito, 2014-11-17 21:17:14]

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

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:48: VLOG(1) <<
"Createing Bluetooth Media Endpoint: " << object_path_.value();
nit: s/Createing/Creating/

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:51:
CHECK(object_path_.IsValid());
Use DCHECKs here please.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:206:
writer.AppendArrayOfBytes(configuration, length);
Make sure that this doesn't cause a crash if the delegate ever passes NULL for
|configuration| and a non-zero value for |length|.

Though you would inherently prevent this problem by using a vector here instead.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:252: } else {
You don't need the else as you're returning in each case. I would turn this if
statement around like:

  if (IsUsingStub())
    return new Fake;

  return new Real;

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

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:41: typedef
base::Callback<void(const uint8_t*, size_t)>
It's a bit nicer to pass an std::vector here than a pointer and a size
separately (this is also what we do in other places).

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:60: size_t length,
I would pass a std::vector here as well (i.e. const std::vector<uint8_t>&)

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

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:34: virtual void
Release();
These methods probably don't need to be virtual since they are meant for testing
you don't expect this class to get extended.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:40: // All
incoming method calles are passed to |delegate_|. |callback| passed to
nit: s/calles/calls/

[Miao, 2014-11-18 19:04:34]

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

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:48: VLOG(1) <<
"Createing Bluetooth Media Endpoint: " << object_path_.value();
On 2014/11/17 21:17:13, armansito wrote:
> nit: s/Createing/Creating/

Done.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:51:
CHECK(object_path_.IsValid());
On 2014/11/17 21:17:14, armansito wrote:
> Use DCHECKs here please.

Done.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:92: VLOG(1) <<
"Clearing uip Bluetooth Media Endpoint: "
s/Clearing uip/Cleaning up/

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:206:
writer.AppendArrayOfBytes(configuration, length);
On 2014/11/17 21:17:14, armansito wrote:
> Make sure that this doesn't cause a crash if the delegate ever passes NULL for
> |configuration| and a non-zero value for |length|.
> 
> Though you would inherently prevent this problem by using a vector here
instead.

Done.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:252: } else {
On 2014/11/17 21:17:13, armansito wrote:
> You don't need the else as you're returning in each case. I would turn this if
> statement around like:
> 
>   if (IsUsingStub())
>     return new Fake;
> 
>   return new Real;

Done.

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

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:41: typedef
base::Callback<void(const uint8_t*, size_t)>
On 2014/11/17 21:17:14, armansito wrote:
> It's a bit nicer to pass an std::vector here than a pointer and a size
> separately (this is also what we do in other places).

Done.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.h:60: size_t length,
On 2014/11/17 21:17:14, armansito wrote:
> I would pass a std::vector here as well (i.e. const std::vector<uint8_t>&)

Done.

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

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:34: virtual void
Release();
On 2014/11/17 21:17:14, armansito wrote:
> These methods probably don't need to be virtual since they are meant for
testing
> you don't expect this class to get extended.

I follow the pattern in other service providers. If is the case, maybe we'd like
to upload a CL to remove virtual keywords from other service providers.

https://codereview.chromium.org/725383003/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:40: // All
incoming method calles are passed to |delegate_|. |callback| passed to
On 2014/11/17 21:17:14, armansito wrote:
> nit: s/calles/calls/

Done.

[armansito, 2014-11-19 00:20:10]

lgtm

[Ben Chan, 2014-11-19 22:58:26]

lgtm w/ nits

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

https://codereview.chromium.org/725383003/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:101: bool
OnOriginThread() {
this method should be const

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

https://codereview.chromium.org/725383003/diff/40001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:30: const
dbus::MessageReader& properties);
fix indentation

[Miao, 2014-11-19 23:26:40]

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

https://codereview.chromium.org/725383003/diff/40001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_media_endpoint_service_provider.cc:101: bool
OnOriginThread() {
On 2014/11/19 22:58:25, Ben Chan wrote:
> this method should be const

Done. And we may want to do the same thing to other service providers.

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

https://codereview.chromium.org/725383003/diff/40001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_media_endpoint_service_provider.h:30: const
dbus::MessageReader& properties);
On 2014/11/19 22:58:25, Ben Chan wrote:
> fix indentation 

Done.

[Miao, 2014-11-20 03:01:59]

The CQ bit was checked by mcchou@chromium.org

[commit-bot: I haz the power, 2014-11-20 03:03:50]

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

[commit-bot: I haz the power, 2014-11-20 04:45:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-20 04:45:56]

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