Comment on attachment 750313 [details] [diff] [review] Patch 1(v1): Implement BluetoothA2dpManager Review of attachment 750313 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +70,5 @@ > +}; > + > +NS_IMETHODIMP > +BluetoothA2dpManagerObserver::Observe(nsISupports* aSubject, > + const char* aTopic, super-nit: align with other parameters @@ +154,5 @@ > +} > + > +bool > +BluetoothA2dpManager::Connect(const nsAString& aDeviceAddress) > +{ nit: add assertion to make sure aDeviceAddress is valid (or at least make sure it's not empty). @@ +171,5 @@ > + BluetoothService* bs = BluetoothService::Get(); > + NS_ENSURE_TRUE(bs, false); > + nsresult rv = bs->SendSinkMessage(aDeviceAddress, NS_LITERAL_STRING("Connect")); > + > + mDeviceAddress = aDeviceAddress; nit: This assignment seems a little strange to be put here. If mDeviceAddress would be assigned even if rv != NS_OK, then I would say let's move it in front of the SendSinkMessage() line. @@ +178,5 @@ > + > +void > +BluetoothA2dpManager::Disconnect() > +{ > + if (!mConnected) { Here we check |mConnected| to see if A2DP connection exists, but we use |if (mSinkState == SinkState::SINK_CONNECTED)| in BluetoothA2dpManager::Connect(). Seems a little inconsistent? @@ +185,5 @@ > + } > + > + BluetoothService* bs = BluetoothService::Get(); > + NS_ENSURE_TRUE_VOID(bs); > + bs->SendSinkMessage(mDeviceAddress, NS_LITERAL_STRING("Disconnect")); nit: add assertion to make sure aDeviceAddress is valid (or at least make sure it's not empty).

Comment on attachment 750314 [details] [diff] [review] Patch 2(v1): Implement SendSinkMessage in BluetoothService Please help to give review or give some feedback, thanks.

Comment on attachment 750315 [details] [diff] [review] Patch 3(v1): Handle Sink property change Please help to give review or give some feedback, thanks.

Comment on attachment 750316 [details] [diff] [review] Patch 4(v1): Implement NotifyStatusChagned() and NotifyAudioManager() Please help to give review or give some feedback, thanks.

Comment on attachment 750317 [details] [diff] [review] Patch 5(v1): Handle A2DP status changed in AudioManager Please help to give review or give some feedback, thanks.

Comment on attachment 750318 [details] [diff] [review] Patch 6(v1): Connect/Disconnect A2dp after Hfp is connected/disconnected Please help to give review or give some feedback, thanks.

Comment on attachment 750314 [details] [diff] [review] Patch 2(v1): Implement SendSinkMessage in BluetoothService Review of attachment 750314 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! There are some small places need to be revised. Please have me review again once you're done. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +959,5 @@ > UnpackIntMessage); > } > > +void > +RunSinkCallback(bool aIsConnected, I have some thoughts about the naming of this argument. In general, we often call an argument "IsConnected" when it represents status of a connection. The argument here indicates that "the original received request is [to connect/disconnect with remote device]". So, I think maybe just using "aConnect" would be more reasonable. @@ +975,5 @@ > + if (!v.get_bool()) { > + if (aIsConnected) { > + NS_WARNING("Failed to connect sink."); > + return; > + } nit: trailing space @@ +977,5 @@ > + NS_WARNING("Failed to connect sink."); > + return; > + } > + NS_WARNING("Failed to disconnect sink."); > + } You can wrap this if-block in a #ifdef DEBUG since we won't do anything except printing out warnings even if v.get_bool() is false. #ifdef DEBUG if (!v.get_bool()) { if (aIsConnected) { NS_WARNING("Failed to connect sink."); } else { NS_WARNING("Failed to disconnect sink."); } } #endif @@ +1880,5 @@ > + > + // Create objectPath for further use (in callback funciton) > + nsString path = GetObjectPathFromAddress(sAdapterPath, aDeviceAddress); > + char* objectPath = new char[path.Length() + 1]; > + strcpy(objectPath, NS_ConvertUTF16toUTF8(path).get()); You can just |char* objectPath = ToNewCString(path);|

Comment on attachment 750313 [details] [diff] [review] Patch 1(v1): Implement BluetoothA2dpManager Review of attachment 750313 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +70,5 @@ > +}; > + > +NS_IMETHODIMP > +BluetoothA2dpManagerObserver::Observe(nsISupports* aSubject, > + const char* aTopic, nit picked. @@ +154,5 @@ > +} > + > +bool > +BluetoothA2dpManager::Connect(const nsAString& aDeviceAddress) > +{ nit picked. @@ +171,5 @@ > + BluetoothService* bs = BluetoothService::Get(); > + NS_ENSURE_TRUE(bs, false); > + nsresult rv = bs->SendSinkMessage(aDeviceAddress, NS_LITERAL_STRING("Connect")); > + > + mDeviceAddress = aDeviceAddress; Agree. @@ +178,5 @@ > + > +void > +BluetoothA2dpManager::Disconnect() > +{ > + if (!mConnected) { Agree. We should check the value |mConnected| in Connect(). @@ +185,5 @@ > + } > + > + BluetoothService* bs = BluetoothService::Get(); > + NS_ENSURE_TRUE_VOID(bs); > + bs->SendSinkMessage(mDeviceAddress, NS_LITERAL_STRING("Disconnect")); nit picked

Comment on attachment 750314 [details] [diff] [review] Patch 2(v1): Implement SendSinkMessage in BluetoothService Review of attachment 750314 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +959,5 @@ > UnpackIntMessage); > } > > +void > +RunSinkCallback(bool aIsConnected, Agree. @@ +975,5 @@ > + if (!v.get_bool()) { > + if (aIsConnected) { > + NS_WARNING("Failed to connect sink."); > + return; > + } nit picked. @@ +977,5 @@ > + NS_WARNING("Failed to connect sink."); > + return; > + } > + NS_WARNING("Failed to disconnect sink."); > + } Wrap them with #ifdef DEBUG is a good idea. @@ +1880,5 @@ > + > + // Create objectPath for further use (in callback funciton) > + nsString path = GetObjectPathFromAddress(sAdapterPath, aDeviceAddress); > + char* objectPath = new char[path.Length() + 1]; > + strcpy(objectPath, NS_ConvertUTF16toUTF8(path).get()); This part is revised in the second version. Please see new patch for more details.

Comment on attachment 750315 [details] [diff] [review] Patch 3(v1): Handle Sink property change Review of attachment 750315 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good! As usual, I made some suggestions for you and please have me review again whenever you're ready. Thanks. ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +132,5 @@ > + state = SINK_PLAYING; > + } else if (aStatus.EqualsLiteral("disconnecting")) { > + state = SINK_DISCONNECTING; > + } else { > + NS_WARNING("Unknown sink status"); I suggest that we should use assertion here. It must be something very wrong if the program goes here. @@ +232,5 @@ > + } else if (name.EqualsLiteral("State")) { > + MOZ_ASSERT(value.type() == BluetoothValue::TnsString); > + SinkState state = StatusStringToSinkState(value.get_nsString()); > + if (state) { > + HandleSinkStateChanged(state); I would like to suggest that we should just pass |state| into HandleSinkStateChanged() without checking |if (state)|. Then you could just write: HandleSinkStateChanged(StatusStringToSinkState(value.get_nsString())); The main reason to avoid using |if (state)| is that, it's not easy for readers to understand what does "state == 0" means at a glance. It could be an invalid value (this case), or it might be something meaningful like DISCONNECTED. Therefore I think either writing it more clear or just let HandleSinkStateChanged() to decide how to deal with it would be better. @@ +261,5 @@ > + * Disconnected from local > + */ > +void > +BluetoothA2dpManager::HandleSinkStateChanged(SinkState aState) > +{ This function looks very complicated but it actually does very little stuff. Did I miss anything? :| If this is what you're going to do in prototype, maybe you could make it simpler now and modify in another bug later on. Actually it could be only: MOZ_ASSERT_IF(aState == SinkState::SINK_PLAYING, prevSinkState == SinkState::SINK_CONNECTED); if (aState == SinkState::SINK_DISCONNECTED) { mDeviceAddress.Truncate(); } ::: dom/bluetooth/BluetoothA2dpManager.h @@ +40,5 @@ > nsresult HandleShutdown(); > bool Init(); > void Cleanup(); > > + enum SinkState StatusStringToSinkState(const nsAString& aStatus); We can make this function static since there's no member variables using inside the function.

Comment on attachment 750315 [details] [diff] [review] Patch 3(v1): Handle Sink property change Review of attachment 750315 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +132,5 @@ > + state = SINK_PLAYING; > + } else if (aStatus.EqualsLiteral("disconnecting")) { > + state = SINK_DISCONNECTING; > + } else { > + NS_WARNING("Unknown sink status"); Agree. @@ +232,5 @@ > + } else if (name.EqualsLiteral("State")) { > + MOZ_ASSERT(value.type() == BluetoothValue::TnsString); > + SinkState state = StatusStringToSinkState(value.get_nsString()); > + if (state) { > + HandleSinkStateChanged(state); Good suggestion. @@ +261,5 @@ > + * Disconnected from local > + */ > +void > +BluetoothA2dpManager::HandleSinkStateChanged(SinkState aState) > +{ Sounds good. Let me update patch. ::: dom/bluetooth/BluetoothA2dpManager.h @@ +40,5 @@ > nsresult HandleShutdown(); > bool Init(); > void Cleanup(); > > + enum SinkState StatusStringToSinkState(const nsAString& aStatus); Suggestion is taken.

Comment on attachment 752651 [details] [diff] [review] Patch 1(v2): Implement BluetoothA2dpManager Review of attachment 752651 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +184,5 @@ > + > + if (!mConnected) { > + NS_WARNING("BluetoothA2dpManager has been disconnected"); > + return; > + } You may want to simplify this kind of if-blocks which are only for printing out warnings by using NS_ENSURE_TRUE_VOID(), NS_ENSURE_TRUE().

Comment on attachment 752652 [details] [diff] [review] Patch 2(v2): Implement SendSinkMessage in BluetoothService Review of attachment 752652 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +958,5 @@ > RunDBusCallback(aMsg, aBluetoothReplyRunnable, > UnpackIntMessage); > } > > +void static would be better @@ +959,5 @@ > UnpackIntMessage); > } > > +void > +RunSinkCallback(bool aConnect, You need to wrap this whole function in #DEBUG as well, or compiler may complain this function is unused in release build. @@ +961,5 @@ > > +void > +RunSinkCallback(bool aConnect, > + DBusMessage* aMsg, > + void *aParam) nit: one line for these arguments should be enough. @@ +975,5 @@ > + BT_WARNING("Failed to disconnect sink."); > + } > +} > + > +void static would be better @@ +983,5 @@ > + RunSinkCallback(true, aMsg, aParam); > +#endif > +} > + > +void static would be better @@ +1877,5 @@ > + bool ret = dbus_func_args_async(mConnection, > + -1, > + callback, > + nullptr, > + objectPath, I didn't see any variable called |objectPath| declared in the function. :|

Comment on attachment 753070 [details] [diff] [review] Patch 3(v2): Handle Sink property change Review of attachment 753070 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +128,5 @@ > + state = SinkState::SINK_CONNECTING; > + } else if (aStatus.EqualsLiteral("connected")) { > + state = SinkState::SINK_CONNECTED; > + } else if (aStatus.EqualsLiteral("playing")) { > + state = SINK_PLAYING; For consistency, please use SinkState::SINK_PLAYING @@ +130,5 @@ > + state = SinkState::SINK_CONNECTED; > + } else if (aStatus.EqualsLiteral("playing")) { > + state = SINK_PLAYING; > + } else if (aStatus.EqualsLiteral("disconnecting")) { > + state = SINK_DISCONNECTING; Same as above, for consistency, please use SinkState::SINK_DISCONNECTING ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +1925,5 @@ > bool ret = dbus_func_args_async(mConnection, > -1, > callback, > nullptr, > + NS_ConvertUTF16toUTF8(aDeviceAddress).get(), Apparently we should put this modification in patch 2. :)

Comment on attachment 750316 [details] [diff] [review] Patch 4(v1): Implement NotifyStatusChagned() and NotifyAudioManager() Review of attachment 750316 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +313,5 @@ > + > +void > +BluetoothA2dpManager::NotifyStatusChanged() > +{ > + MOZ_ASSERT(NS_IsMainThread()); You may want to add MOZ_ASSERT(!mDeviceAddress.IsEmpty()) @@ +329,5 @@ > + parameters.AppendElement(BluetoothNamedValue(name, v)); > + > + if (!BroadcastSystemMessage(type, parameters)) { > + NS_WARNING("Failed to broadcast system message to settings"); > + return; nit: unnecessary return @@ +336,5 @@ > + > +void > +BluetoothA2dpManager::NotifyAudioManager() > +{ > + MOZ_ASSERT(NS_IsMainThread()); You may want to add MOZ_ASSERT(!mDeviceAddress.IsEmpty())

Comment on attachment 752652 [details] [diff] [review] Patch 2(v2): Implement SendSinkMessage in BluetoothService Review of attachment 752652 [details] [diff] [review]: ----------------------------------------------------------------- Suggestions are all taken. Thanks!

Make other functions in BluetoothDBusService.cpp static as well and remove unused functions.

Comment on attachment 753688 [details] [diff] [review] Patch 2(v3): Implement SendSinkMessage in BluetoothService Review of attachment 753688 [details] [diff] [review]: ----------------------------------------------------------------- Almost done! Please see my review comments and I believe that the next revision will be the final one. :) ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +976,5 @@ > + BT_WARNING("Failed to disconnect sink."); > + } > +} > + > +#ifdef DEBUG Ah, in the last comment I said we should wrap RunSinkCallback in DEBUG, not SinkConnectCallback. Otherwise it would cause build error in release mode. @@ +1850,5 @@ > + bool ret = dbus_func_args_async(mConnection, > + -1, > + callback, > + nullptr, > + objectPath, Still don't see objectPath declared in this patch. Although I know this would be fixed in patch 3, we should move that line here to make it more clear.

Comment on attachment 750317 [details] [diff] [review] Patch 5(v1): Handle A2DP status changed in AudioManager Review of attachment 750317 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Please have me review again once the revised patch is done. Thanks. ::: dom/system/gonk/AudioManager.cpp @@ +161,5 @@ > + begin = i + 1; > + } > + } > + nsCString tmp(nsDependentCSubstring(aMessage, begin)); > + commands.AppendElement(tmp); I would like to provide two suggestions: 1. Using FindChar(), so you may want to use |while| instead of |for|. 2. Using function |Substring()| instead of |nsDependentCSubstring()| because it's shorter. :) nsTArray<nsCString> commands; int startPos = 0; int endPos = aMessage.FindChar(','); while (endPos != -1) { commands.AppendElement(Substring(aMessage, startPos, endPos - startPos)); startPos = endPos + 1; endPos = aMessage.FindChar(',', startPos); } commands.AppendElement(Substring(aMessage, startPos)); @@ +174,5 @@ > + NS_WARNING("Wrong format in BluetoothStatusChanged message"); > + return NS_ERROR_FAILURE; > + } > + > + nsCString key(nsDependentCSubstring(command, 0, pos)); Same as above. Replace nsDependentCSubstring with Substring. @@ +175,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + nsCString key(nsDependentCSubstring(command, 0, pos)); > + nsCString value(nsDependentCSubstring(command, pos + 1, command.Length())); The third argument should be the lenght of substring, so I don't think command.Length() is what you want. Actually, to get |value|, you don't even need the third argument. @@ +181,5 @@ > + if (value.EqualsLiteral("true")) { > + *aRetStatus = 1; > + } else if (value.EqualsLiteral("false")) { > + *aRetStatus = 0; > + } else { nit: trailing whitespaces @@ +186,5 @@ > + NS_WARNING("Unknown value in BluetoothStatusChanged message"); > + return NS_ERROR_FAILURE; > + } > + } else if (key.EqualsLiteral("address")) { > + aRetAddress = value; nit: trailing whitespace @@ +205,5 @@ > + bool status; > + nsCString address; > + nsCString data = NS_ConvertUTF16toUTF8(aData); > + nsresult rv = ParseBluetoothStatusChagnedMessage(data, address, &status); > + if (NS_FAILED(rv)) { nit: trailing whitespace nit: rv is not necessary. NS_FAILED(ParseBluetoothStatusChagnedMessage()) should be fine. @@ +240,5 @@ > + String8 cmd("bluetooth_enabled=true"); > + AudioSystem::setParameters(0, cmd); > + cmd.setTo("A2dpSuspended=false"); > + AudioSystem::setParameters(0, cmd); > + } Question: do we need to handle the situation when status is false? Or it should be ok since we've already set audioState to AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE?

Comment on attachment 756413 [details] [diff] [review] Patch 2(v4): Implement SendSinkMessage in BluetoothService, r=echou Fixed as comments.

Comment on attachment 756416 [details] [diff] [review] Patch 4(v2): Implement NotifyStatusChagned() and NotifyAudioManager() Review of attachment 756416 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit picked. Please don't forget to rebase on top of current codebase. ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +325,5 @@ > +void > +BluetoothA2dpManager::OnGetServiceChannel(const nsAString& aDeviceAddress, > + const nsAString& aServiceUuid, > + int aChannel) > +{ nit: please leave comment like 'do nothing' to explicitly tell people that we implement this just because we have to.

Comment on attachment 756417 [details] [diff] [review] Patch 5(v2): Handle A2DP status changed in AudioManager Review of attachment 756417 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me in terms of Bluetooth related logic. Please ask AudioManager owner for another review. Thanks. ::: dom/system/gonk/AudioManager.cpp @@ +170,3 @@ > if (!strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED)) { > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); nit: 80 chars per line @@ +170,5 @@ > if (!strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED)) { > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_IN_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); nit: 80 chars per line @@ +185,5 @@ > SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_NONE); > } > + } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED)) { > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); nit: 80 chars per line @@ +191,5 @@ > + String8 cmd("bluetooth_enabled=true"); > + AudioSystem::setParameters(0, cmd); > + cmd.setTo("A2dpSuspended=false"); > + AudioSystem::setParameters(0, cmd); > + } Question: when do we need to set A2dpSuspended to true?

