This also blocked "TC_AG_TCA" series test cases.

Sorry, I don't really understand this title. Does it mean that this is a must have item for the Bluetooth certification that we're having right now? Thanks.

Yes, Kevin. This is mandatory for the Bluetooth Hfp certification. I'm working on the implementation for platform side, and I will file another one for Dialer App. When we switch audio stream from Bluetooth headset to speaker, Dialer app can close Bluetooth Sco socket by new API. When switching back to Bluetooth headset, it can also create Bluetooth Sco socket by new API.

Thank you, Gina. In this case, I think it looks like a tef+ case.

Comment on attachment 747266 [details] [diff] [review] Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter Three interfaces are added in this patch.

Comment on attachment 747262 [details] [diff] [review] Patch 2(v1): Implementation in BluetoothHfpManager Review of attachment 747262 [details] [diff] [review]: ----------------------------------------------------------------- The general idea for this patch is let BluetoothHfpManager owns a SCO socket, and move functions from BluetoothScoManager to BluetoothHfpManager.

Remove class BluetoothScoManager

Comment on attachment 747266 [details] [diff] [review] Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter Review of attachment 747266 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. ::: dom/bluetooth/BluetoothAdapter.cpp @@ +150,5 @@ > + > + void > + ReleaseMembers() > + { > + BluetoothReplyRunnable::ReleaseMembers(); Since we don't do anything else than BluetoothReplyRunnable::ReleaseMembers(), no need to implement this function. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +2846,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + BluetoothHfpManager* hfp = BluetoothHfpManager::Get(); > + NS_ENSURE_TRUE_VOID(hfp); > + if(!hfp->ConnectSco(aRunnable)) { nit: need space after 'if'

Comment on attachment 747262 [details] [diff] [review] Patch 2(v1): Implementation in BluetoothHfpManager Review of attachment 747262 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gina, please have me review again once revision is done. Thanks! ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +519,5 @@ > + do_GetService("@mozilla.org/observer-service;1"); > + NS_ENSURE_TRUE_VOID(obs); > + > + const PRUnichar* addr = > + mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading(); There are two problems: 1. Once mDevicePath is set in OnConnectSuccess(), it seems not to be cleaned at any other places, like OnDisconnect(). The original implementation seems to work well. How about making addr as an argument of this function? 2. If mDevicePath.BeginReading() returns nullptr when it is empty, then we don't need the IsEmpty() check. @@ +523,5 @@ > + mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading(); > + > + if (NS_FAILED(obs->NotifyObservers(nullptr, > + BLUETOOTH_SCO_STATUS_CHANGED, > + addr))) { nit: weird indent @@ +1489,5 @@ > +BluetoothHfpManager::OnScoConnectSuccess() > +{ > + // For active connection request, we need to reply the DOMRequest > + if (mScoRunnable) { > + DispatchBluetoothReply(mRunnable, This should be mScoRunnable, right? @@ +1491,5 @@ > + // For active connection request, we need to reply the DOMRequest > + if (mScoRunnable) { > + DispatchBluetoothReply(mRunnable, > + BluetoothValue(true), NS_LITERAL_STRING("")); > + mScoRunnable.forget(); I think you should do mScoRunnable = nullptr; or mScoRunnable may leak. @@ +1506,5 @@ > + if (mScoRunnable) { > + NS_NAMED_LITERAL_STRING(replyError, "Failed to create SCO socket!"); > + DispatchBluetoothReply(mScoRunnable, BluetoothValue(), replyError); > + > + mScoRunnable.forget(); I think you should do mScoRunnable = nullptr; or mScoRunnable may leak. @@ +1510,5 @@ > + mScoRunnable.forget(); > + } > + > + mScoSocket->Disconnect(); > + mScoSocketStatus = mScoSocket->GetConnectionStatus(); Since it's "ConnectError", it means that SCO has not been established. Maybe we don't need to call Disconnect() and update mScoSocketStatus. ListenSco() would be just fine. @@ +1551,5 @@ > + return false; > + } > + > + if (!mSocket) { > + NS_WARNING("BluetoothHfpManager is not connected"); If you want to check whether there is a 'connected' SLC or not, using IsConnected() would be more accurate than using !mSocket @@ +1565,5 @@ > + } > + > + mScoSocket->Disconnect(); > + > + mRunnable = aRunnable; mRunnable or mScoRunnable? @@ +1578,5 @@ > +bool > +BluetoothHfpManager::DisconnectSco() > +{ > + if (!mSocket) { > + NS_WARNING("BluetoothHfpManager is not connected"); If you want to check whether there is a 'connected' SLC or not, using IsConnected() would be more accurate than using !mSocket @@ +1592,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (gInShutdown) { > + MOZ_ASSERT(false, "ListenSco called while in shutdown!"); Why we use MOZ_ASSERT here and NS_WARNING at other places? :| @@ +1599,5 @@ > + > + if (mScoSocket->GetConnectionStatus() == > + SocketConnectionStatus::SOCKET_LISTENING) { > + NS_WARNING("SCO socket has been already listening"); > + return true; It's not the same strategy as HFP. In BluetoothHfpManager::Listen(), once we found that there is a connected HFP connection, false is returned. We should make these two profiles act in the same way.

Update patch as comments from echou.

Comment on attachment 747262 [details] [diff] [review] Patch 2(v1): Implementation in BluetoothHfpManager Review of attachment 747262 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +519,5 @@ > + do_GetService("@mozilla.org/observer-service;1"); > + NS_ENSURE_TRUE_VOID(obs); > + > + const PRUnichar* addr = > + mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading(); Fixed. @@ +523,5 @@ > + mDevicePath.IsEmpty() ? nullptr : mDevicePath.BeginReading(); > + > + if (NS_FAILED(obs->NotifyObservers(nullptr, > + BLUETOOTH_SCO_STATUS_CHANGED, > + addr))) { Fixed. @@ +1489,5 @@ > +BluetoothHfpManager::OnScoConnectSuccess() > +{ > + // For active connection request, we need to reply the DOMRequest > + if (mScoRunnable) { > + DispatchBluetoothReply(mRunnable, Fixed. @@ +1491,5 @@ > + // For active connection request, we need to reply the DOMRequest > + if (mScoRunnable) { > + DispatchBluetoothReply(mRunnable, > + BluetoothValue(true), NS_LITERAL_STRING("")); > + mScoRunnable.forget(); This should be fine, right? @@ +1506,5 @@ > + if (mScoRunnable) { > + NS_NAMED_LITERAL_STRING(replyError, "Failed to create SCO socket!"); > + DispatchBluetoothReply(mScoRunnable, BluetoothValue(), replyError); > + > + mScoRunnable.forget(); This should be fine, right? @@ +1510,5 @@ > + mScoRunnable.forget(); > + } > + > + mScoSocket->Disconnect(); > + mScoSocketStatus = mScoSocket->GetConnectionStatus(); Fixed. @@ +1551,5 @@ > + return false; > + } > + > + if (!mSocket) { > + NS_WARNING("BluetoothHfpManager is not connected"); Fixed. @@ +1565,5 @@ > + } > + > + mScoSocket->Disconnect(); > + > + mRunnable = aRunnable; Fixed. @@ +1578,5 @@ > +bool > +BluetoothHfpManager::DisconnectSco() > +{ > + if (!mSocket) { > + NS_WARNING("BluetoothHfpManager is not connected"); Fixed. @@ +1592,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (gInShutdown) { > + MOZ_ASSERT(false, "ListenSco called while in shutdown!"); I also modified this part in Listen(). @@ +1599,5 @@ > + > + if (mScoSocket->GetConnectionStatus() == > + SocketConnectionStatus::SOCKET_LISTENING) { > + NS_WARNING("SCO socket has been already listening"); > + return true; Fixed.

Comment on attachment 747377 [details] [diff] [review] Patch 2(v2): Implementation in BluetoothHfpManager Note that some logic is changed in OnDisconnect().

Comment on attachment 747266 [details] [diff] [review] Patch 1(v1): Add ConnectSco, DisconnectSco, IsScoConnected in nsIDOMBluetoothAdapter Review of attachment 747266 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +2867,5 @@ > + return; > + } > + > + NS_NAMED_LITERAL_STRING(replyError, > + "SCO socket doesn't exist or HFP is not connected"); Super-nit: there's an extra space between "not" and "connected". @@ +2879,5 @@ > + > + BluetoothHfpManager* hfp = BluetoothHfpManager::Get(); > + NS_ENSURE_TRUE_VOID(hfp); > + DispatchBluetoothReply(aRunnable, > + hfp->IsScoConnected(), NS_LITERAL_STRING("")); Use EmptyString() instead of NS_LITERAL_STRING("") here.

Comment on attachment 747377 [details] [diff] [review] Patch 2(v2): Implementation in BluetoothHfpManager Review of attachment 747377 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +1490,5 @@ > + // For active connection request, we need to reply the DOMRequest > + if (mScoRunnable) { > + DispatchBluetoothReply(mScoRunnable, > + BluetoothValue(true), NS_LITERAL_STRING("")); > + mScoRunnable.forget(); This should be: mScoRunnable = nullptr; otherwise it'll leak. @@ +1505,5 @@ > + if (mScoRunnable) { > + NS_NAMED_LITERAL_STRING(replyError, "Failed to create SCO socket!"); > + DispatchBluetoothReply(mScoRunnable, BluetoothValue(), replyError); > + > + mScoRunnable.forget(); This also leaks. @@ +1516,5 @@ > +BluetoothHfpManager::OnScoDisconnect() > +{ > + if (mScoSocketStatus == SocketConnectionStatus::SOCKET_CONNECTED) { > + ListenSco(); > + NotifyAudioManager(NS_LITERAL_STRING("")); All of the places where you use NS_LITERAL_STRING("") should be changed to use EmptyString(). @@ +1568,5 @@ > + BluetoothService* bs = BluetoothService::Get(); > + NS_ENSURE_TRUE(bs, false); > + nsresult rv = bs->GetScoSocket(mDeviceAddress, true, false, mScoSocket); > + > + return NS_FAILED(rv) ? false : true; Please change this to: return NS_SUCCEEDED(rv).

Just a suggestion, Gaia dialer also needs to change in order to let user have Bluetooth switch button for audio path. It is required to have an "icon" added into Dailer callscreen. In order to speed up progress, I suggest Gaia side can go in parallel.

After using |mRunnable = nullptr;| rather than |mRunnable.forget();|, I got a crash. Here's the backtrace: mozilla::dom::bluetooth::PBluetoothRequestParent::Write (this=0x5a5a5a5a, __v=0x5a5a5a5a, __msg=0x480deb00, __nullable=false) at /objdir-gecko/unagi/central_debug/ipc/ipdl/PBluetoothRequestParent.cpp:635 635 id = (__v)->mId; (gdb) bt #0 mozilla::dom::bluetooth::PBluetoothRequestParent::Write (this=0x5a5a5a5a, __v=0x5a5a5a5a, __msg=0x480deb00, __nullable=false) at /objdir-gecko/unagi/central_debug/ipc/ipdl/PBluetoothRequestParent.cpp:635 #1 0x41a0afb2 in mozilla::dom::bluetooth::PBluetoothRequestParent::Send__delete__ (actor=0x5a5a5a5a, response=...) at /objdir-gecko/unagi/central_debug/ipc/ipdl/PBluetoothRequestParent.cpp:78 #2 0x415e754e in mozilla::dom::bluetooth::BluetoothRequestParent::ReplyRunnable::Run (this=0x45326dc0) at /mozilla-central/dom/bluetooth/ipc/BluetoothParent.cpp:59 #3 0x41d3bb1a in nsThread::ProcessNextEvent (this=0x40404320, mayWait=<value optimized out>, result=0xbeb1a6ef) at /mozilla-central/xpcom/threads/nsThread.cpp:627 #4 0x41d02400 in NS_ProcessNextEvent (thread=0x5a5a5a5a, mayWait=false) at /objdir-gecko/unagi/central_debug/xpcom/build/nsThreadUtils.cpp:238 #5 0x419ee652 in mozilla::ipc::MessagePump::Run (this=0x404024c0, aDelegate=0x404310c0) at /mozilla-central/ipc/glue/MessagePump.cpp:82 #6 0x41d80e46 in MessageLoop::RunInternal (this=0x404310c0) at /mozilla-central/ipc/chromium/src/base/message_loop.cc:219 #7 0x41d80ea6 in MessageLoop::RunHandler (this=0x404310c0) at /mozilla-central/ipc/chromium/src/base/message_loop.cc:212 #8 MessageLoop::Run (this=0x404310c0) at /mozilla-central/ipc/chromium/src/base/message_loop.cc:186 #9 0x41924206 in nsBaseAppShell::Run (this=0x44025c40) at /mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:163 #10 0x418169ca in nsAppStartup::Run (this=0x44194b20) at /mozilla-central/toolkit/components/startup/nsAppStartup.cpp:289 #11 0x40d53316 in XREMain::XRE_mainRun (this=0xbeb1a984) at /mozilla-central/toolkit/xre/nsAppRunner.cpp:3879 #12 0x40d56022 in XREMain::XRE_main (this=0xbeb1a984, argc=<value optimized out>, argv=<value optimized out>, aAppData=0x21174) at /mozilla-central/toolkit/xre/nsAppRunner.cpp:3946 #13 0x40d561d2 in XRE_main (argc=1, argv=0xbeb1cb84, aAppData=0x21174, aFlags=<value optimized out>) at /mozilla-central/toolkit/xre/nsAppRunner.cpp:4147 #14 0x00009b2e in do_main (argc=1, argv=0xbeb1cb84) at /mozilla-central/b2g/app/nsBrowserApp.cpp:168 #15 main (argc=1, argv=0xbeb1cb84) at /mozilla-central/b2g/app/nsBrowserApp.cpp:261 (gdb)

Comment on attachment 747377 [details] [diff] [review] Patch 2(v2): Implementation in BluetoothHfpManager Review of attachment 747377 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. You may need to rebase because bug 860166 has landed.

The crash I mentioned in comment 21 should be also fixed in bug 860166. Will try again after rebasing.

We need Gaia support to call the new Bluetooth DOM API to control SCO. The followup is bug 870683.

try server: https://tbpl.mozilla.org/?tree=Try&rev=fb5bb0c0aa7d https://tbpl.mozilla.org/?tree=Try&rev=8b837c3d1382

http://hg.mozilla.org/projects/birch/rev/7c517de515f6 http://hg.mozilla.org/projects/birch/rev/d9e378c9a11b http://hg.mozilla.org/projects/birch/rev/aa56d0c01f07

Note that these patches must be landed after Bug 860166.

https://hg.mozilla.org/mozilla-central/rev/7c517de515f6 https://hg.mozilla.org/mozilla-central/rev/d9e378c9a11b https://hg.mozilla.org/mozilla-central/rev/aa56d0c01f07

https://hg.mozilla.org/releases/mozilla-b2g18/rev/9bcf08f31a56 https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e6991923537 https://hg.mozilla.org/releases/mozilla-b2g18/rev/c1835326ac3a https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/98bb9be2bbba https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/226258283ff7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f8b5fb65f5a5

Backed out for bustage. Bad time to mention that the branch-specific patches also didn't apply cleanly? https://hg.mozilla.org/releases/mozilla-b2g18/rev/4c06280df29a https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fab75f5db17d

The b2g18 patch 1 was missing a few rather important lines in for nsIDOMBluetoothAdapter.idl. Details details... https://hg.mozilla.org/releases/mozilla-b2g18/rev/0590172e959f https://hg.mozilla.org/releases/mozilla-b2g18/rev/6a86c543b604 https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed914f801af6 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a2cff60baac2 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8d20b1c20aa7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f45effe15482

Ryan, thanks for your help. I did miss the changes in nsIDOMBluetoothAdapter.idl in b2g18 patch 1. I remember that we should roll the uuid for idl when interface changes. So, I'm going to provide one more patch for b2g18 branch to fix it. Please help me to land it b2g18. If it's not necessary to roll the uuid, then it should be fine and just ignore my new patch. Thanks. :)

I checked the latest b2g18_v1_0_1, b2g18, and m-c, and I found something is missing in m-c patch 2. :(

Pushed to birch. http://hg.mozilla.org/projects/birch/rev/2745b9feab44

Thanks, Ryan.

Unable to verify. Lack of resources. Need Bluetooth PTS tool to verify. Marking as QARegressExclude.