851046 - [Bluetooth] BluetoothSocket implementation

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Eric Chou [:ericchou] [:echou]]

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.

Comment 1

[Andrew Overholt [:overholt]]

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

Comment 2

[Eric Chou [:ericchou] [:echou]]

Yes, please mark this tef+.

Comment 3

[Andrew Overholt [:overholt]]

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

Comment 4

[Eric Chou [:ericchou] [:echou]]

(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.

Comment 5

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: New interface: BluetoothSocketObserver

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

Comment 6

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: v1: New class: BluetoothSocket

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.

Comment 7

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 3: v1: Replace (Bluetooth*Managers)->CloseSocket() with (Bluetooth*Managers)->Disconnect()

Comment 8

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 4: v1: Apply BluetoothSocket to BluetoothOppManager

Comment 9

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 5: v1: Apply BluetoothSocket to BluetoothHfpManager

Comment 10

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 6: v1: Apply BluetoothSocket to BluetoothScoManager

Comment 11

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 7: v1: Remove function ListenSocketViaService()

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

Comment 12

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 8: v1: Add BluetoothSocket* as an argument of callback functions in BluetoothSocketObserver

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).

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Reviews planned for today.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

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 16

[Blake Kaplan (:mrbkap) (inactive)]

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 17

[Blake Kaplan (:mrbkap) (inactive)]

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 18

[Blake Kaplan (:mrbkap) (inactive)]

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 19

[Blake Kaplan (:mrbkap) (inactive)]

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 20

[Blake Kaplan (:mrbkap) (inactive)]

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 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 727458 [details] [diff] [review]
patch 7: v1: Remove function ListenSocketViaService()

Review of attachment 727458 [details] [diff] [review]:
-----------------------------------------------------------------

r++

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 23

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: v2: New class: BluetoothSocket

* 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 24

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 25

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 4: v2: Apply BluetoothSocket to BluetoothOppManager

* 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())

Comment 26

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 5: v2: Apply BluetoothSocket to BluetoothHfpManager

* 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.

Comment 27

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 6: v2: Apply BluetoothSocket to BluetoothScoManager

* Updated according to Blake's review

Comment 28

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 8: v1: Add BluetoothSocket* as an argument of callback functions in BluetoothSocketObserver

* Updated based on Blake's review

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

(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 30

[Blake Kaplan (:mrbkap) (inactive)]

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 31

[Blake Kaplan (:mrbkap) (inactive)]

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 32

[Blake Kaplan (:mrbkap) (inactive)]

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. :)

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

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!

Comment 34

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 9: v1: small misc fixes

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

Comment 35

[Eric Chou [:ericchou] [:echou]]

(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 36

[Alex Keybl [:akeybl]]

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.

Comment 37

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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...

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 40

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 41

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=239ff8f66cb6

Comment 42

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 43

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Checking in all the files helps.

Try #2: https://tbpl.mozilla.org/?tree=Try&rev=a573d9710386

Comment 44

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: patch 8: v1: Add BluetoothSocket* as an argument of callback functions in BluetoothSocketObserver

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

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 46

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 47

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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. :|

Comment 48

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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. :(

Comment 49

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 50

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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

Comment 52

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 53

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Whoops. No need to reopen apparently.

Comment 54

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 1: v1: New interface: BluetoothSocketObserver

Same as Patch 1 on m-c

Comment 55

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 2: v2: New class: BluetoothSocket

Same as patch 2 on m-c

Comment 56

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 3: v1: Replace (Bluetooth*Managers)->CloseSocket() with (Bluetooth*Managers)->Disconnect()

Same as patch 3 on m-c

Comment 57

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 4: v2: Apply BluetoothSocket to BluetoothOppManager

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

Comment 58

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 5: v2: Apply BluetoothSocket to BluetoothHfpManager

Required merge due to lack of SendRingIndicator and BT_LOG calls

Comment 59

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 6: v2: Apply BluetoothSocket to BluetoothScoManager

Same as patch 6 on m-c

Comment 60

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 7: v1: Remove function ListenSocketViaService()

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

Comment 61

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 8: v1: Add BluetoothSocket* as an argument of callback functions in BluetoothSocketObserver

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

Comment 62

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: b2g18 - patch 9: v1: small misc fixes

Same patch as on m-c.

Comment 63

[Daniel Coloma:dcoloma]

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

Comment 64

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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 65

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Note: Patch 6 does NOT like b2g18_v1_0_1.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 68

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 69

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 70

[Eric Chou [:ericchou] [:echou]]

(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?

Comment 71

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 72

[Gina Yeh [:gyeh] [:ginayeh]]

(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 :)

Comment 73

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 74

[Eric Chou [:ericchou] [:echou]]

(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. :)

Comment 75

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 1: r=mrbkap

Comment 76

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 2, r=mrbkap

Comment 77

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 3, r=mrbkap

Comment 78

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 4, r=mrbkap

Comment 79

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 5, r=mrbkap

Comment 80

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 6, r=mrbkap

Comment 81

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 7, r=mrbkap

Comment 82

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 8, r=mrbkap

Comment 83

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18_v1_0_1, Patch 9, r=mrbkap

Comment 84

[Ryan VanderMeulen [:RyanVM]]

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