Hi mrbkap, could you help to review these patches? I'd like to get some comments from you. Thanks. :)

Comment on attachment 756417 [details] [diff] [review] Patch 5(v2): Handle A2DP status changed in AudioManager Hi mwu and randy, please help to review AudioManager part. Let me know if you have any concern. Thanks.

Comment on attachment 756417 [details] [diff] [review] Patch 5(v2): Handle A2DP status changed in AudioManager Review of attachment 756417 [details] [diff] [review]: ----------------------------------------------------------------- Randy is on PTO today, so I help to do it first. ::: dom/system/gonk/AudioManager.cpp @@ +146,4 @@ > AudioManager::Observe(nsISupports* aSubject, > const char* aTopic, > const PRUnichar* aData) > { I think it would be better to check whether aSubject is for BT or not then doing the following thing. @@ +158,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + BluetoothProfileManagerBase* profile = > + static_cast<BluetoothProfileManagerBase*>(aSubject); Is it better to use "nsCOMPtr<XXX> XXX = do_QueryInterface(aSubject);" then check the result? @@ +164,5 @@ > + > + audio_policy_dev_state_t audioState = AUDIO_POLICY_DEVICE_STATE_AVAILABLE; > + if (!status) { > + audioState = AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE; > + } audio_policy_dev_state_t audioState = status ? AUDIO_POLICY_DEVICE_STATE_AVAILABLE : AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE; @@ +171,5 @@ > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_IN_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); > + if (status) { Maybe to use audioState here will be more clear on why the commands as below need to do.

Hi Marco, Sounds reasonable to me. Will update in next version. Thanks.

Comment on attachment 756417 [details] [diff] [review] Patch 5(v2): Handle A2DP status changed in AudioManager Review of attachment 756417 [details] [diff] [review]: ----------------------------------------------------------------- This patch LGTM after mchen suggest. :) ::: dom/system/gonk/AudioManager.cpp @@ +253,5 @@ > nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > if (NS_FAILED(obs->RemoveObserver(this, BLUETOOTH_SCO_STATUS_CHANGED))) { > NS_WARNING("Failed to add bluetooth-sco-status-changed oberver!"); > + } else if (NS_FAILED(obs->RemoveObserver(this, BLUETOOTH_A2DP_STATUS_CHANGED))) { > + NS_WARNING("Failed to add bluetooth-a2dp-status-changed oberver!"); Failed to add (remove)

Thanks, Randy. Will fix in the next patch.

(In reply to Marco Chen [:mchen] from comment #39) > Comment on attachment 756417 [details] [diff] [review] > > @@ +158,5 @@ > > + return NS_ERROR_FAILURE; > > + } > > + > > + BluetoothProfileManagerBase* profile = > > + static_cast<BluetoothProfileManagerBase*>(aSubject); > > Is it better to use "nsCOMPtr<XXX> XXX = do_QueryInterface(aSubject);" then > check the result? Marco, |aSubject| is a pointer to a concrete class (BluetoothProfileManagerBase) rather then an XPCOM interface pointer. So |aSubject| should be cast to a nsRefPtr, right? Is there anyway to do this instead of using static_cast?

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #43) > Marco, |aSubject| is a pointer to a concrete class > (BluetoothProfileManagerBase) rather then an XPCOM interface pointer. So > |aSubject| should be cast to a nsRefPtr, right? Is there anyway to do this > instead of using static_cast? Ok, if it is not a XPCOM Component then just do it by static_cast is enough. But don't forget to check whether aSubject is a nullptr or not. Thanks.

Comment on attachment 757859 [details] [diff] [review] Patch 5(v3): Handle A2DP status changed in AudioManager Review of attachment 757859 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for addressing the comments. And please fix others in this round then passing the review to Michael. ::: dom/system/gonk/AudioManager.cpp @@ +170,5 @@ > + > + audio_policy_dev_state_t audioState = status > + ? AUDIO_POLICY_DEVICE_STATE_AVAILABLE > + : AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE; > + } Is this a redundant bracket? @@ +259,5 @@ > nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > if (NS_FAILED(obs->RemoveObserver(this, BLUETOOTH_SCO_STATUS_CHANGED))) { > NS_WARNING("Failed to add bluetooth-sco-status-changed oberver!"); > + } else if (NS_FAILED(obs->RemoveObserver(this, BLUETOOTH_A2DP_STATUS_CHANGED))) { > + NS_WARNING("Failed to add bluetooth-a2dp-status-changed oberver!"); Replace "add" by "remove" in comments of SCO & A2DP.

Oops! This patches isn't The latest version. Let me update it tmw.

Updated as feedback from Marck and Randy.

Disconnect A2DP when we actively disconnect HFP.

Comment on attachment 758365 [details] [diff] [review] Patch 5(v4): Handle A2DP status changed in AudioManager, r=echou Review of attachment 758365 [details] [diff] [review]: ----------------------------------------------------------------- It looks good to me. Thanks.

Comment on attachment 758365 [details] [diff] [review] Patch 5(v4): Handle A2DP status changed in AudioManager, r=echou Review of attachment 758365 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/AudioManager.cpp @@ +147,5 @@ > const char* aTopic, > const PRUnichar* aData) > { > + if ((strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED) != 0) && > + (strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED) != 0)) { Marco has a patch which adds an additional topic which isn't bluetooth related in bug 876334, so this might not work out too well. @@ +152,5 @@ > + NS_WARNING("Unexpected topic in AudioManager"); > + return NS_ERROR_FAILURE; > + } > + > + nsString address; nsAutoString is probably a bit better here. @@ +158,5 @@ > + int status = NS_ConvertUTF16toUTF8(aData).ToInteger(&rv); > + if (NS_FAILED(rv) || status > 1 || status < 0) { > + nsCString errorMsg; > + errorMsg.Append("Wrong data value of "); > + errorMsg.Append(aopic); aopic looks like a typo I think you can use nsPrintfCString to make this prettier. @@ +169,5 @@ > + profile->GetAddress(address); > + > + audio_policy_dev_state_t audioState = status > + ? AUDIO_POLICY_DEVICE_STATE_AVAILABLE > + : AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE; nit: I think it's more common to put ? and : at the end of the line instead of the beginning. @@ +175,5 @@ > if (!strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED)) { > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_IN_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); Might be better to have a separate variable store the utf8 version of address. Also, get() is more commonly used than BeginReading().

