Closed
Bug 842948
Opened 12 years ago
Closed 11 years ago
[b2g-bluetooth] Support Bluetooth AVRCP 1.3
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: shawnjohnjr, Assigned: gyeh)
References
Details
(Whiteboard: [fixed-in-birch])
Attachments
(6 files, 14 obsolete files)
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.
Reporter | ||
Updated•12 years ago
|
Blocks: b2g-bluetooth-a2dp
Reporter | ||
Updated•12 years ago
|
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 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•11 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•11 years ago
|
||
Assignee | ||
Comment 7•11 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•11 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•11 years ago
|
||
Assignee | ||
Comment 10•11 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•11 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•11 years ago
|
||
Assignee | ||
Comment 13•11 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•11 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•11 years ago
|
||
Assignee | ||
Comment 16•11 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•11 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•11 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 18•11 years ago
|
||
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•11 years ago
|
||
Thanks, mrbkap.
Comment 20•11 years ago
|
||
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•11 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 22•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #771976 -
Flags: review?(echou) → review+
Comment 23•11 years ago
|
||
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•11 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 25•11 years ago
|
||
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•11 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.
Comment 27•11 years ago
|
||
> > @@ +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•11 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•11 years ago
|
||
Rename API to "SendMediaMetaData" and "SendMediaPlayStatus"
Attachment #771965 -
Attachment is obsolete: true
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #771972 -
Attachment is obsolete: true
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #771976 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #771979 -
Attachment is obsolete: true
Attachment #771979 -
Flags: review?(echou)
Attachment #777725 -
Flags: review?(echou)
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #771982 -
Attachment is obsolete: true
Attachment #771982 -
Flags: review?(echou)
Attachment #777726 -
Flags: review?(echou)
Assignee | ||
Comment 34•11 years ago
|
||
Attachment #777725 -
Attachment is obsolete: true
Attachment #777725 -
Flags: review?(echou)
Attachment #777730 -
Flags: review?(echou)
Comment 35•11 years ago
|
||
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 36•11 years ago
|
||
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•11 years ago
|
||
Attachment #777722 -
Attachment is obsolete: true
Assignee | ||
Comment 38•11 years ago
|
||
Attachment #778832 -
Attachment is obsolete: true
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #777723 -
Attachment is obsolete: true
Assignee | ||
Comment 40•11 years ago
|
||
Attachment #777724 -
Attachment is obsolete: true
Assignee | ||
Comment 41•11 years ago
|
||
Attachment #777730 -
Attachment is obsolete: true
Assignee | ||
Comment 42•11 years ago
|
||
Attachment #777726 -
Attachment is obsolete: true
Assignee | ||
Comment 43•11 years ago
|
||
Assignee | ||
Comment 44•11 years ago
|
||
Attachment #778837 -
Attachment is obsolete: true
Assignee | ||
Comment 45•11 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 | ||
Comment 46•11 years ago
|
||
Eric, Please help to review this patch. Thanks.
Attachment #780344 -
Flags: review?(echou)
Comment 47•11 years ago
|
||
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•11 years ago
|
||
Final patch with nits picked.
Attachment #780344 -
Attachment is obsolete: true
Assignee | ||
Comment 49•11 years ago
|
||
Assignee | ||
Comment 50•11 years ago
|
||
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
Whiteboard: [fixed-in-birch]
Comment 51•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 52•11 years ago
|
||
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•11 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•