We'd like to make our API more friendly by automatically connecting to multiple profiles with the class of device as basis.

I'm going to drawing an architecture diagram for these patches. Will attach it once I finish. Please hold the review requests and wait for the diagram. It should be helpful to understand what I'm doing in these patches.

This is a simple diagram to describe the current architecture and call flow.

Compared with the other attachment, "Current Architecture", this diagram describes the general idea of the proposed architecture and new call flow.

Comment on attachment 792052 [details] [diff] [review] Patch 1(v1): Implementation of BluetoothProfileController Review of attachment 792052 [details] [diff] [review]: ----------------------------------------------------------------- Gina, please see my comments below. I'll review the rest of the patch once modifications are complete. Thanks. ::: dom/bluetooth/BluetoothProfileController.cpp @@ +39,5 @@ > + profile = BluetoothHidManager::Get(); > + break; > + } > + > + NS_ENSURE_TRUE_VOID(profile); We have to deal with aRunnable once 'profile' is invalid, otherwise it won't be handled. @@ +57,5 @@ > + bool hasRendering = HAS_RENDERING(aCod); > + bool isPeripheral = IS_PERIPHERAL(aCod); > + > + NS_ENSURE_FALSE_VOID(!hasAudio && !hasObjectTransfer && > + !hasRendering && !isPeripheral); Just curious, why not NS_ENSURE_TRUE_VOID(hasAudio || hasObjectTransfer || hasRendering || isPeripheral); ? @@ +68,5 @@ > + * It's almost impossible to send file to a remote device which is an Audio > + * device or a Rendering device, so we won't connect OPP in that case. > + */ > + if (hasAudio) { > + mProfiles.AppendElement(BluetoothHfpManager::Get()); You should check if return value of BluetoothHfpManager::Get() is nullptr, just like how you did in the ctor above. Otherwise it may cause a crash in ConnectNext(). Here and below. @@ +163,5 @@ > + BT_WARNING(NS_ConvertUTF16toUTF8(aErrorStr).get()); > + } > + > + mProfilesIndex++; > + ConnectNext(); Since the function name is Connect"Next"(), it seems to be more reasonable to increase the value of mProfilesIndex in that function. ::: dom/bluetooth/BluetoothProfileController.h @@ +6,5 @@ > + > +#ifndef mozilla_dom_bluetooth_bluetoothprofilecontroller_h__ > +#define mozilla_dom_bluetooth_bluetoothprofilecontroller_h__ > + > +#include "BluetoothReplyRunnable.h" Forward declaration should be fine. @@ +24,5 @@ > + * https://www.bluetooth.org/en-us/specification/assigned-numbers/baseband > + */ > + > +// Bit 23 ~ Bit 13: Major service class > +#define GetMajorServiceClass(cod) ((cod & 0xffe000) >> 13); Please do not add a semicolon at the end of a macro. Here and below. In addition, we usually make macros all caps. @@ +33,5 @@ > +// Bit 7 ~ Bit 2: Minor device class > +#define GetMinorDeviceClass(cod) ((cod & 0xfc) >> 2); > + > +// Bit 21: Major service class = 0x100, Audio > +#define HAS_AUDIO(cod) ((cod & 0x200000) >> 21); According to the macro name, I guess we will use this macro when we need to know if a device has Audio service. If so, the only thing we care about would be the value of bit 21. So how about: #define HAS_AUDIO(cod) ((cod & 0x200000) > 0) or even #define HAS_AUDIO(cod) (cod & 0x200000) Here and below. @@ +39,5 @@ > +// Bit 20: Major service class = 0x80, Object Transfer > +#define HAS_OBJECT_TRANSFER(cod) ((cod & 0x100000) >> 20); > + > +// Bit 18: Major service class = 0x20, Rendering > +#define HAS_RENDERING(cod) ((cod & 0x40000) >> 18); super-nit: please align with other macros. @@ +45,5 @@ > +// Bit 11 and Bit 9: Major device class = 0xA, Peripheral > +#define IS_PERIPHERAL(cod) (((cod & 0x200) >> 9) && ((cod & 0x800) >> 11)); > + > +// Bit 10: Major device class = 0x4, Audio/Video > +// Bit 2: Minor device class = 0x1, Wearable Headset Device super-nit: trailing whitespace @@ +98,5 @@ > + nsTArray<BluetoothProfileManagerBase*> mProfiles; > + > + uint32_t mCod; > + nsString mDeviceAddress; > + BluetoothReplyRunnable* mRunnable; Please use nsRefPtr<BluetoothReplyRunnable> to hold a reference of mRunnable.