Comment on attachment 758365 [details] [diff] [review] Patch 5(v4): Handle A2DP status changed in AudioManager, r=echou Review of attachment 758365 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your review, mwu. Will update patch and send review request again. ::: dom/system/gonk/AudioManager.cpp @@ +147,5 @@ > const char* aTopic, > const PRUnichar* aData) > { > + if ((strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED) != 0) && > + (strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED) != 0)) { Discussed with Marco. He'll rebase his patch based on this. @@ +152,5 @@ > + NS_WARNING("Unexpected topic in AudioManager"); > + return NS_ERROR_FAILURE; > + } > + > + nsString address; Will fix in the next patch. @@ +158,5 @@ > + int status = NS_ConvertUTF16toUTF8(aData).ToInteger(&rv); > + if (NS_FAILED(rv) || status > 1 || status < 0) { > + nsCString errorMsg; > + errorMsg.Append("Wrong data value of "); > + errorMsg.Append(aopic); Cool. Let me try it. @@ +169,5 @@ > + profile->GetAddress(address); > + > + audio_policy_dev_state_t audioState = status > + ? AUDIO_POLICY_DEVICE_STATE_AVAILABLE > + : AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE; Will fix in the next patch. @@ +175,5 @@ > if (!strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED)) { > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); > + AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_IN_BLUETOOTH_SCO_HEADSET, > + audioState, NS_ConvertUTF16toUTF8(address).BeginReading()); Will fix in the next patch.

Updated as comments in previous patch.

Comment on attachment 756411 [details] [diff] [review] Patch 1(v2): Implement BluetoothA2dpManager, r=echou Review of attachment 756411 [details] [diff] [review]: ----------------------------------------------------------------- Please address my questions before checking in. As they're pretty minor stylistic things, marking r+. ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +23,5 @@ > +using namespace mozilla; > +USING_BLUETOOTH_NAMESPACE > + > +namespace { > + StaticAutoPtr<BluetoothA2dpManager> gBluetoothA2dpManager; Given that the manager is a singleton, could you just have the observer be a member instead of a static variable? @@ +99,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + sA2dpObserver = new BluetoothA2dpManagerObserver(); > + if (!sA2dpObserver->Init()) { > + NS_WARNING("Cannot set up A2dp Observers!"); You probably want to null out sA2dpObserver if this happens. ::: dom/bluetooth/BluetoothA2dpManager.h @@ +33,5 @@ > + void Disconnect(); > + void HandleSinkPropertyChanged(const BluetoothSignal& aSignal); > + > +private: > + friend class BluetoothA2dpManagerObserver; Why not just make HandleShutdown public so that the observer doesn't have to be a friend?

Comment on attachment 756413 [details] [diff] [review] Patch 2(v4): Implement SendSinkMessage in BluetoothService, r=echou Review of attachment 756413 [details] [diff] [review]: ----------------------------------------------------------------- Clearing request until my question about compiling and running this in debug mode is answered. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +965,5 @@ > } > > +#ifdef DEBUG > +static void > +RunSinkCallback(bool aConnect, DBusMessage* aMsg, void *aParam) Please rename this so it's obvious that it's side-effect free. It seems like it could be called "CheckForError". @@ +967,5 @@ > +#ifdef DEBUG > +static void > +RunSinkCallback(bool aConnect, DBusMessage* aMsg, void *aParam) > +{ > + UnpackVoidMessage(aMsg, nullptr, BluetoothValue(), nsString()); The last parameter should be EmptyString(). @@ +968,5 @@ > +static void > +RunSinkCallback(bool aConnect, DBusMessage* aMsg, void *aParam) > +{ > + UnpackVoidMessage(aMsg, nullptr, BluetoothValue(), nsString()); > + if (!v.get_bool()) { I don't see v declared anywhere. Has this been compiled and tested in debug mode?

