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.

Redirect to Gina

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.

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.

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()

Comment on attachment 771972 [details] [diff] [review] Patch 2(v1): Handle control signal "PropertyChanged" and add IsConnected() in BluetoothProfileManagerBase Please review this patch. Thanks.

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.

Comment on attachment 771976 [details] [diff] [review] Patch 3(v1): Cache AVRCP data in BluetoothA2dpManager One more review request to echou~

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().

Comment on attachment 771979 [details] [diff] [review] Patch 4(v1): Handle control signal of "GetPlayStatus" One more!

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.

Comment on attachment 771982 [details] [diff] [review] Patch 5(v1): Implement UpdateNotification Should be the last patch for this bug :|

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.

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? :)

(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);"

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?

(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.

(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")) {

(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.

Rename API to "SendMediaMetaData" and "SendMediaPlayStatus"

Comment on attachment 777730 [details] [diff] [review] Patch 4(v2): Handle signal "GetPlayStatus" Review of attachment 777730 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

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.

try: https://tbpl.mozilla.org/?tree=Try&rev=83b8ef76a237 https://tbpl.mozilla.org/?tree=Try&rev=c51f9d394368

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.

Eric, Please help to review this patch. Thanks.

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.

Final patch with nits picked.

try: https://tbpl.mozilla.org/?tree=Try&rev=ddc719d9a1d1 https://tbpl.mozilla.org/?tree=Try&rev=3bc55c851e0a

https://hg.mozilla.org/projects/birch/rev/9ea0ac7eaad0 https://hg.mozilla.org/projects/birch/rev/8e6480737492 https://hg.mozilla.org/projects/birch/rev/6f310a63ef09 https://hg.mozilla.org/projects/birch/rev/901cf9892170 https://hg.mozilla.org/projects/birch/rev/63e223a89cda https://hg.mozilla.org/projects/birch/rev/64dfe43d269f

https://hg.mozilla.org/mozilla-central/rev/9ea0ac7eaad0 https://hg.mozilla.org/mozilla-central/rev/8e6480737492 https://hg.mozilla.org/mozilla-central/rev/6f310a63ef09 https://hg.mozilla.org/mozilla-central/rev/901cf9892170 https://hg.mozilla.org/mozilla-central/rev/63e223a89cda https://hg.mozilla.org/mozilla-central/rev/64dfe43d269f

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?

(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.