Thanks for reviewing. All suggestions are taken and I'll update them in the next patch. A few questions are answered below. (In reply to Eric Chou [:ericchou] [:echou] from comment #11) > @@ +57,5 @@ > > + bool hasRendering = HAS_RENDERING(aCod); > > + bool isPeripheral = IS_PERIPHERAL(aCod); > > + > > + NS_ENSURE_FALSE_VOID(!hasAudio && !hasObjectTransfer && > > + !hasRendering && !isPeripheral); > > Just curious, why not NS_ENSURE_TRUE_VOID(hasAudio || hasObjectTransfer || > hasRendering || isPeripheral); ? They are exactly the same. Either way should be fine with me. > @@ +33,5 @@ > > +// Bit 7 ~ Bit 2: Minor device class > > +#define GetMinorDeviceClass(cod) ((cod & 0xfc) >> 2); > > + > > +// Bit 21: Major service class = 0x100, Audio > > +#define HAS_AUDIO(cod) ((cod & 0x200000) >> 21); > > According to the macro name, I guess we will use this macro when we need to > know if a device has Audio service. If so, the only thing we care about > would be the value of bit 21. So how about: > > #define HAS_AUDIO(cod) ((cod & 0x200000) > 0) > > or even > > #define HAS_AUDIO(cod) (cod & 0x200000) Yes, we only care about the value of specific bit, and I prefer the latter. Will update soon.

Eric, please take a look at the updated patch. Thanks