Comment on attachment 758366 [details] [diff] [review] Patch 4(v3): Implement NotifyStatusChagned() and NotifyAudioManager(), r=echou Review of attachment 758366 [details] [diff] [review]: ----------------------------------------------------------------- One major comment here. ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +290,5 @@ > + > + nsString name; > + name.AssignLiteral("connected"); > + BluetoothValue v = mConnected; > + parameters.AppendElement(BluetoothNamedValue(name, v)); Why bother with a local variable for name as opposed to using '...(BluetoothNamedValue(NS_LITERAL_STRING("connected"), v)...'? @@ +294,5 @@ > + parameters.AppendElement(BluetoothNamedValue(name, v)); > + > + name.AssignLiteral("address"); > + v = mDeviceAddress; > + parameters.AppendElement(BluetoothNamedValue(name, v)); Ditto here. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +1665,5 @@ > } > return false; > } > + > +NS_IMPL_ISUPPORTS0(BluetoothHfpManager) Now that BluetoothHfpManager (and the rest of the Bluetooth managers) implement nsISupports, they're refcounted, but nobody ever reference counts them. You should make the StaticAutoPtrs for them (the ones that hold the singleton instance) StaticRefPtrs so that they don't accidentally get deleted early.

Comment on attachment 756411 [details] [diff] [review] Patch 1(v2): Implement BluetoothA2dpManager, r=echou Review of attachment 756411 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +23,5 @@ > +using namespace mozilla; > +USING_BLUETOOTH_NAMESPACE > + > +namespace { > + StaticAutoPtr<BluetoothA2dpManager> gBluetoothA2dpManager; I followed the changes in bug 798035. Currently, we have similar implementation in both BluetoothHfpManager and BluetoothOppManager. Can I create a follow-up to modify three Bluetooth*Manager at one time? @@ +99,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + sA2dpObserver = new BluetoothA2dpManagerObserver(); > + if (!sA2dpObserver->Init()) { > + NS_WARNING("Cannot set up A2dp Observers!"); Agree. ::: dom/bluetooth/BluetoothA2dpManager.h @@ +33,5 @@ > + void Disconnect(); > + void HandleSinkPropertyChanged(const BluetoothSignal& aSignal); > + > +private: > + friend class BluetoothA2dpManagerObserver; Agree.

Comment on attachment 758366 [details] [diff] [review] Patch 4(v3): Implement NotifyStatusChagned() and NotifyAudioManager(), r=echou Review of attachment 758366 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for reviewing. Will update patch later. ::: dom/bluetooth/BluetoothA2dpManager.cpp @@ +290,5 @@ > + > + nsString name; > + name.AssignLiteral("connected"); > + BluetoothValue v = mConnected; > + parameters.AppendElement(BluetoothNamedValue(name, v)); Fixed. @@ +294,5 @@ > + parameters.AppendElement(BluetoothNamedValue(name, v)); > + > + name.AssignLiteral("address"); > + v = mDeviceAddress; > + parameters.AppendElement(BluetoothNamedValue(name, v)); Fixed. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +1665,5 @@ > } > return false; > } > + > +NS_IMPL_ISUPPORTS0(BluetoothHfpManager) Fixed in the next patch.

