[b2g-bluetooth] Support Bluetooth AVRCP 1.3

RESOLVED FIXED

Status

Firefox OS
Bluetooth
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shawnjohnjr, Assigned: gyeh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(6 attachments, 14 obsolete attachments)

22.13 KB, patch
Details | Diff | Splinter Review
16.84 KB, patch
Details | Diff | Splinter Review
3.69 KB, patch
Details | Diff | Splinter Review
8.58 KB, patch
Details | Diff | Splinter Review
5.55 KB, patch
Details | Diff | Splinter Review
19.52 KB, patch
Details | Diff | Splinter Review
Implement DOM API for AVRCP 1.3 meta data.
Music application can use it to pass Music meta data information to Bluetooth subsystem.
Such as: Song title, artist name, total track numbers,
track number, Play time, Duration, Position, Play Status.
Blocks: 807758
Blocks: 834553
No longer blocks: 807758
Redirect to Gina
Assignee: shuang → gyeh
(Assignee)

Updated

5 years ago
Duplicate of this bug: 887623
(Assignee)

Updated

5 years ago
Blocks: 843436
(Assignee)

Comment 3

5 years ago
Created attachment 771965 [details] [diff] [review]
Patch 1(v1): Implement API for AVRCP 1.3
(Assignee)

Comment 4

5 years ago
Comment on attachment 771965 [details] [diff] [review]
Patch 1(v1): Implement API for AVRCP 1.3

Two API are added in nsIDOMBluetoothAdatper.idl
1. nsIDOMDOMRequest sendMusicMetaData(in jsval aOptions);
2. nsIDOMDOMRequest sendMusicPlayStatus(in jsval aOptions);

For the first API, the parameter should be a dictionary with type "MusicMetaData".
For the second API, the parameter should be a dictionary with type "MusicPlayStatus".

Each member of the dictionaries is clearly defined with comments in nsIDOMBluetoothAdapter.idl.
(Assignee)

Comment 5

5 years ago
Comment on attachment 771965 [details] [diff] [review]
Patch 1(v1): Implement API for AVRCP 1.3

Please help to review this patch, echou and mrbkap. Thanks.
Attachment #771965 - Flags: superreview?(mrbkap)
Attachment #771965 - Flags: review?(echou)
(Assignee)

Comment 6

5 years ago
Created attachment 771972 [details] [diff] [review]
Patch 2(v1): Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase
(Assignee)

Comment 7

5 years ago
Comment on attachment 771972 [details] [diff] [review]
Patch 2(v1): Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase

Several changes in this patch:

* Handle control signal of "PropertyChanged"
* Add IsConnected() in class BluetoothProfileManagerBase
  - In BluetoothOppManager, rename function IsTransferring() to function IsConnected()
  - In BluetoothA2dpManager, implement function IsConnected() and IsAvrcpConnected()
* Reset A2dp and Avrcp data members when turning on Bluetooth
* Make code neater for function GetConnectedDevicePropertiesInternal() and function IsConnected()
(Assignee)

Comment 8

5 years ago
Comment on attachment 771972 [details] [diff] [review]
Patch 2(v1): Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase

Please review this patch. Thanks.
Attachment #771972 - Flags: review?(echou)
(Assignee)

Comment 9

5 years ago
Created attachment 771976 [details] [diff] [review]
Patch 3(v1): Cache AVRCP data in BluetoothA2dpManager
(Assignee)

Comment 10

5 years ago
Comment on attachment 771976 [details] [diff] [review]
Patch 3(v1): Cache AVRCP data in BluetoothA2dpManager

AVRCP data are kept in class BluetoothA2dpManager and are updated whenever the APIs are called.
(Assignee)

Comment 11

5 years ago
Comment on attachment 771976 [details] [diff] [review]
Patch 3(v1): Cache AVRCP data in BluetoothA2dpManager

One more review request to echou~
Attachment #771976 - Flags: review?(echou)
(Assignee)

Comment 12

5 years ago
Created attachment 771979 [details] [diff] [review]
Patch 4(v1): Handle control signal of "GetPlayStatus"
(Assignee)

