Currently each Bluetooth*Manager inherits UnixSocketConsumer and it's not good. Instead of using UnixSocketConsumer in Bluetooth*Manager, I would like to create a new class 'BluetoothSocket' for Bluetooth*Manager using.

This blocks the tef+ bug 823803 so should it also be tef+?

Yes, please mark this tef+.

Okay. Feel free to re-assign if you're not the best owner, Eric.

(In reply to Andrew Overholt [:overholt] from comment #3) > Okay. Feel free to re-assign if you're not the best owner, Eric. I've started working on this for a while. Thanks, Andrew.

For those instances which want to be notified of events sent from a BluetoothSocket instance, they need to implement this interface to get notification.

After this new class is landed, communicating with other devices on profile level should become more intuitive and reasonable. Each Bluetooth*Manager doesn't need to inherit UnixSocketConsumer, instead, BluetoothSocket inherits UnixSocketConsumer. That makes Bluetooth*Manager be able to have more than 1 Bluetooth connections at a time with different remote devices.

Since we are now using BluetoothSocket directly for listening, this function can be removed.

A Bluetooth*Manager may hold more than one BluetoothSocket at a time, therefore we need to identify the caller (which socket calls the callback function).

:mrbkap, are you able to assist with the review this week? thanks

Reviews planned for today.

Comment on attachment 727448 [details] [diff] [review] patch 1: v1: New interface: BluetoothSocketObserver Review of attachment 727448 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothSocketObserver.h @@ +19,5 @@ > +public: > + virtual void ReceiveSocketData(nsAutoPtr<UnixSocketRawData>& aMessage) = 0; > + virtual void OnConnectSuccess() = 0; > + virtual void OnConnectError() = 0; > + virtual void OnDisconnect() = 0; Please add comments about when these methods are called.

Comment on attachment 727448 [details] [diff] [review] patch 1: v1: New interface: BluetoothSocketObserver Review of attachment 727448 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothSocketObserver.h @@ +13,5 @@ > +using namespace mozilla::ipc; > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +class BluetoothSocketObserver All of these methods exist and are identical in UnixSocketConsumer. Let's make UnixSocketConsumer inherit from BluetoothSocketObserver to avoid the duplication. Need to see another patch for that.

Comment on attachment 727451 [details] [diff] [review] patch 2: v1: New class: BluetoothSocket Review of attachment 727451 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothSocket.cpp @@ +11,5 @@ > +BluetoothSocket::BluetoothSocket(BluetoothSocketType aType, > + bool aAuth, > + bool aEncrypt, > + BluetoothSocketObserver* aObserver) > + : UnixSocketConsumer() No need to call the default constructor of your base class here. @@ +15,5 @@ > + : UnixSocketConsumer() > + , mType(aType) > + , mAuth(aAuth) > + , mEncrypt(aEncrypt) > + , mObserver(aObserver) The arguments are out of order with respect to the member order and you'll trigger warnings on gcc. Your member order looks good so please just reorder the initializers here. @@ +17,5 @@ > + , mAuth(aAuth) > + , mEncrypt(aEncrypt) > + , mObserver(aObserver) > +{ > + MOZ_ASSERT(aObserver); Which thread is this supposed to happen on? Let's add an assertion to this and all other methods in this class. @@ +20,5 @@ > +{ > + MOZ_ASSERT(aObserver); > +} > + > +BluetoothSocket::~BluetoothSocket() Since this doesn't do anything let's just remove this and take the compiler's default generated destructor. @@ +27,5 @@ > + > +bool > +BluetoothSocket::Connect(const nsACString& aDeviceAddress, int aChannel) > +{ > + MOZ_ASSERT(!aDeviceAddress.IsEmpty()); Can we assert anything about aChannel here? Non-zero? Greater than zero? @@ +30,5 @@ > +{ > + MOZ_ASSERT(!aDeviceAddress.IsEmpty()); > + > + BluetoothUnixSocketConnector* c = > + new BluetoothUnixSocketConnector(mType, aChannel, mAuth, mEncrypt); This should be in a nsAutoPtr to prevent leaking in case of failure below. @@ +39,5 @@ > + BT_LOG("%s failed. Current connected device address: %s", > + __FUNCTION__, NS_ConvertUTF16toUTF8(addr).get()); > + return false; > + } > + Nit: whitespace @@ +40,5 @@ > + __FUNCTION__, NS_ConvertUTF16toUTF8(addr).get()); > + return false; > + } > + > + return true; You'll want to .forget() your nsAutoPtr here also. @@ +47,5 @@ > +bool > +BluetoothSocket::Listen(int aChannel) > +{ > + BluetoothUnixSocketConnector* c = > + new BluetoothUnixSocketConnector(mType, aChannel, mAuth, mEncrypt); nsAutoPtr here too. @@ +63,5 @@ > + > +void > +BluetoothSocket::Disconnect() > +{ > + CloseSocket(); This can be inlined, let's just move it to the header. @@ +97,5 @@ > + > +void > +BluetoothSocket::GetAddress(nsAString& aDeviceAddress) > +{ > + GetSocketAddr(aDeviceAddress); This could be inlined too. Move to header. @@ +103,5 @@ > + > +BluetoothSocketState > +BluetoothSocket::GetState() const > +{ > + return (BluetoothSocketState)GetConnectionStatus(); Move to header. ::: dom/bluetooth/BluetoothSocket.h @@ +7,5 @@ > +#ifndef mozilla_dom_bluetooth_BluetoothSocket_h > +#define mozilla_dom_bluetooth_BluetoothSocket_h > + > +#include "BluetoothCommon.h" > +#include "BluetoothSocketObserver.h" This can just be forward-declared. @@ +8,5 @@ > +#define mozilla_dom_bluetooth_BluetoothSocket_h > + > +#include "BluetoothCommon.h" > +#include "BluetoothSocketObserver.h" > +#include "BluetoothUnixSocketConnector.h" This looks unused here. @@ +9,5 @@ > + > +#include "BluetoothCommon.h" > +#include "BluetoothSocketObserver.h" > +#include "BluetoothUnixSocketConnector.h" > +#include <mozilla/ipc/UnixSocket.h> Nit: This should use quotes, not brackets. @@ +11,5 @@ > +#include "BluetoothSocketObserver.h" > +#include "BluetoothUnixSocketConnector.h" > +#include <mozilla/ipc/UnixSocket.h> > + > +using namespace mozilla::ipc; using declarations aren't allowed in headers. Please remove and fully specify below (or use typedefs to make things easier). @@ +15,5 @@ > +using namespace mozilla::ipc; > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +enum BluetoothSocketState Is there a benefit to having a new enum with slightly different names that just duplicates an existing enum? Let's just use SocketConnectionStatus where we need this. @@ +31,5 @@ > + BluetoothSocket(BluetoothSocketType aType, > + bool aAuth, > + bool aEncrypt, > + BluetoothSocketObserver* aObserver); > + ~BluetoothSocket(); UnixSocketConsumer is refcounted so this destructor should be protected, not public. @@ +37,5 @@ > + bool Connect(const nsACString& aDeviceAddress, int aChannel); > + bool Listen(int aChannel); > + void Disconnect(); > + > + void OnConnectSuccess() MOZ_OVERRIDE; These are virtual already but lets make it explicit by marking these here.

Comment on attachment 727454 [details] [diff] [review] patch 4: v1: Apply BluetoothSocket to BluetoothOppManager Review of attachment 727454 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothOppManager.cpp @@ +84,4 @@ > */ > static const uint32_t kPutRequestHeaderSize = 6; > > +static nsAutoPtr<BluetoothOppManager> sInstance; Please use StaticAutoPtr. @@ +194,5 @@ > class CloseSocketTask : public Task > { > public: > + CloseSocketTask(BluetoothSocket* aSocket) > + : Task() No need to call the default constructor of your base class here. @@ +195,5 @@ > { > public: > + CloseSocketTask(BluetoothSocket* aSocket) > + : Task() > + , mSocket(aSocket) Can we assert that the socket is non-null? And that it's in the CONNECTED state? Also, which thread does this happen on? @@ +209,5 @@ > } > } > + > +private: > + BluetoothSocket* mSocket; This is a refcounted class and you're not holding a reference. mSocket could die during your delayed task. Please use nsRefPtr. ::: dom/bluetooth/BluetoothOppManager.h @@ +8,4 @@ > #define mozilla_dom_bluetooth_bluetoothoppmanager_h__ > > #include "BluetoothCommon.h" > +#include "BluetoothSocket.h" This can be forward-declared. @@ +101,4 @@ > /** > * RFCOMM socket status. > */ > + enum BluetoothSocketState mSocketStatus; Doesn't look like there's much need for this any longer since you always set it to mSocket->GetState(), right? Just get rid of this and always ask the socket. @@ +177,4 @@ > > nsRefPtr<BluetoothReplyRunnable> mRunnable; > nsRefPtr<DeviceStorageFile> mDsFile; > + RefPtr<BluetoothSocket> mSocket; nsRefPtr please.

Comment on attachment 727455 [details] [diff] [review] patch 5: v1: Apply BluetoothSocket to BluetoothHfpManager Review of attachment 727455 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +44,4 @@ > USING_BLUETOOTH_NAMESPACE > > namespace { > + static nsAutoPtr<BluetoothHfpManager> gBluetoothHfpManager; StaticAutoPtr. @@ +288,3 @@ > nsAutoCString ringMsg(kHfpCrlf); > ringMsg += "RING"; > ringMsg += kHfpCrlf; Nit: This should use AppendLiteral for both of these, and in several places below. That avoids lots of strlen calls. @@ +465,1 @@ > return nullptr; Returning null isn't enough here, you'll leave gBluetoothHfpManager set. Please just revert these changes (but then you can get rid of the null check after new()). @@ +481,1 @@ > ? true : false ; Nit: Get rid of the |? true : false| here, the == returns a bool anyway. @@ +1361,5 @@ > + > +bool > +BluetoothHfpManager::IsConnected() > +{ > + return (mSocket->GetState() == BluetoothSocketState::CONNECTED); Nit: remove extra parentheses. ::: dom/bluetooth/BluetoothHfpManager.h @@ +8,4 @@ > #define mozilla_dom_bluetooth_bluetoothhfpmanager_h__ > > #include "BluetoothCommon.h" > +#include "BluetoothSocket.h" Forward-declare. @@ +54,3 @@ > { > public: > + ~BluetoothHfpManager(); This should be protected. @@ +60,4 @@ > MOZ_OVERRIDE; > + void OnConnectSuccess() MOZ_OVERRIDE; > + void OnConnectError() MOZ_OVERRIDE; > + void OnDisconnect() MOZ_OVERRIDE; Nit: Add the virtual to all these methods here. @@ +113,4 @@ > nsString mDevicePath; > nsString mMsisdn; > nsString mOperatorName; > + BluetoothSocketState mSocketStatus; Let's lose this in favor of always calling mSocket->GetState(). @@ +117,5 @@ > > nsTArray<Call> mCurrentCallArray; > nsAutoPtr<BluetoothTelephonyListener> mListener; > nsRefPtr<BluetoothReplyRunnable> mRunnable; > + RefPtr<BluetoothSocket> mSocket; nsRefPtr.

Comment on attachment 727457 [details] [diff] [review] patch 6: v1: Apply BluetoothSocket to BluetoothScoManager Review of attachment 727457 [details] [diff] [review]: ----------------------------------------------------------------- Looks like all the comments that applied to the previous managers apply here as well. Please update accordingly.

Comment on attachment 727458 [details] [diff] [review] patch 7: v1: Remove function ListenSocketViaService() Review of attachment 727458 [details] [diff] [review]: ----------------------------------------------------------------- r++

Comment on attachment 727460 [details] [diff] [review] patch 8: v1: Add BluetoothSocket* as an argument of callback functions in BluetoothSocketObserver Review of attachment 727460 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +1303,4 @@ > } > > void > +BluetoothHfpManager::OnConnectSuccess(BluetoothSocket* aSocket) For pretty much all of these methods you should be asserting that aSocket == mSocket. In all the other managers too (Except maybe the OPP manager since we expect more than one some day... There you can just assert non-null). ::: dom/bluetooth/BluetoothScoManager.cpp @@ +175,4 @@ > > // Virtual function of class SocketConsumer > void > +BluetoothScoManager::ReceiveSocketData(nsAutoPtr<UnixSocketRawData>& aMessage, Nit: trailing whitespace there. ::: dom/bluetooth/BluetoothSocketObserver.h @@ +19,5 @@ > class BluetoothSocketObserver > { > public: > + virtual void ReceiveSocketData(nsAutoPtr<UnixSocketRawData>& aMessage, > + BluetoothSocket* aSocket) = 0; aSocket should be the first parameter here.

* Fixed problems mentioned in Blake's review, except: @@ +27,5 @@ > + > +bool > +BluetoothSocket::Connect(const nsACString& aDeviceAddress, int aChannel) > +{ > + MOZ_ASSERT(!aDeviceAddress.IsEmpty()); > Can we assert anything about aChannel here? Non-zero? Greater than zero? Currently aChannel can be positive integer, 0, or -1 for SCO connection (Although it's not used in the SCO case). Actually it can be any value, we can tell if aChannel is valid from the the returning value of ConnectSocket(). @@ +15,5 @@ > +using namespace mozilla::ipc; > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +enum BluetoothSocketState > > Is there a benefit to having a new enum with slightly different names that just duplicates an existing enum? Let's just use SocketConnectionStatus where we need this. Originally I did this because I would like to make a more friendly interface for upper layers who use BluetoothSocket instances. In this case they can avoid using SocketConnectionStatus under mozilla::ipc namespace. However, the problem here is that every time SocketConnectionStatus is modified, BluetoothState should be changed as well. Therefore I took Blake's advice and removed the enum BluetoothSocketState.

Comment on attachment 731196 [details] [diff] [review] patch 2: v2: New class: BluetoothSocket Review of attachment 731196 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothSocket.cpp @@ +33,5 @@ > + > + nsAutoPtr<BluetoothUnixSocketConnector> c( > + new BluetoothUnixSocketConnector(mType, aChannel, mAuth, mEncrypt)); > + > + if (!ConnectSocket(c.forget(), aDeviceAddress.BeginReading())) { I missed that ConnectSocket takes ownership of c, even in the failure case! In that case, there's no need to use the AutoPtr.

* Fixed problems mentioned by Blake, except: @@ +195,5 @@ > { > public: > + CloseSocketTask(BluetoothSocket* aSocket) > + : Task() > + , mSocket(aSocket) > Can we assert that the socket is non-null? And that it's in the CONNECTED state? > Also, which thread does this happen on? I added an assertion to check if the socket is null, but we don't really care which thread is the instance created on since we've already had an NS_IsMainThread() assertion in function Run(). Furthermore, the CloseSocketTask() is used to prevent from leaving the OPP connection open. In other words, if remote device has dropped the connection, then we don't have to do anything with that. So I don't think the CONNECTED assertion is necessary, it should be o.k even if it's DISCONNECTED. @@ +101,4 @@ > /** > * RFCOMM socket status. > */ > + enum BluetoothSocketState mSocketStatus; > > Doesn't look like there's much need for this any longer since you always set it to mSocket->GetState(), right? Just get rid of this and always ask the socket. The reason why Bluetooth*Manager caches status of the socket is that we need to know the "previous" state of the socket in the callback function OnDisconnect(). (In function UnixSocketConsumer::NotifyDisconnect() in UnixSocket.cpp, mConnectionStatus is set to SOCKET_DISCONNECTED before calling OnDisconnect())

* Fixed problems mentioned by Blake, except: @@ +54,3 @@ > { > public: > + ~BluetoothHfpManager(); > This should be protected. The compiler will complain if the dtor is not public. That's because we try to do assignment: BluetoothScoManager* manager = new BluetoothScoManager(); NS_ENSURE_TRUE(manager, nullptr); NS_ENSURE_TRUE(manager->Init(), nullptr); gBluetoothScoManager = manager; // Here! Of course we can declare "friend class StaticAutoPtr<BluetoothScoManager>" in BluetoothScoManager.h to make this work, but just wondering if we really need to do that. If I misunderstood anything, please feel free to correct me, thank you.

* Updated according to Blake's review

* Updated based on Blake's review

(In reply to Eric Chou [:ericchou] [:echou] from comment #25) > The reason why Bluetooth*Manager caches status of the socket is that we need > to know the "previous" state of the socket in the callback function > OnDisconnect(). (In function UnixSocketConsumer::NotifyDisconnect() in > UnixSocket.cpp, mConnectionStatus is set to SOCKET_DISCONNECTED before > calling OnDisconnect()) In that case, the name of the member should be something like "mPrevSocketState". Or, looking at the uses, it seems like the member could become a boolean: "mConnected".

Comment on attachment 731766 [details] [diff] [review] patch 4: v2: Apply BluetoothSocket to BluetoothOppManager Review of attachment 731766 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment 29 taken into account (could do that in a separate patch).

Comment on attachment 731807 [details] [diff] [review] patch 5: v2: Apply BluetoothSocket to BluetoothHfpManager Review of attachment 731807 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +464,1 @@ > NS_ENSURE_TRUE(manager, nullptr); Nit (sorry I didn't catch this before): no need for the null check after new. ::: dom/bluetooth/BluetoothHfpManager.h @@ +113,4 @@ > nsString mDevicePath; > nsString mMsisdn; > nsString mOperatorName; > + enum SocketConnectionStatus mSocketStatus; Nit: no need for 'enum' here in C++ code.

Comment on attachment 731810 [details] [diff] [review] patch 6: v2: Apply BluetoothSocket to BluetoothScoManager Review of attachment 731810 [details] [diff] [review]: ----------------------------------------------------------------- Ibid. ::: dom/bluetooth/BluetoothScoManager.cpp @@ +167,1 @@ > NS_ENSURE_TRUE(manager, nullptr); Ditto for null-checking the result of new here. ::: dom/bluetooth/BluetoothScoManager.h @@ +44,2 @@ > > + enum SocketConnectionStatus mSocketStatus; Ditto comments about mSocketStatus and use of the enum keyword. :)

To be clear: I need a response to comment 29 before this can land, but we're really close now. Thanks for your patience, Eric!

Based on Blake's review, several places have been revised in this patch: 1. Rename mSocketStatus 2. Remove keyword enum 3. Remove null-checking after new Bluetooth*Manager instances

(In reply to Blake Kaplan (:mrbkap) from comment #33) > To be clear: I need a response to comment 29 before this can land, but we're > really close now. Thanks for your patience, Eric! Hi Blake, Thanks for your review, Blake! I've been in hospital taking care of my wife since Monday night (my baby daughter was born on Tuesday!), so I'll check my mail as possible as I can. :) Eric

Comment on attachment 727448 [details] [diff] [review] patch 1: v1: New interface: BluetoothSocketObserver Spoke with mrbkap, there is value to landing this to mozilla-central early (while echou is out). This checkin applies to all r+'d patches.

(In reply to Eric Chou [:ericchou] [:echou] from comment #35) > (In reply to Blake Kaplan (:mrbkap) from comment #33) > > To be clear: I need a response to comment 29 before this can land, but we're > > really close now. Thanks for your patience, Eric! > > Hi Blake, > > Thanks for your review, Blake! I've been in hospital taking care of my wife > since Monday night (my baby daughter was born on Tuesday!), so I'll check my > mail as possible as I can. :) > > Eric And let me be the first to congratulate you via the most inappropriate interface possible! :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/4511485feea4 https://hg.mozilla.org/integration/mozilla-inbound/rev/59c72b632fcc https://hg.mozilla.org/integration/mozilla-inbound/rev/2a293cde1854 https://hg.mozilla.org/integration/mozilla-inbound/rev/21cca5f779e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/75c16100fa30 https://hg.mozilla.org/integration/mozilla-inbound/rev/674752e14308 https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc4ec4ef36b https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2bf6653814 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1dd628499a I can't wait to attempt to uplift this...

And because no good deed goes unpunished, backed out for build bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/2233d7948013 https://tbpl.mozilla.org/php/getParsedLog.php?id=21396666&tree=Mozilla-Inbound /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothOppManager.cpp: In static member function 'static mozilla::dom::bluetooth::BluetoothOppManager* mozilla::dom::bluetooth::BluetoothOppManager::Get()': /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothOppManager.cpp:252: error: cannot allocate an object of abstract type 'mozilla::dom::bluetooth::BluetoothOppManager' /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothOppManager.h:26: note: because the following virtual functions are pure within 'mozilla::dom::bluetooth::BluetoothOppManager': /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothSocketObserver.h:22: note: virtual void mozilla::dom::bluetooth::BluetoothSocketObserver::ReceiveSocketData(nsAutoPtr<mozilla::ipc::UnixSocketRawData>&) /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothSocketObserver.h:23: note: virtual void mozilla::dom::bluetooth::BluetoothSocketObserver::OnConnectSuccess() /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothSocketObserver.h:24: note: virtual void mozilla::dom::bluetooth::BluetoothSocketObserver::OnConnectError() /builds/slave/m-in-ics_a7_g-0000000000000000/build/dom/bluetooth/BluetoothSocketObserver.h:25: note: virtual void mozilla::dom::bluetooth::BluetoothSocketObserver::OnDisconnect() There's a bunch of warnings in that log too. Maybe clean those up while you're at it?

Ok, I'm working on getting this landed since Eric decided having a child was a better use of his time. Some people. :| It looks like patch 8 was not applied correctly in the checkin?. Specifically, the diff in https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2bf6653814, BluetoothSocketObserver.h does not have the function prototype change patches as seen in Patch 8 listed in this bug. Sure enough, I got a conflict I had to hand merge when I put this together locally too. I think there was a patch missing somewhere that added comments. I'll make a new version of that patch, and submit this whole thing to try.

Got the fixed patch up on try. Did a quick experimental to see what status of this is against b2g18. 15 hunks failed. I don't think the merge will be too difficult though.

Checking in all the files helps. Try #2: https://tbpl.mozilla.org/?tree=Try&rev=a573d9710386

Patch 8 merge fix since the comment mismatch in the Hfp header was causing conflicts.

Pushed at https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9ba6b7c64335, then backed out due to hg rebase eating the first patch. I hate you, hg.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf47d3b3d6c1 https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f5fa4c5fc9 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1c1298bd85 https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a3e3a4e534 https://hg.mozilla.org/integration/mozilla-inbound/rev/4530d1c784f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5a3e86a843 https://hg.mozilla.org/integration/mozilla-inbound/rev/26f388aeced9 https://hg.mozilla.org/integration/mozilla-inbound/rev/1246e3291a04 https://hg.mozilla.org/integration/mozilla-inbound/rev/b5f2678e1cc8

And hg ate the patch in the backed out commit because it was empty due to me forgetting to add the new files. But didn't actually say anything about it being empty. So I still hate hg. :|

Working on b2g18 uplift patches now. Major problem here is working around the fact that we didn't uplift the reader thread fix in opp, so I'm having to merge /around/ that. It is gross. :(

Ok, this merge is going to be hell if we don't also uplift: - Bug 826931 patch 1 - Just turns UnixSocketRawData parameters from * to nsAutoPtr&'s, but conflicts all over the place for this - Bug 844705 - Fixes main thread sends for OppManager There's also another patch before 844705 to change from using a UnixSocketImpl* to sInstance, but I think we can ignore that one. Is there a good way to nominate a single patch of a multipatch set for uplift, or should I make a new bug for it?

Bug 845148 conflicts with bug 844705. However, if we back it out, bring in b Ok, got the merge done. Here's my merge strategy for the moment: - Back out bug 845148 - Merge bug 844705, fixing what 845148 did and making us compatible for this bug, since we don't have a UnixSocketImpl* anymore anyways. - Merge bug 826931 patch 1 - Merge patch set from this bug. Small cleanup due to different logging systems, RIL->telephony namechanges that haven't been backported, and no SendRingIndicator, but it's not too bad. There's only conflicts on 3 of the patches, everything else merges cleanly. Is this a viable strategy, or should I look at working around the 845148 backout? I think it's going to be REALLY gross to try that, but I'm not exactly sure about my choices here.

https://hg.mozilla.org/mozilla-central/rev/bf47d3b3d6c1 https://hg.mozilla.org/mozilla-central/rev/c3f5fa4c5fc9 https://hg.mozilla.org/mozilla-central/rev/fc1c1298bd85 https://hg.mozilla.org/mozilla-central/rev/a7a3e3a4e534 https://hg.mozilla.org/mozilla-central/rev/4530d1c784f3 https://hg.mozilla.org/mozilla-central/rev/1d5a3e86a843 https://hg.mozilla.org/mozilla-central/rev/26f388aeced9 https://hg.mozilla.org/mozilla-central/rev/1246e3291a04 https://hg.mozilla.org/mozilla-central/rev/b5f2678e1cc8

Talked to mrbkap, going ahead with above merge strategy. Uploading b2g18 patch set shortly.

Whoops. No need to reopen apparently.

Same as Patch 1 on m-c

Same as patch 2 on m-c

Same as patch 3 on m-c

Required merge to deal with lack of SendRingIndicator and BT_LOG calls.

Required merge due to lack of SendRingIndicator and BT_LOG calls

Same as patch 6 on m-c

Required merge due to log messages and other changes in DBusServer, in the end the patch is the same though. Just different line numbers.

Couple of small merges in HfpManager due to lack of ring indicator

Same patch as on m-c.

Marking the status flags accordingly so the uplift work to the appropriate branches is tracked

Ok, made a new bug for the other UnixSocket patch so we don't have to bring all of RIL with it. It's Bug 859005

Comment on attachment 734094 [details] [diff] [review] b2g18 - patch 1: v1: New interface: BluetoothSocketObserver Review of attachment 734094 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothSocketObserver.h @@ +7,5 @@ > +#ifndef mozilla_dom_bluetooth_BluetoothSocketObserver_h > +#define mozilla_dom_bluetooth_BluetoothSocketObserver_h > + > +#include "BluetoothCommon.h" > +#include <mozilla/ipc/UnixSocket.h> This should have been fixed in the trunk patch as well... this should use quotes instead of braces.

Note: Patch 6 does NOT like b2g18_v1_0_1.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/15321131e53b https://hg.mozilla.org/releases/mozilla-b2g18/rev/a3eb01854dcb https://hg.mozilla.org/releases/mozilla-b2g18/rev/52b929f9c1a4 https://hg.mozilla.org/releases/mozilla-b2g18/rev/c6215c786969 https://hg.mozilla.org/releases/mozilla-b2g18/rev/8385b4ded9bb https://hg.mozilla.org/releases/mozilla-b2g18/rev/86aeecf5b958 https://hg.mozilla.org/releases/mozilla-b2g18/rev/b18f322ecdd6 https://hg.mozilla.org/releases/mozilla-b2g18/rev/3b9f8af0ba39 https://hg.mozilla.org/releases/mozilla-b2g18/rev/4839116be2b0 But yeah, this is going to need some b2g18_v1_0_1 patches or uplift of some dependent bugs.

Joy. :| Will get a 1.0.1 patchset up today.

Working on 1.0.1. Everything not involving HfpManager applies cleanly. I'm working on figuring out merge differences now (bug 827212, bug 828798, bug 827255, bug 827204, the list goes on. :| ). Unfortunately I'm out for the next few hours, so this is going to miss today's v1.0.1 uplift (already talked to RyanVM about it). Hoping to get this sorted once and for all tonight, and will get it in tomorrow's v1.0.1 uplift.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #69) > Working on 1.0.1. Everything not involving HfpManager applies cleanly. I'm > working on figuring out merge differences now (bug 827212, bug 828798, bug > 827255, bug 827204, the list goes on. :| ). Unfortunately I'm out for the > next few hours, so this is going to miss today's v1.0.1 uplift (already > talked to RyanVM about it). Hoping to get this sorted once and for all > tonight, and will get it in tomorrow's v1.0.1 uplift. Kyle, just a friendly reminder that, I saw that you removed the whole SendRingIndicatorTask and the flag sStopSendingRingFlag, but we need this task to send ringtone every 3 sec when there is an incoming call. Is there any filed bug we missed?

AGH. Ok, no more doing merges for other people on 9 patch sets for me in code I'm unfamiliar with. We'll just let it wait 'til the original dev is back next time. :) So yeah, that shouldn't have been removed. I have no idea why it was. My merger (meld) were showing it as non-existant in b2g18 in the first place, but that was apparently because it was removing it and I just went along with it. I guess patch that back in. That said, you may want to deal with the 1.0.1 patch. I tried it a couple of times today and it's a mess, since there's a bunch of differences in what's in 1.0.1 and what's in m-c in HFPManager, and I couldn't get it to work cleanly. Everything but HfpManager works fine.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #71) > That said, you may want to deal with the 1.0.1 patch. I tried it a couple of > times today and it's a mess, since there's a bunch of differences in what's > in 1.0.1 and what's in m-c in HFPManager, and I couldn't get it to work > cleanly. Everything but HfpManager works fine. You're right, Kyle. Since we land all patches regarding to Hfp certification on v1.1 but not v1.0.1, that's why you see there's a bunch of differences. Would you mind reverting b2g18 patch or opening a new bug for fixing it? Feel free to assign that to me if you want :)

Followup and patch filed in bug 860171. Hopefully that's the only thing that went wrong. :)

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #71) > AGH. Ok, no more doing merges for other people on 9 patch sets for me in > code I'm unfamiliar with. We'll just let it wait 'til the original dev is > back next time. :) > Oh, Kyle, I really appreciate your help. That should be my work and you realy did a lot. Please ignore if I said anything sounds offended or something. :)

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9ee68bf1e7c7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/ef1c83e56532 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0b6f34748df7 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/1085a5064358 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9365b5607640 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/cacfc55f639d https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/933616c80582 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6013264068af https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d91afcc29b91