Comment on attachment 795340 [details] [diff] [review] Patch 1(v2): Implementation of BluetoothProfileController Review of attachment 795340 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gina, I have several suggestions about the design of BluetoothProfileController. First, let's make constructors simpler. Currently you did different things in three ctors in order to initialize an environment for further use, which are actually Connnect() and Disconnect(). In my opinion, we should only take essential parameters in the ctor. In this case, they are [const nsAString& aDeviceAddress], [BluetoothReplyRunnable* aRunnable] and [BluetoothProfileControllerCallback aCallback]. Next, we need to extend the functionality of Connect() and Disconnect() since mProfiles hasn't been set yet. We can define interface like: Connect(BluetoothServiceClass aClass) / Connect(uint32_t aCod) / Disconnect(BluetoothServiceClass aClass = 0) In other words, I think overriding Connect() / Disconnect() would be better than overriding constructor. Make sense? ::: dom/bluetooth/BluetoothProfileController.cpp @@ +139,5 @@ > + > +void > +BluetoothProfileController::Init(const nsAString& aDeviceAddress, > + BluetoothReplyRunnable* aRunnable, > + BluetoothProfileControllerCallback aCallback) I think we could simplify the interface by using setter in ctor, then these parameters are no longer needed. @@ +153,5 @@ > + mProfiles.Clear(); > +} > + > +void > +BluetoothProfileController::Connect() We should directly call ConnectNext() since Connect() seems to be redundant. @@ +188,5 @@ > + > +void > +BluetoothProfileController::Disconnect() > +{ > + DisconnectNext(); We should directly call DisconnectNext() since Disconnect() seems to be redundant.

(In reply to Eric Chou [:ericchou] [:echou] from comment #21) > Next, we need to extend the functionality of Connect() and Disconnect() > since mProfiles hasn't been set yet. We can define interface like: > > Connect(BluetoothServiceClass aClass) / Connect(uint32_t aCod) / > Disconnect(BluetoothServiceClass aClass = 0) > > In other words, I think overriding Connect() / Disconnect() would be better > than overriding constructor. Make sense? Sounds good to me. Let me try and update patch later. Thanks!

Hi Eric, The patch has been updated. Please review it again. Thanks.

Comment on attachment 797129 [details] [diff] [review] Patch 1(v3): Implementation of BluetoothProfileController Review of attachment 797129 [details] [diff] [review]: ----------------------------------------------------------------- Almost there, please see my comments below. ::: dom/bluetooth/BluetoothProfileController.cpp @@ +34,5 @@ > +} > + > +BluetoothProfileController::~BluetoothProfileController() > +{ > + mProfilesIndex = -1; nit: this seems to be unnecessary. @@ +70,5 @@ > +} > + > +void > +BluetoothProfileController::Connect(BluetoothServiceClass aClass) > +{ Please add assertion to check if it's on the right thread. Here and below (Connect / Disconnect / OnConnect / OnDisconnect). @@ +89,5 @@ > + bool hasAudio = HAS_AUDIO(aCod); > + bool hasObjectTransfer = HAS_OBJECT_TRANSFER(aCod); > + bool hasRendering = HAS_RENDERING(aCod); > + bool isPeripheral = IS_PERIPHERAL(aCod); > + There are several NS_ENSURE* function calls in this function, but mRunnable would not be handled once returned early. @@ +185,5 @@ > + NS_ENSURE_TRUE_VOID(profile); > + if (profile->IsConnected()) { > + mProfiles.AppendElement(profile); > + } > + profile = BluetoothA2dpManager::Get(); I would like to suggest that moving A2DP to the top of disconnecting queue. This is because it's very common to drop A2DP connection before SLC (HFP connection) for most devices which support both HFP and A2DP. Maybe we should reverse the connecting sequence as disconnecting sequence. ::: dom/bluetooth/BluetoothProfileController.h @@ +73,5 @@ > + * corresponding profile. Otherwise, disconnect all profiles connected to the > + * remote device. > + */ > + void Disconnect(BluetoothServiceClass aClass = BluetoothServiceClass::UNKNOWN); > + nit: extra blanks

Comment on attachment 798408 [details] [diff] [review] Patch 1(v4): Implementation of BluetoothProfileController Changeset: * Add thread assertion in public functions. * Change the order of disconnect profiles. * Add function |AddProfile| and |AddProfileWithServiceClass|. In these functions, |mRunnable| is replied when we failed to get profile manager.

Comment on attachment 798408 [details] [diff] [review] Patch 1(v4): Implementation of BluetoothProfileController Review of attachment 798408 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Thanks for your effort. :) ::: dom/bluetooth/BluetoothProfileController.cpp @@ +79,5 @@ > + mCallback(); > + return false; > + } > + > + NS_ENSURE_TRUE(!aCheckConnected || aProfile->IsConnected(), false); NS_WARNING would be called inside NS_ENSURE_TRUE() once the condition checking failed, so we should use NS_ENSURE_TRUE when warning on failure cases are necessary. In this case, when Disconnect() is called, the warning won't show up only if connections for 4 profiles all exist. Thus, I prefer using common 'if' here.

Comment on attachment 797130 [details] [diff] [review] Patch 2(v1): Make parameter service UUID of Connect() optional Review of attachment 797130 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gina, please see my comments below. ::: dom/bluetooth/BluetoothSocketObserver.h @@ +31,1 @@ > Please assist with removing the extra blank. @@ +37,1 @@ > Please assist with removing the extra blank. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +2522,5 @@ > + BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid); > + > + sController = > + new BluetoothProfileController(aDeviceAddress, aRunnable, > + DestroyBluetoothProfileController); I don't think this will work. Assume that Connect() is called while sController has value, which means that the previous Connect() is still ongoing, the memory block which is held by sController will become invalid right after sController is assigned to a new BluetoothProfileController object. That may cause crash or other unexpected errors. Since we don't want to forbid users to share files and connect to a Bluetooth headset at the same time, we should solve this. ::: dom/webidl/BluetoothAdapter.webidl @@ +48,5 @@ > [GetterThrows] > readonly attribute any uuids; > > [SetterThrows] > + attribute EventHandler ondevicefound; Per comment in bug 910974, please modify the format. @@ +99,5 @@ > * > + * Note that service UUID is optional. If it isn't passed when calling > + * Connect, multiple profiles are tried sequencely based on the class of > + * device (CoD). If it isn't passed when calling Disconnect, all connected > + * profiles are going to closed. nit: to /be/ closed