(In reply to Blake Kaplan (:mrbkap) from comment #55) > ::: dom/bluetooth/linux/BluetoothDBusService.cpp > @@ +965,5 @@ > > } > > > > +#ifdef DEBUG > > +static void > > +RunSinkCallback(bool aConnect, DBusMessage* aMsg, void *aParam) > > Please rename this so it's obvious that it's side-effect free. It seems like > it could be called "CheckForError". Renaming this function to |CheckForSinkError| in updated patch. > > @@ +967,5 @@ > > +#ifdef DEBUG > > +static void > > +RunSinkCallback(bool aConnect, DBusMessage* aMsg, void *aParam) > > +{ > > + UnpackVoidMessage(aMsg, nullptr, BluetoothValue(), nsString()); > > The last parameter should be EmptyString(). This part should be modified and please see new patch for details. > > @@ +968,5 @@ > > +static void > > +RunSinkCallback(bool aConnect, DBusMessage* aMsg, void *aParam) > > +{ > > + UnpackVoidMessage(aMsg, nullptr, BluetoothValue(), nsString()); > > + if (!v.get_bool()) { > > I don't see v declared anywhere. Has this been compiled and tested in debug > mode? This part should be modified and please see new patch for details.

Comment on attachment 758412 [details] [diff] [review] Patch 5(v5): Handle A2DP status changed in AudioManager, r=echou Review of attachment 758412 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine overall. ::: dom/system/gonk/AudioManager.cpp @@ +39,5 @@ > #define HEADPHONES_STATUS_HEADPHONE NS_LITERAL_STRING("headphone").get() > #define HEADPHONES_STATUS_OFF NS_LITERAL_STRING("off").get() > #define HEADPHONES_STATUS_UNKNOWN NS_LITERAL_STRING("unknown").get() > #define BLUETOOTH_SCO_STATUS_CHANGED "bluetooth-sco-status-changed" > +#define BLUETOOTH_A2DP_STATUS_CHANGED "bluetooth-a2dp-status-changed" Are these bluetooth strings something we can put in a header? (for a follow up, this is fine for now) @@ +197,5 @@ > { > + if ((strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED) == 0) || > + (strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED) == 0)) { > + nsresult rv; > + int status = NS_ConvertUTF16toUTF8(aData).ToInteger(&rv); Hmm, for something this simple you might be able to compare *aData against '1' and '0'. Up to you. @@ +200,5 @@ > + nsresult rv; > + int status = NS_ConvertUTF16toUTF8(aData).ToInteger(&rv); > + if (NS_FAILED(rv) || status > 1 || status < 0) { > + nsCString errorMsg; > + errorMsg.Append(nsPrintfCString("Wrong data value of %s", aTopic)); I think you can use nsPrintfCString directly instead of appending it. @@ +222,5 @@ > AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_IN_BLUETOOTH_SCO_HEADSET, > + audioState, address.get()); > + if (audioState == AUDIO_POLICY_DEVICE_STATE_AVAILABLE) { > + String8 cmd; > + cmd.appendFormat("bt_samplerate=%d", kBtSampleRate); kBtSampleRate means it's a constant.. but it's not - it's a variable. Can you fix this while you're here? Either hardcode 8000 in the string or make kBtSampleRate const.

(In reply to Blake Kaplan (:mrbkap) from comment #56) > > ::: dom/bluetooth/BluetoothHfpManager.cpp > @@ +1665,5 @@ > > } > > return false; > > } > > + > > +NS_IMPL_ISUPPORTS0(BluetoothHfpManager) > > Now that BluetoothHfpManager (and the rest of the Bluetooth managers) > implement nsISupports, they're refcounted, but nobody ever reference counts > them. You should make the StaticAutoPtrs for them (the ones that hold the > singleton instance) StaticRefPtrs so that they don't accidentally get > deleted early. Thanks. I'll update patch.

(In reply to Michael Wu [:mwu] from comment #60) > > #define BLUETOOTH_SCO_STATUS_CHANGED "bluetooth-sco-status-changed" > > +#define BLUETOOTH_A2DP_STATUS_CHANGED "bluetooth-a2dp-status-changed" > > Are these bluetooth strings something we can put in a header? (for a follow > up, this is fine for now) Will file a follow-up for this. > > + nsCString errorMsg; > > + errorMsg.Append(nsPrintfCString("Wrong data value of %s", aTopic)); > > I think you can use nsPrintfCString directly instead of appending it. Got it! Will fix in the final patch. > @@ +222,5 @@ > > + cmd.appendFormat("bt_samplerate=%d", kBtSampleRate); > > kBtSampleRate means it's a constant.. but it's not - it's a variable. Can > you fix this while you're here? Either hardcode 8000 in the string or make > kBtSampleRate const. I'd like to make kBtSampleRate as a const int.

Updated based on comments from mrbkap. Will file a follow-up for removing all observer classes for Bluetooth*Manager at one time.

Patch updated based on review comments from mrbkap. Please help to review again. Thanks.

Patch updated based on comment from mrbkap. All instances of profile manager are kept by StaticRefPtr rather than by StatidAutoPtr.

Patch updated based on comment from mwu.

I decided to (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #62) > (In reply to Michael Wu [:mwu] from comment #60) > > > #define BLUETOOTH_SCO_STATUS_CHANGED "bluetooth-sco-status-changed" > > > +#define BLUETOOTH_A2DP_STATUS_CHANGED "bluetooth-a2dp-status-changed" > > > > Are these bluetooth strings something we can put in a header? (for a follow > > up, this is fine for now) > > Will file a follow-up for this. > I found it's just an easy fix. Let's fix it in the final patach.

Declare topic strings in BluetoothCommon.h

The dbus call of "Disconnect" for a2dp would fail when hands-free has been disconnected. So we should only send "Disconnect" message to dbus when we close the socket actively.

Comment on attachment 759656 [details] [diff] [review] Patch 4(v4): Implement NotifyStatusChagned() and NotifyAudioManager(), r=echou, r=mrbkap Review of attachment 759656 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.

try: https://tbpl.mozilla.org/?tree=Try&rev=9db4f9072328 https://tbpl.mozilla.org/?tree=Try&rev=492de880d1aa

Update final patch

try: https://tbpl.mozilla.org/?tree=Try&rev=2bc27a5dc062 https://tbpl.mozilla.org/?tree=Try&rev=6ad081a0c2f9 https://tbpl.mozilla.org/?tree=Try&rev=086b1710e05a

https://hg.mozilla.org/projects/birch/rev/72653e055a56 https://hg.mozilla.org/projects/birch/rev/d9d5d7de3e2c https://hg.mozilla.org/projects/birch/rev/b659ff61bce8 https://hg.mozilla.org/projects/birch/rev/4df0816bb82c https://hg.mozilla.org/projects/birch/rev/9ac1a0ab4203

https://hg.mozilla.org/mozilla-central/rev/72653e055a56 https://hg.mozilla.org/mozilla-central/rev/d9d5d7de3e2c https://hg.mozilla.org/mozilla-central/rev/b659ff61bce8 https://hg.mozilla.org/mozilla-central/rev/4df0816bb82c https://hg.mozilla.org/mozilla-central/rev/9ac1a0ab4203

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #57) > ::: dom/bluetooth/BluetoothA2dpManager.cpp > @@ +23,5 @@ > > +using namespace mozilla; > > +USING_BLUETOOTH_NAMESPACE > > + > > +namespace { > > + StaticAutoPtr<BluetoothA2dpManager> gBluetoothA2dpManager; > > I followed the changes in bug 798035. > > Currently, we have similar implementation in both BluetoothHfpManager and > BluetoothOppManager. Can I create a follow-up to modify three > Bluetooth*Manager at one time? Follow-up: bug 882551