Comment 13

5 years ago
Comment on attachment 771979 [details] [diff] [review]
Patch 4(v1): Handle control signal of "GetPlayStatus"

The general idea of this patch is that, when receiving signal of "GetPlayStatus", we send a dbus call of "UpdatePlayStatus" as reply.

In this patch, I used the cached data in BluetoothA2dpManager for replying. So several getter functions are added in class BluetoothA2dpManager.

On the other hands, renaming function CheckForError() and reusing it in ControlCallback().
(Assignee)

Comment 14

5 years ago
Comment on attachment 771979 [details] [diff] [review]
Patch 4(v1): Handle control signal of "GetPlayStatus"

One more!
Attachment #771979 - Flags: review?(echou)
(Assignee)

Comment 15

5 years ago
Created attachment 771982 [details] [diff] [review]
Patch 5(v1): Implement UpdateNotification
(Assignee)

Comment 16

5 years ago
Comment on attachment 771982 [details] [diff] [review]
Patch 5(v1): Implement UpdateNotification

Three essential notifications are implemented in this patch:
1. EVENT_TRACK_CHANGED
2. EVENT_PLAYBACK_POS_CHANGED
3. EVENT_PLAYBACK_STATUS_CHANGED

I'd like to implement the others in a follow-up.
(Assignee)

Comment 17

5 years ago
Comment on attachment 771982 [details] [diff] [review]
Patch 5(v1): Implement UpdateNotification

Should be the last patch for this bug :|
Attachment #771982 - Flags: review?(echou)
(Assignee)

Updated

5 years ago
Component: General → Bluetooth
Summary: [Bluetooth]Implement DOM callback function for AVRCP 1.3 meta data feature → [b2g-bluetooth] Support Bluetooth AVRCP 1.3
Comment on attachment 771965 [details] [diff] [review]
Patch 1(v1): Implement API for AVRCP 1.3