(In reply to Eric Chou [:ericchou] [:echou] from comment #31) > ::: dom/bluetooth/linux/BluetoothDBusService.cpp > @@ +2522,5 @@ > > + BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid); > > + > > + sController = > > + new BluetoothProfileController(aDeviceAddress, aRunnable, > > + DestroyBluetoothProfileController); > > I don't think this will work. Assume that Connect() is called while > sController has value, which means that the previous Connect() is still > ongoing, the memory block which is held by sController will become invalid > right after sController is assigned to a new BluetoothProfileController > object. That may cause crash or other unexpected errors. > > Since we don't want to forbid users to share files and connect to a > Bluetooth headset at the same time, we should solve this. I think we can return the second DOM request directly with an error while we're processing the first request. At the same time, I'll file a follow-up to queue all requests and process them 1-by-1. What do you think?

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #32) > (In reply to Eric Chou [:ericchou] [:echou] from comment #31) > > ::: dom/bluetooth/linux/BluetoothDBusService.cpp > > @@ +2522,5 @@ > > > + BluetoothUuidHelper::GetBluetoothServiceClass(aServiceUuid); > > > + > > > + sController = > > > + new BluetoothProfileController(aDeviceAddress, aRunnable, > > > + DestroyBluetoothProfileController); > > > > I don't think this will work. Assume that Connect() is called while > > sController has value, which means that the previous Connect() is still > > ongoing, the memory block which is held by sController will become invalid > > right after sController is assigned to a new BluetoothProfileController > > object. That may cause crash or other unexpected errors. > > > > Since we don't want to forbid users to share files and connect to a > > Bluetooth headset at the same time, we should solve this. > > I think we can return the second DOM request directly with an error while > we're processing the first request. At the same time, I'll file a follow-up > to queue all requests and process them 1-by-1. What do you think? I'm fine with this. Go ahead. :)

Hi Eric, Patch has been updated based on your comment. Will file a follow-up for creating a request queue. I'll send a superreview request to Blake after your review. Thanks.

Comment on attachment 798809 [details] [diff] [review] Patch 2(v2): Make parameter service UUID of Connect() optional Review of attachment 798809 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and please remember to file a followup for the potential 'connecting at the same time' issue. Thanks.

Comment on attachment 798809 [details] [diff] [review] Patch 2(v2): Make parameter service UUID of Connect() optional Hi Blake, Please help to review this patch. Thanks.

Comment on attachment 795363 [details] [diff] [review] Patch 3(v1): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager Review of attachment 795363 [details] [diff] [review]: ----------------------------------------------------------------- ok, it's almost there. Please ask for review again once the patch is updated. Thanks. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +504,5 @@ > nsAutoString replyError; > UnpackVoidMessage(aMsg, nullptr, v, replyError); > + > + BluetoothServiceClass* serviceClass = > + static_cast<BluetoothServiceClass*>(aServiceClass); Please use nsAutoPtr<BluetoothServiceClass> to avoid manipulating memory ourselves. @@ +1930,4 @@ > } else if (aMessage.EqualsLiteral("Disconnect")) { > callback = InputDisconnectCallback; > + } else { > + BT_WARNING("Unknown sink message"); We can use MOZ_ASSERT(false) instead warnings since it's totally unreasonable to reach this point. Here and below. @@ +1950,5 @@ > + MOZ_ASSERT(aCallback); > + MOZ_ASSERT(!aObjectPath.IsEmpty()); > + MOZ_ASSERT(aInterface); > + > + BluetoothServiceClass* serviceClass = new BluetoothServiceClass(); Please use nsAutoPtr<BluetoothServiceClass> to avoid manipulating memory ourselves. @@ +3158,5 @@ > > static void > ControlCallback(DBusMessage* aMsg, void* aParam) > { > + if (!aMsg) { nit: NS_ENSURE_TRUE_VOID(aMsg) @@ +3166,5 @@ > + > + BluetoothValue v; > + nsAutoString replyError; > + UnpackVoidMessage(aMsg, nullptr, v, replyError); > + if(!v.get_bool()) { nit: need a blank between if and '('

nit picked.

Comment on attachment 799259 [details] [diff] [review] Patch 3(v2): Remove BluetoothReplyRunnable in SendInputMessage. Implement SendAsyncDBusMessage, CheckDBusReply, and ReplyErrorToProfileManager Review of attachment 799259 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +509,5 @@ > + profile = BluetoothHidManager::Get(); > + } else if (mServiceClass == BluetoothServiceClass::A2DP) { > + profile = BluetoothA2dpManager::Get(); > + } else { > + BT_WARNING("Invalid profile"); Use MOZ_ASSERT(false) for consistency. @@ +549,3 @@ > } > + > + delete serviceClass; Please don't call delete here. serviceClass will delete the memory it holds after leaving this function.. @@ +1982,4 @@ > bool ret = dbus_func_args_async(mConnection, > -1, > + aCallback, > + static_cast<void*>(serviceClass), You have to use serviceClass.forget() here, or the memory would become invalid after leaving this function.

(In reply to Eric Chou [:ericchou] [:echou] from comment #39) > > + delete serviceClass; > > Please don't call delete here. serviceClass will delete the memory it holds > after leaving this function.. > > @@ +1982,4 @@ > > bool ret = dbus_func_args_async(mConnection, > > -1, > > + aCallback, > > + static_cast<void*>(serviceClass), > > You have to use serviceClass.forget() here, or the memory would become > invalid after leaving this function. My bad... I'll cover them in the final patch.

Comment on attachment 798809 [details] [diff] [review] Patch 2(v2): Make parameter service UUID of Connect() optional Review of attachment 798809 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/BluetoothAdapter.webidl @@ +97,5 @@ > * To check the value of service UUIDs, please check "Bluetooth Assigned > * Numbers" / "Service Discovery Protocol" for more information. > * > + * Note that service UUID is optional. If it isn't passed when calling > + * Connect, multiple profiles are tried sequencely based on the class of "sequentially"

Comment on attachment 795364 [details] [diff] [review] Patch 4(v1): Fix BluetoothHfpManager Review of attachment 795364 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gina, the main reason of r- is I think we should move OnConnect/OnDisconnect from interface BluetoothProfileManagerBase to each Bluetooth*Manager. Please see my comments below. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +1098,2 @@ > { > + Please add an assertion here to check current thread. ::: dom/bluetooth/BluetoothHfpManager.h @@ +7,5 @@ > #ifndef mozilla_dom_bluetooth_bluetoothhfpmanager_h__ > #define mozilla_dom_bluetooth_bluetoothhfpmanager_h__ > > #include "BluetoothCommon.h" > +#include "BluetoothProfileController.h" Forward declaration of BluetoothProfileController should be fine. @@ +152,5 @@ > > nsTArray<Call> mCurrentCallArray; > nsAutoPtr<BluetoothTelephonyListener> mListener; > nsRefPtr<BluetoothReplyRunnable> mRunnable; > + BluetoothProfileController* mController; Please use nsAutoPtr<BluetoothProfileController> ::: dom/bluetooth/BluetoothProfileManagerBase.h @@ +59,5 @@ > + * If it establishes/releases a connection successfully, the error string > + * will be empty. Otherwise, the error string shows the failure reason. > + */ > + virtual void OnConnect(const nsAString& aErrorStr) = 0; > + virtual void OnDisconnect(const nsAString& aErrorStr) = 0; I may want to remove these two functions from interface BluetoothProfileManagerBase. It's weird to define a virtual function which will not be called from outside, but in another pure virtual function which is also implemented in the same class. Since OnConnect/OnDisconnect have already been implemented in BluetoothProfileController, I think we can change these two into local functions.

(In reply to Blake Kaplan (:mrbkap) [PTO Sep 5-Sep 19] from comment #41) > Comment on attachment 798809 [details] [diff] [review] > Patch 2(v2): Make parameter service UUID of Connect() optional > > Review of attachment 798809 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/BluetoothAdapter.webidl > @@ +97,5 @@ > > * To check the value of service UUIDs, please check "Bluetooth Assigned > > * Numbers" / "Service Discovery Protocol" for more information. > > * > > + * Note that service UUID is optional. If it isn't passed when calling > > + * Connect, multiple profiles are tried sequencely based on the class of > > "sequentially" Thanks for your review, Blake. :)

(In reply to Eric Chou [:ericchou] [:echou] from comment #42) > ::: dom/bluetooth/BluetoothProfileManagerBase.h > @@ +59,5 @@ > > + * If it establishes/releases a connection successfully, the error string > > + * will be empty. Otherwise, the error string shows the failure reason. > > + */ > > + virtual void OnConnect(const nsAString& aErrorStr) = 0; > > + virtual void OnDisconnect(const nsAString& aErrorStr) = 0; > > I may want to remove these two functions from interface > BluetoothProfileManagerBase. It's weird to define a virtual function which > will not be called from outside, but in another pure virtual function which > is also implemented in the same class. Since OnConnect/OnDisconnect have > already been implemented in BluetoothProfileController, I think we can > change these two into local functions. These functions are used in Patch 3 for error handling. For BluetoothA2dpManager and BluetoothHidManager, we might fail to establish/release connection with an error string returned, and OnConnect/OnDisconnect is invoked in this case.

Comment on attachment 795364 [details] [diff] [review] Patch 4(v1): Fix BluetoothHfpManager Review of attachment 795364 [details] [diff] [review]: ----------------------------------------------------------------- ok, r+ per comment 44 and discussion with Gina, Shawn and Ben. Please still pick other nits mentioned in comment 42. Thank you.

Comment on attachment 795360 [details] [diff] [review] Patch 5(v1): Fix BluetoothOppManager Review of attachment 795360 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothOppManager.h @@ +214,5 @@ > nsCOMPtr<nsIOutputStream> mOutputStream; > nsCOMPtr<nsIInputStream> mInputStream; > nsCOMPtr<nsIVolumeMountLock> mMountLock; > nsRefPtr<BluetoothReplyRunnable> mRunnable; > + BluetoothProfileController* mController; Please use nsAutoPtr<BluetoothProfileController>. In addition, BluetoothProfileController should be declared somewhere in this header file or it will cause build break, however I don't see related code.

(In reply to Eric Chou [:ericchou] [:echou] from comment #46) > Comment on attachment 795360 [details] [diff] [review] > Patch 5(v1): Fix BluetoothOppManager > > Review of attachment 795360 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/BluetoothOppManager.h > @@ +214,5 @@ > > nsCOMPtr<nsIOutputStream> mOutputStream; > > nsCOMPtr<nsIInputStream> mInputStream; > > nsCOMPtr<nsIVolumeMountLock> mMountLock; > > nsRefPtr<BluetoothReplyRunnable> mRunnable; > > + BluetoothProfileController* mController; > > Please use nsAutoPtr<BluetoothProfileController>. In addition, > BluetoothProfileController should be declared somewhere in this header file > or it will cause build break, however I don't see related code. Wait, I found that BluetoothProfileController can't be held by nsAutoPtr in each Bleutooth*Manager because the BluetoothProfileController instance will be killed after mController set to nullptr. Therefore please ignore my comment in comment 42 and comment 46. It's ok to keep it raw pointer for now, however I think it would be better if we could make BluetoothProfileController reference counted.

Comment on attachment 798437 [details] [diff] [review] Patch 6(v1): Fix BluetoothA2dpManager Review of attachment 798437 [details] [diff] [review]: ----------------------------------------------------------------- r=me with questions answered. ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +255,5 @@ > + aSignal.value().get_ArrayOfBluetoothNamedValue(); > + MOZ_ASSERT(arr.Length() == 1); > + > + const nsString& name = arr[0].name(); > + NS_ENSURE_TRUE_VOID(name.EqualsLiteral("State")); Question: This means that we will only process the PropertyChanged event of attribute "State" and ignore "Connected" and "Playing". Is that what we want? If so, please leave a comment here. Thanks. @@ +278,5 @@ > + // case 5: Audio stream suspended > + } else if (mSinkState == SinkState::SINK_DISCONNECTED && > + prevState == SinkState::SINK_CONNECTING) { > + // case 2: Connection attempt failed > + OnConnect(EmptyString()); Question: Does it make sense to call OnConnect() without error string on connecting failed case? @@ +305,5 @@ > + // case 7: Disconnected from local > + if (prevState == SinkState::SINK_DISCONNECTING) { > + OnDisconnect(EmptyString()); > + } > + } Suggestion: Since there are only 5 states, I would probably check mSinkState first, then all unreasonable cases could be handled in each if/else-if blocks.

Thanks, Eric :)

(In reply to Eric Chou [:ericchou] [:echou] from comment #48) > ::: dom/bluetooth/BluetoothA2dpManager.cpp > @@ +255,5 @@ > > + aSignal.value().get_ArrayOfBluetoothNamedValue(); > > + MOZ_ASSERT(arr.Length() == 1); > > + > > + const nsString& name = arr[0].name(); > > + NS_ENSURE_TRUE_VOID(name.EqualsLiteral("State")); > > Question: This means that we will only process the PropertyChanged event of > attribute "State" and ignore "Connected" and "Playing". Is that what we > want? If so, please leave a comment here. Thanks. Yep! I will add a comment here. > @@ +278,5 @@ > > + // case 5: Audio stream suspended > > + } else if (mSinkState == SinkState::SINK_DISCONNECTED && > > + prevState == SinkState::SINK_CONNECTING) { > > + // case 2: Connection attempt failed > > + OnConnect(EmptyString()); > > Question: Does it make sense to call OnConnect() without error string on > connecting failed case? Agree. We should return an error string in this case.

try: https://tbpl.mozilla.org/?tree=Try&rev=faf8c3653943 https://tbpl.mozilla.org/?tree=Try&rev=f639cd256c53

https://hg.mozilla.org/integration/b2g-inbound/rev/ffd9e8e157dd https://hg.mozilla.org/integration/b2g-inbound/rev/ef9b729c38a1 https://hg.mozilla.org/integration/b2g-inbound/rev/585da14f78e2 https://hg.mozilla.org/integration/b2g-inbound/rev/a79e09ef9e0b https://hg.mozilla.org/integration/b2g-inbound/rev/066ddde89edf https://hg.mozilla.org/integration/b2g-inbound/rev/141a21a230d2 https://hg.mozilla.org/integration/b2g-inbound/rev/abaf0fa5fd0d

https://hg.mozilla.org/mozilla-central/rev/ffd9e8e157dd https://hg.mozilla.org/mozilla-central/rev/ef9b729c38a1 https://hg.mozilla.org/mozilla-central/rev/585da14f78e2 https://hg.mozilla.org/mozilla-central/rev/a79e09ef9e0b https://hg.mozilla.org/mozilla-central/rev/066ddde89edf https://hg.mozilla.org/mozilla-central/rev/141a21a230d2 https://hg.mozilla.org/mozilla-central/rev/abaf0fa5fd0d