Review of attachment 771965 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2882,5 @@
> +
> +  bool ret = dbus_func_args_async(mConnection,
> +                                  -1,
> +                                  GetVoidCallback,
> +                                  (void*)runnable,

Please write this as: runnable.get() to avoid the c-style cast.

@@ +2955,5 @@
> +  uint32_t tempPlayStatus = playStatus;
> +  bool ret = dbus_func_args_async(mConnection,
> +                                  -1,
> +                                  GetVoidCallback,
> +                                  (void*)runnable,

Ditto here.
Attachment #771965 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 19

5 years ago
Thanks, mrbkap.
Comment on attachment 771965 [details] [diff] [review]
Patch 1(v1): Implement API for AVRCP 1.3

Review of attachment 771965 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2904,5 @@
> +{
> +  ControlPlayStatus playStatus = ControlPlayStatus::PLAYSTATUS_UNKNOWN;
> +  if (aPlayStatus.EqualsLiteral("STOPPED")) {
> +    playStatus = ControlPlayStatus::PLAYSTATUS_STOPPED;
> +  } if (aPlayStatus.EqualsLiteral("PLAYING")) {

It seems that you missed an 'else'.

@@ +2935,5 @@
> +  }
> +
> +  ControlPlayStatus playStatus =
> +    PlayStatusStringToControlPlayStatus(aPlayStatus);
> +  if (playStatus ==  ControlPlayStatus::PLAYSTATUS_UNKNOWN) {

super-nit: extra blank after the double-equal sign

::: dom/bluetooth/nsIDOMBluetoothAdapter.idl
@@ +92,5 @@
>    nsIDOMDOMRequest confirmReceivingFile(in DOMString aDeviceAddress, in bool aConfirmation);
>  
> +  // AVRCP 1.3 methods
> +  nsIDOMDOMRequest sendMusicMetaData(in jsval aOptions);
> +  nsIDOMDOMRequest sendMusicPlayStatus(in jsval aOptions);

Since AVRCP is the "Audio/Video Remote Control Profile", 'Music' seems a little too specific.

How about sendMediaMetaData/sendMediaPlayStatus? :)
Attachment #771965 - Flags: review?(echou) → review+
(Assignee)

Comment 21

5 years ago
(In reply to Eric Chou [:ericchou] [:echou] from comment #20)
> > +  if (aPlayStatus.EqualsLiteral("STOPPED")) {
> > +    playStatus = ControlPlayStatus::PLAYSTATUS_STOPPED;
> > +  } if (aPlayStatus.EqualsLiteral("PLAYING")) {
> 
> It seems that you missed an 'else'.

I think so :|

> > +  // AVRCP 1.3 methods
> > +  nsIDOMDOMRequest sendMusicMetaData(in jsval aOptions);
> > +  nsIDOMDOMRequest sendMusicPlayStatus(in jsval aOptions);
> 
> Since AVRCP is the "Audio/Video Remote Control Profile", 'Music' seems a
> little too specific.
> 
> How about sendMediaMetaData/sendMediaPlayStatus? :)

Good points. Let me update patch.
Comment on attachment 771972 [details] [diff] [review]
Patch 2(v1): Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase

Review of attachment 771972 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +70,5 @@
>  #define BLUEZ_DBUS_BASE_PATH      "/org/bluez"
>  #define BLUEZ_DBUS_BASE_IFC       "org.bluez"
>  #define BLUEZ_ERROR_IFC           "org.bluez.Error"
>  
> +

nit: extra line

@@ +2542,3 @@
>    }
>  
> +  return profile->IsConnected();

It would be better to have a null-check before using [profile], i.e. "NS_ENSURE_TRUE(profile, false);"
Attachment #771972 - Flags: review?(echou) → review+
Attachment #771976 - Flags: review?(echou) → review+
Comment on attachment 771979 [details] [diff] [review]
Patch 4(v1): Handle control signal of "GetPlayStatus"

Review of attachment 771979 [details] [diff] [review]:
-----------------------------------------------------------------

Gina, please check my comments and take your time to reply. Thanks.

::: dom/bluetooth/BluetoothA2dpManager.h
@@ +60,5 @@
>                          ControlPlayStatus aPlayStatus);
> +  void GetDuration(uint32_t* aDuration);
> +  void GetPlayStatus(ControlPlayStatus* aPlayStatus);
> +  void GetPosition(uint32_t* aPosition);
> +  void GetMediaNumber(uint32_t* aMediaNumber);

For these getters which return primitive-type values, I would prefer returning uint32_t/ControlPlayStatus directly.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1553,5 @@
>                          sSinkProperties,
>                          ArrayLength(sSinkProperties));
> +  } else if (dbus_message_is_signal(aMsg, DBUS_CTL_IFACE, "GetPlayStatus")) {
> +    nsRefPtr<nsRunnable> task = new SendPlayStatusTask();
> +    NS_DispatchToMainThread(task);

NS_DispatchToMainThread(new SendPlayStatusTask()) should be fine.

@@ +1576,5 @@
>    nsRefPtr<nsRunnable> task;
>    if (signalInterface.EqualsLiteral(DBUS_SINK_IFACE)) {
>      task = new SinkPropertyChangedHandler(signal);
> +  } else if (signalInterface.EqualsLiteral(DBUS_CTL_IFACE) &&
> +             signalName.EqualsLiteral("PropertyChanged")) {

Question: What are other signals we may receive besides "GetPlayStatus"? If there are some, then those signals will be directed to the 'else', which will be handled as

  task = new DistributeBluetoothSignalTask(signal);

Is this the same as expected?
(Assignee)

Comment 24

5 years ago
(In reply to Eric Chou [:ericchou] [:echou] from comment #23)
> 
> ::: dom/bluetooth/BluetoothA2dpManager.h
> @@ +60,5 @@
> >                          ControlPlayStatus aPlayStatus);
> > +  void GetDuration(uint32_t* aDuration);
> > +  void GetPlayStatus(ControlPlayStatus* aPlayStatus);
> > +  void GetPosition(uint32_t* aPosition);
> > +  void GetMediaNumber(uint32_t* aMediaNumber);
> 
> For these getters which return primitive-type values, I would prefer
> returning uint32_t/ControlPlayStatus directly.

Okay. I'll update in the next patch.

> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1553,5 @@
> >                          sSinkProperties,
> >                          ArrayLength(sSinkProperties));
> > +  } else if (dbus_message_is_signal(aMsg, DBUS_CTL_IFACE, "GetPlayStatus")) {
> > +    nsRefPtr<nsRunnable> task = new SendPlayStatusTask();
> > +    NS_DispatchToMainThread(task);
> 
> NS_DispatchToMainThread(new SendPlayStatusTask()) should be fine.
> 

Suggestion is taken. Will update in the next patch.

> @@ +1576,5 @@
> >    nsRefPtr<nsRunnable> task;
> >    if (signalInterface.EqualsLiteral(DBUS_SINK_IFACE)) {
> >      task = new SinkPropertyChangedHandler(signal);
> > +  } else if (signalInterface.EqualsLiteral(DBUS_CTL_IFACE) &&
> > +             signalName.EqualsLiteral("PropertyChanged")) {
> 
> Question: What are other signals we may receive besides "GetPlayStatus"? If
> there are some, then those signals will be directed to the 'else', which
> will be handled as
> 
>   task = new DistributeBluetoothSignalTask(signal);
> 
> Is this the same as expected?

Four signals would be received.
(B2G/external/bluetooth/bluez/audio/control.c)
static GDBusSignalTable control_signals[] = {
  { "Connected",      "", G_DBUS_SIGNAL_FLAG_DEPRECATED},
  { "Disconnected",   "", G_DBUS_SIGNAL_FLAG_DEPRECATED},
  { "PropertyChanged",    "sv"  },
  { "GetPlayStatus",    ""  },
  { NULL, NULL }
};

The first two are going to deprecated and I think we can just ignore them.

In my patch, when receiving "Connected"/"Disconnected" signal from control interface, signals are forwarded to BluetoothDevice.cpp and then being ignored.

It works as expected but we can just discard the signals and do not forward them to BluetoothDevice, right?
Comment on attachment 771982 [details] [diff] [review]
Patch 5(v1): Implement UpdateNotification

Review of attachment 771982 [details] [diff] [review]:
-----------------------------------------------------------------

Gina, please check my comments and reply. Thank you.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2988,5 @@
> +  nsAutoString prevTitle;
> +  a2dp->GetTitle(prevTitle);
> +
> +  ControlEventId eventId = ControlEventId::EVENT_UNKNOWN;
> +  uint64_t data;

These two variables seem unnecessary since they have been only used in the following UpdateNotification(). Using

  UpdateNotification(ControlEventId::EVENT_TRACK_CHANGED, aMediaNumber);

should be the same as current implementation.

@@ +2989,5 @@
> +  a2dp->GetTitle(prevTitle);
> +
> +  ControlEventId eventId = ControlEventId::EVENT_UNKNOWN;
> +  uint64_t data;
> +  if (aMediaNumber != prevMediaNumber || !aTitle.Equals(prevTitle)) {

Question: why do we need to check MediaNumber and Title at the same time? Can we just check MediaNumber?

@@ +3088,5 @@
> +  uint64_t data;
> +  if (aPosition != prevPosition) {
> +    eventId = ControlEventId::EVENT_PLAYBACK_POS_CHANGED;
> +    data = aPosition;
> +  } else if (playStatus != prevPlayStauts) {

Question: is it possible that both "aPosition != prevPosition" and "playStatus != prevPlayStauts" are true at the same time? If it is, only EVENT_PLAYBACK_POS_CHANGED would be sent to the remote device. Is this the same as expected or we should send two notifications?

@@ +3166,5 @@
> +                                  nullptr,
> +                                  NS_ConvertUTF16toUTF8(objectPath).get(),
> +                                  DBUS_CTL_IFACE,
> +                                  "UpdateNotification",
> +                                  DBUS_TYPE_UINT16, &aEventId,

We should use a uint16_t variable to hold the value of aEventId, or at least cast aEventId to uint16_t.

::: dom/bluetooth/linux/BluetoothDBusService.h
@@ +174,5 @@
>    SendSinkMessage(const nsAString& aDeviceAddresses,
>                    const nsAString& aMessage) MOZ_OVERRIDE;
>  
>  private:
> +  enum ControlEventId {

Please add a simple comment about where this enumeration is defined.
(Assignee)

Comment 26

5 years ago
(In reply to Eric Chou [:ericchou] [:echou] from comment #25)
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +2988,5 @@
> > +  nsAutoString prevTitle;
> > +  a2dp->GetTitle(prevTitle);
> > +
> > +  ControlEventId eventId = ControlEventId::EVENT_UNKNOWN;
> > +  uint64_t data;
> 
> These two variables seem unnecessary since they have been only used in the
> following UpdateNotification(). Using
> 
>   UpdateNotification(ControlEventId::EVENT_TRACK_CHANGED, aMediaNumber);
> 
> should be the same as current implementation.

Sounds great! Will update in the next patch.

> > @@ +2989,5 @@
> > +  a2dp->GetTitle(prevTitle);
> > +
> > +  ControlEventId eventId = ControlEventId::EVENT_UNKNOWN;
> > +  uint64_t data;
> > +  if (aMediaNumber != prevMediaNumber || !aTitle.Equals(prevTitle)) {
> 
> Question: why do we need to check MediaNumber and Title at the same time?
> Can we just check MediaNumber?
> 

It could be possible. For example, when we're playing the 1st track of one album, we want to play a new album from the 1st track. Then, the media number won't be changed in this case, and I think title/album could be helpful, right?

> @@ +3088,5 @@
> > +  uint64_t data;
> > +  if (aPosition != prevPosition) {
> > +    eventId = ControlEventId::EVENT_PLAYBACK_POS_CHANGED;
> > +    data = aPosition;
> > +  } else if (playStatus != prevPlayStauts) {
> 
> Question: is it possible that both "aPosition != prevPosition" and
> "playStatus != prevPlayStauts" are true at the same time? If it is, only
> EVENT_PLAYBACK_POS_CHANGED would be sent to the remote device. Is this the
> same as expected or we should send two notifications?

With our UI design, I think we can only do one operation (seek/stop/play) at a time. But it would be great if we separate them into two if conditions. :)

> 
> @@ +3166,5 @@
> > +                                  nullptr,
> > +                                  NS_ConvertUTF16toUTF8(objectPath).get(),
> > +                                  DBUS_CTL_IFACE,
> > +                                  "UpdateNotification",
> > +                                  DBUS_TYPE_UINT16, &aEventId,
> 
> We should use a uint16_t variable to hold the value of aEventId, or at least
> cast aEventId to uint16_t.
> 

Suggestion is taken. ;)

> ::: dom/bluetooth/linux/BluetoothDBusService.h
> @@ +174,5 @@
> >    SendSinkMessage(const nsAString& aDeviceAddresses,
> >                    const nsAString& aMessage) MOZ_OVERRIDE;
> >  
> >  private:
> > +  enum ControlEventId {
> 
> Please add a simple comment about where this enumeration is defined.

Ok. I will add comment in the next patch.
> > @@ +1576,5 @@
> > >    nsRefPtr<nsRunnable> task;
> > >    if (signalInterface.EqualsLiteral(DBUS_SINK_IFACE)) {
> > >      task = new SinkPropertyChangedHandler(signal);
> > > +  } else if (signalInterface.EqualsLiteral(DBUS_CTL_IFACE) &&
> > > +             signalName.EqualsLiteral("PropertyChanged")) {
> > 
> > Question: What are other signals we may receive besides "GetPlayStatus"? If
> > there are some, then those signals will be directed to the 'else', which
> > will be handled as
> > 
> >   task = new DistributeBluetoothSignalTask(signal);
> > 
> > Is this the same as expected?
> 
> Four signals would be received.
> (B2G/external/bluetooth/bluez/audio/control.c)
> static GDBusSignalTable control_signals[] = {
>   { "Connected",      "", G_DBUS_SIGNAL_FLAG_DEPRECATED},
>   { "Disconnected",   "", G_DBUS_SIGNAL_FLAG_DEPRECATED},
>   { "PropertyChanged",    "sv"  },
>   { "GetPlayStatus",    ""  },
>   { NULL, NULL }
> };
> 
> The first two are going to deprecated and I think we can just ignore them.
> 
> In my patch, when receiving "Connected"/"Disconnected" signal from control
> interface, signals are forwarded to BluetoothDevice.cpp and then being
> ignored.
> 
> It works as expected but we can just discard the signals and do not forward
> them to BluetoothDevice, right?

DBus event "Connected"/"Disconnected" sent from DBUS_CTL_IFACE would be handled by the 'else' block at the end of the huge 'if' in function EventFilter(), is it right? If it is, then we shouldn't be too worried about these stuff and should be able to remove 

  signalName.EqualsLiteral("PropertyChanged")

from 

+  } else if (signalInterface.EqualsLiteral(DBUS_CTL_IFACE) &&
+             signalName.EqualsLiteral("PropertyChanged")) {
(Assignee)

Comment 28

5 years ago
(In reply to Eric Chou [:ericchou] [:echou] from comment #27)
> DBus event "Connected"/"Disconnected" sent from DBUS_CTL_IFACE would be
> handled by the 'else' block at the end of the huge 'if' in function
> EventFilter(), is it right? If it is, then we shouldn't be too worried about
> these stuff and should be able to remove 
> 
>   signalName.EqualsLiteral("PropertyChanged")
> 
> from 
> 
> +  } else if (signalInterface.EqualsLiteral(DBUS_CTL_IFACE) &&
> +             signalName.EqualsLiteral("PropertyChanged")) {

Agree. It could be removed since signal GetPlayStatus has been handled and returned before.
(Assignee)

Comment 29

5 years ago
Created attachment 777722 [details] [diff] [review]
Patch 1: Implement API for AVRCP 1.3, r=echou, sr=mrbkap

Rename API to "SendMediaMetaData" and "SendMediaPlayStatus"
Attachment #771965 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Created attachment 777723 [details] [diff] [review]
Patch 2: Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase, r=echou
Attachment #771972 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Created attachment 777724 [details] [diff] [review]
Patch 3: Cache AVRCP data in BluetoothA2dpManager, r=echou
Attachment #771976 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 777725 [details] [diff] [review]
Patch 4(v2): Handle signal "GetPlayStatus"
Attachment #771979 - Attachment is obsolete: true
Attachment #771979 - Flags: review?(echou)
Attachment #777725 - Flags: review?(echou)
(Assignee)

Comment 33

5 years ago
Created attachment 777726 [details] [diff] [review]
Patch 5(v2): Implement UpdateNotification
Attachment #771982 - Attachment is obsolete: true
Attachment #771982 - Flags: review?(echou)
Attachment #777726 - Flags: review?(echou)
(Assignee)

Comment 34

5 years ago
Created attachment 777730 [details] [diff] [review]
Patch 4(v2): Handle signal "GetPlayStatus"
Attachment #777725 - Attachment is obsolete: true
Attachment #777725 - Flags: review?(echou)
Attachment #777730 - Flags: review?(echou)
Comment on attachment 777730 [details] [diff] [review]
Patch 4(v2): Handle signal "GetPlayStatus"

Review of attachment 777730 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #777730 - Flags: review?(echou) → review+
Comment on attachment 777726 [details] [diff] [review]
Patch 5(v2): Implement UpdateNotification

Review of attachment 777726 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit addressed. Good work!

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2979,5 @@
>  
> +  nsAutoString prevTitle;
> +  a2dp->GetTitle(prevTitle);
> +
> +  if (aMediaNumber != a2dp->GetMediaNumber() || !aTitle.Equals(prevTitle)) {

Per discussion with Gina and Shawn, let's also check if 'album' is changed. It it is, then send EVENT_TRACK_CHANGED as well.
Attachment #777726 - Flags: review?(echou) → review+
(Assignee)

Comment 37

5 years ago
Created attachment 778832 [details] [diff] [review]
[Final] Patch 1: Implement API for AVRCP 1.3, r=echou, sr=mrbkap
Attachment #777722 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
Created attachment 778833 [details] [diff] [review]
[Final] Patch 1: Implement API for AVRCP 1.3, r=echou, sr=mrbkap
Attachment #778832 - Attachment is obsolete: true
(Assignee)

Comment 39

5 years ago
Created attachment 778834 [details] [diff] [review]
[Final] Patch 2: Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase, r=echou
Attachment #777723 - Attachment is obsolete: true
(Assignee)

Comment 40

5 years ago
Created attachment 778835 [details] [diff] [review]
[Final] Patch 3: Cache AVRCP data in BluetoothA2dpManager, r=echou
Attachment #777724 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
Created attachment 778836 [details] [diff] [review]
[Final] Patch 4: Handle signal "GetPlayStatus", r=echou
Attachment #777730 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 778837 [details] [diff] [review]
[Final] Patch 5: Implement UpdateNotification, r=echou
Attachment #777726 - Attachment is obsolete: true
(Assignee)

Comment 44

5 years ago
Created attachment 779059 [details] [diff] [review]
[Final] Patch 5: Implement UpdateNotification, r=echou
Attachment #778837 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Note that these patches aren't ready to land since they break the build on other platforms.

The main problem is that our dictionary generator (js/xpconnect/src/dictionary_helper_gen.py) cannot load nsIDOMBluetoothAdapter.idl properly on other platforms because bluetooth module is only build on B2G.

Per thinker, we'll open a bug for fixing it and land these patches after then.
(Assignee)

Updated

5 years ago
Depends on: 896388
(Assignee)

Comment 46

5 years ago
Created attachment 780344 [details] [diff] [review]
Patch 6(v1): Implement dictionaries

Eric, Please help to review this patch. Thanks.
Attachment #780344 - Flags: review?(echou)
Comment on attachment 780344 [details] [diff] [review]
Patch 6(v1): Implement dictionaries

Review of attachment 780344 [details] [diff] [review]:
-----------------------------------------------------------------

No big revisions are required. Although this mechanism may be refactored again after bug 888595 lands, please check it into codebase with these nits addressed. Thank you.

::: dom/bluetooth/MediaMetaData.cpp
@@ +8,5 @@
> +
> +#include "nsCxPusher.h"
> +#include "nsContentUtils.h"
> +#include "nsThreadUtils.h"
> +#include "nsJSUtils.h"

Alphabetic order, please.

@@ +10,5 @@
> +#include "nsContentUtils.h"
> +#include "nsThreadUtils.h"
> +#include "nsJSUtils.h"
> +
> +#include "BluetoothCommon.h"

Unnecessary since it's already included in MediaMetaData.h

@@ +19,5 @@
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GonkDBus", args);
> +#else
> +#define BTDEBUG true
> +#define LOG(args...) if (BTDEBUG) printf(args);
> +#endif

Can we use BT_LOG() instead?

@@ +25,5 @@
> +using namespace mozilla;
> +USING_BLUETOOTH_NAMESPACE
> +
> +MediaMetaData::MediaMetaData() :
> +  album(EmptyString()),

No need to initialize an nsString instance with 'EmptyString()', which is equal to nsString(). Here and below.

@@ +33,5 @@
> +  title(EmptyString()),
> +  totalMediaCount(-1)
> +{}
> +
> +MediaMetaData::~MediaMetaData() {}

Since the destructor does nothing, please remove it.

::: dom/bluetooth/MediaMetaData.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_bluetooth_media_metadata_h__

Please remove the underscore after 'media'.

@@ +7,5 @@
> +#ifndef mozilla_dom_bluetooth_media_metadata_h__
> +#define mozilla_dom_bluetooth_media_metadata_h__
> +
> +#include "jsapi.h"
> +//#include "nsError.h"

Please remove this.

@@ +10,5 @@
> +#include "jsapi.h"
> +//#include "nsError.h"
> +#include "nsString.h"
> +#include "nsCOMPtr.h"
> +#include "BluetoothCommon.h"

Alphabetic order, please.

@@ +17,5 @@
> +
> +class MediaMetaData
> +{
> +public:
> +

nit: extra line

@@ +23,5 @@
> +  ~MediaMetaData();
> +
> +  nsresult Init(JSContext* aCx, const jsval* aVal);
> +
> +  nsString album;

The name of class member properties should start with an 'm'. (e.g. mDuration). Here and below.

::: dom/bluetooth/MediaPlayStatus.cpp
@@ +8,5 @@
> +
> +#include "nsCxPusher.h"
> +#include "nsContentUtils.h"
> +#include "nsThreadUtils.h"
> +#include "nsJSUtils.h"

Alphabetic order, please.

@@ +10,5 @@
> +#include "nsContentUtils.h"
> +#include "nsThreadUtils.h"
> +#include "nsJSUtils.h"
> +
> +#include "BluetoothCommon.h"

Unnecessary since it's already included in MediaPlayStatus.h.

@@ +19,5 @@
> +#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GonkDBus", args);
> +#else
> +#define BTDEBUG true
> +#define LOG(args...) if (BTDEBUG) printf(args);
> +#endif

Can we use BT_LOG() instead?

@@ +26,5 @@
> +USING_BLUETOOTH_NAMESPACE
> +
> +MediaPlayStatus::MediaPlayStatus() :
> +  duration(-1),
> +  playStatus(),

This is unnecessary.

@@ +30,5 @@
> +  playStatus(),
> +  position(-1)
> +{}
> +
> +MediaPlayStatus::~MediaPlayStatus() {}

Since the destructor does nothing, please remove it.

::: dom/bluetooth/MediaPlayStatus.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_bluetooth_media_playstatus_h__

Please remove the underscore after 'media'.

@@ +8,5 @@
> +#define mozilla_dom_bluetooth_media_playstatus_h__
> +
> +#include "jsapi.h"
> +#include "nsString.h"
> +//#include "nsCOMPtr.h"

Please remove this.

@@ +9,5 @@
> +
> +#include "jsapi.h"
> +#include "nsString.h"
> +//#include "nsCOMPtr.h"
> +#include "BluetoothCommon.h"

Alphabetic order, please.

@@ +21,5 @@
> +  ~MediaPlayStatus();
> +
> +  nsresult Init(JSContext* aCx, const jsval* aVal);
> +
> +  int64_t duration;

The name of class member properties should start with an 'm'. (e.g. mDuration). Here and below.
Attachment #780344 - Flags: review?(echou) → review+
(Assignee)

Comment 48

5 years ago
Created attachment 782420 [details] [diff] [review]
[Final] Patch 6: Implement dictionaries, r=echou

Final patch with nits picked.
Attachment #780344 - Attachment is obsolete: true
So I don't understand this manual dictionary handling.
In general any manual significant JS API usage in C++ code is, IMHO, a bug. We should have
tools to hide that. And WebIDL dictionaries should work just fine here. Or am I missing something?
(Assignee)

Comment 53

5 years ago
(In reply to Olli Pettay [:smaug] from comment #52)
> So I don't understand this manual dictionary handling.
> In general any manual significant JS API usage in C++ code is, IMHO, a bug.
> We should have
> tools to hide that. And WebIDL dictionaries should work just fine here. Or
> am I missing something?

Originally, I wanted to create a MediaMetaData.webidl and a MediaPlayStatus.webidl.

However, I realized that these two WebIDLs will be removed after we move nsIDOMBluetoothAdapter.idl to BluetoothAdapter.webidl in bug 888595, and I think that it's not necessary to put these dictionaries in independent WebIDLs.

After discussed with Eric, we decided to implement dictionaries manually for now, and remove them after bug 888595 is landed.
(Assignee)

Updated

5 years ago
Blocks: 905599
You need to log in before you can comment on or make changes to this bug.