This Bug covers Connect, Disconnect, ConnectStatusChanged. And the BTDaemon Part will file in another bug.

This patch includes 1. The prototype of BluetoothHidManager.cpp, BluetoothDaemonHidInterface.cpp 2. and modifications of other related files.

This patch includes basic HID connection/disconnection implementation.

Comment on attachment 8702184 [details] [diff] [review] Bug 1223729 - Patch1 (v1) : Prototype of HID Implementation Redirect the review process back to the tutor.

Comment on attachment 8702184 [details] [diff] [review] Bug 1223729 - Patch1 (v1) : Prototype of HID Implementation Review of attachment 8702184 [details] [diff] [review]: ----------------------------------------------------------------- Quick summary: 1) If you would like to split connect/disconnect into patch2, please keep all connection management stuff in patch2. 2) Please do not declare things/implement things that are actually unused. ::: dom/base/nsGkAtomList.h @@ +791,5 @@ > GK_ATOM(onhashchange, "onhashchange") > GK_ATOM(onheadphoneschange, "onheadphoneschange") > GK_ATOM(onheld, "onheld") > GK_ATOM(onhfpstatuschanged, "onhfpstatuschanged") > +GK_ATOM(onhidstatuschanged, "onhidstatuschanged") Move connection related stuff to patch2. ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.cpp @@ +189,5 @@ > + > +// Notifications > +// > + > +//Returns the current notification handler to a notification runnable a space between '/' and 'R'. ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.h @@ +201,5 @@ > + BluetoothAddress, BluetoothHidStatus, > + const BluetoothAddress&, BluetoothHidStatus> > + HandshakeNotification; > + > + //class ...Op; what's this? @@ +229,5 @@ > + DaemonSocketPDU& aPDU, > + DaemonSocketResultHandler* aRes); > + > + static BluetoothHidNotificationHandler* sNotificationHandler; > + remove this empty line. ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp @@ +12,5 @@ > #include "BluetoothDaemonAvrcpInterface.h" > #include "BluetoothDaemonCoreInterface.h" > #include "BluetoothDaemonGattInterface.h" > #include "BluetoothDaemonHandsfreeInterface.h" > +#include "BluetoothDaemonHidInterface.h" sort by alphabetical order. ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +112,5 @@ > + > +private: > + RefPtr<BluetoothProfileResultHandler> mRes; > + nsresult mRv; > +}; It's a bit weird that you implement result handlers in this patch without implementing those functions that might actually use these result handlers. Please only implement result handlers when you are actually implementing a function that will use it. ::: dom/bluetooth/bluedroid/BluetoothHidManager.h @@ +58,5 @@ > + RefPtr<BluetoothProfileController> mController; > + > + // > + // Self-Define functions > + // What does this mean? @@ +91,5 @@ > + const BluetoothAddress& aBdAddr, > + BluetoothHidStatus aStatus) override; > + void HandShakeNotification( > + const BluetoothAddress& aBdAddr, > + BluetoothHidStatus aStatus) override; Suggest to revise the private block as below: 1) class *ResultHandler; 2) functions 3) notification functions 4) member (ex:bool mHidConnected;) Besides, I don't see where're these member variables being used in cpp file. Please define members only when it will be used. And I suppose those override notification functions could be added until you're actually implementing them? @@ +96,5 @@ > +}; > + > +END_BLUETOOTH_NAMESPACE > + > +#endif // mozilla_dom_bluetooth_BluetoothHidManager_h mozilla_dom_bluetooth_bluedroid_BluetoothHidManager_h ::: dom/bluetooth/common/BluetoothCommon.h @@ +205,5 @@ > * dispatch one of the following events. > */ > #define A2DP_STATUS_CHANGED_ID "a2dpstatuschanged" > #define HFP_STATUS_CHANGED_ID "hfpstatuschanged" > +#define HID_STATUS_CHANGED_ID "hidstatuschanged" patch2. ::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp @@ +528,5 @@ > } else if (aData.name().EqualsLiteral(DEVICE_UNPAIRED_ID)) { > HandleDeviceUnpaired(aData.value()); > } else if (aData.name().EqualsLiteral(HFP_STATUS_CHANGED_ID) || > aData.name().EqualsLiteral(SCO_STATUS_CHANGED_ID) || > + aData.name().EqualsLiteral(HID_STATUS_CHANGED_ID) || patch 2. ::: dom/bluetooth/common/webapi/BluetoothAdapter.h @@ +94,3 @@ > IMPL_EVENT_HANDLER(a2dpstatuschanged); > IMPL_EVENT_HANDLER(hfpstatuschanged); > + IMPL_EVENT_HANDLER(hidstatuschanged); Move to patch2, please. ::: dom/bluetooth/moz.build @@ +15,5 @@ > 'common/BluetoothRilListener.cpp' > ] > > SOURCES += [ > +# 'common/BluetoothHidManager.cpp', Suggest to delete this line directly. BTW, is this file still needed? If this is needed for bluez, you could consider moving it into bluez folder. Otherwise, consider removing this file directly. ::: dom/webidl/BluetoothAdapter.webidl @@ +63,5 @@ > // Fired when handsfree connection status changed > attribute EventHandler onhfpstatuschanged; > > + // Fired when handsfree connection status changed > + attribute EventHandler onhidstatuschanged; Belongs to patch2.

Comment on attachment 8702186 [details] [diff] [review] Bug 1223729 - Patch2 (v1) : HID Connection/Disconnection Implementation Review of attachment 8702186 [details] [diff] [review]: ----------------------------------------------------------------- Please move all connection management stuff into this patch. Cancel the review flag first.

This patch fixed the problems mentions in the comments and move connection related parts to patch2(v2).

This patch adds the connection related parts which are misplaced in patch1(v1).

Comment on attachment 8708248 [details] [diff] [review] Bug 1223729 - Patch1 (v2) : Prototype of HID Implementation Review of attachment 8708248 [details] [diff] [review]: ----------------------------------------------------------------- Hi Louis, I think more parts should be implemented for adding HID support in gecko. Please see my comments below. ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.cpp @@ +25,5 @@ > +BluetoothDaemonHidModule::HandleSvc( > + const DaemonSocketPDUHeader& aHeader, DaemonSocketPDU& aPDU, > + DaemonSocketResultHandler* aRes) > +{ > +} HandleSvc and SetNotificationHandler could be done in the prototype patch. And I would also recommend you to implement HidModule and HidInterface completely in this patch. ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +34,5 @@ > +bool > +BluetoothHidManager::Init() > +{ > + return true; > +} I would suggest you should implement all the profile mgr initialization and uninitialization stuff(Init/Deinit/RegisterModule/UnregisterModule... ) in the prototype patch. ::: dom/bluetooth/bluedroid/BluetoothHidManager.h @@ +45,5 @@ > + class RegisterModuleResultHandler; > + class UnregisterModuleResultHandler; > + > + class ConnectResultHandler; > + class DisconnectResultHandler; patch2. @@ +50,5 @@ > + > + BluetoothHidManager(); > + bool Init(); > + void HandleShutdown(); > + void NotifyConnectionStateChanged(const nsAString& aType); patch2? @@ +58,5 @@ > + // > + > + void ConnectionStateNotification( > + const BluetoothAddress& aBdAddr, > + BluetoothHidConnectionState aState) override; patch2. ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +1995,5 @@ > BluetoothGattManager::DeinitGattInterface, > BluetoothAvrcpManager::DeinitAvrcpInterface, > BluetoothA2dpManager::DeinitA2dpInterface, > + BluetoothHfpManager::DeinitHfpInterface, > + BluetoothHidManager::DeinitHidInterface it should be in the opposite order to initialization. ::: dom/bluetooth/common/BluetoothCommon.h @@ +887,5 @@ > + uint16_t mProductId; > + uint16_t mVersion; > + uint8_t mCountryCode; > + uint16_t mDescriptorLength; > + uint8_t mDescriptorValue[884]; Define the maximum value as 884 and use it here. And add a comment to describe why 884 (ex: add descriptions from spec and specify the section in the spec.) above that definition. @@ +892,5 @@ > +}; > + > +struct BluetoothHidGetReportParam { > + uint16_t mReportLength; > + uint8_t mReportData[512]; DITTO. ::: dom/bluetooth/common/BluetoothInterface.h @@ +298,5 @@ > + BluetoothHidProtocolMode aProtocolMode); > + > + virtual void > + IdleTimeNotification(const BluetoothAddress& aBdAddr, > + BluetoothHidStatus aStatus, uint16_t aIdleTime); put uint16_t aIdleTime in a separate line to be consistent. @@ +303,5 @@ > + > + virtual void > + GetReportNotification(const BluetoothAddress& aBdAddr, > + BluetoothHidStatus aStatus, > + const BluetoothHidGetReportParam& aGetReportParam); In this case, I would prefer a more concrete name here instead of |*ReportParam|. For example, https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothCommon.h?case=true&from=dom%2Fbluetooth%2Fcommon%2FBluetoothCommon.h#715 @@ +383,5 @@ > + uint8_t aReportId, uint16_t aBuffSize, > + BluetoothHidResultHandler* aRes) = 0; > + virtual void SetReport(const BluetoothAddress& aBdAddr, > + BluetoothHidReportType aType, > + uint16_t aReportLen, uint8_t* aReportData, You could use the structure you defined for |GetReportNotification|. ::: dom/bluetooth/moz.build @@ +84,5 @@ > 'bluedroid/BluetoothDaemonInterface.cpp', > 'bluedroid/BluetoothDaemonSetupInterface.cpp', > 'bluedroid/BluetoothDaemonSocketInterface.cpp', > 'bluedroid/BluetoothGattManager.cpp', > + 'bluedroid/BluetoothHidManager.cpp', Suggest to move the original file to bluez folder and revise moz.build accordingly.

Comment on attachment 8708253 [details] [diff] [review] Bug 1223729 - Patch2 (v2) : HID Connection/Disconnection Implementation Review of attachment 8708253 [details] [diff] [review]: ----------------------------------------------------------------- Since you're going to combine these two patches, cancel the review request first.

Hi Jocelyn, This patch fixed the problems mentioned in the comments. And i also combined the patch1 & patch2 into this patch. Please review again thanks.

Comment on attachment 8709298 [details] [diff] [review] Bug 1223729 - Patch (v3) : HID Connection Implementation Review of attachment 8709298 [details] [diff] [review]: ----------------------------------------------------------------- https://bugzilla.mozilla.org/show_bug.cgi?id=1239979 has revised profile manager Init/Deinit, please check the changeset in that bug and apply them to your patch since those patches will be landed before your patch judging from its current status. ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.cpp @@ +127,5 @@ > + aSetInfoParam.mApplicationId, aSetInfoParam.mVendorId, > + aSetInfoParam.mProductId, aSetInfoParam.mVersion, > + aSetInfoParam.mCountryCode, aSetInfoParam.mDescriptorLength, > + PackArray<uint8_t>(aSetInfoParam.mDescriptorValue, > + aSetInfoParam.mDescriptorLength), *pdu); Suggest to add |PackPDU(const BluetoothHidSetInfoParam& aIn, DaemonSocketPDU& aPDU aPDU)| instead of manually writing all struct members here. BTW, please check if the change in DaemonSocketPDUHelpers.h is still needed after you revise the code here. ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.h @@ +76,5 @@ > + /* Report */ > + > + nsresult GetReportCmd(const BluetoothAddress& aBdAddr, > + BluetoothHidReportType aType, > + uint8_t aReportId, uint16_t aBuffSize, newline before |aBuffSize|. ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +16,5 @@ > +#include "mozilla/Services.h" > +#include "mozilla/StaticPtr.h" > +#include "MainThreadUtils.h" > +#include "nsIObserverService.h" > +#include "nsThreadUtils.h" I'm not sure all headers are necessary to be included, are they? For example, BluetoothTypes.h? @@ +38,5 @@ > + > +void > +BluetoothHidManager::Reset() > +{ > + mHidConnected = false; How about using |IsConnected()| directly? @@ +41,5 @@ > +{ > + mHidConnected = false; > + mController = nullptr; > + mConnectionState = HID_CONNECTION_STATE_DISCONNECTED; > + mPrevConnectionState = HID_CONNECTION_STATE_DISCONNECTED; unused member? @@ +137,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + if (sBluetoothHidInterface) { > + BT_LOGR("Bluetooth Hid interface is already initialized."); s/Hid/HID @@ +602,5 @@ > + OnConnect(EmptyString()); > + } else if (aState == HID_CONNECTION_STATE_DISCONNECTED) { > + mHidConnected = false; > + OnDisconnect(EmptyString()); > + } Are other states possible here? @@ +605,5 @@ > + OnDisconnect(EmptyString()); > + } > + > + NotifyConnectionStateChanged( > + NS_LITERAL_STRING(BLUETOOTH_HID_STATUS_CHANGED_ID)); In other profile managers, the sequence is 1. NotifyConnectionStateChanged 2. call onConnect/onDisconnect and notify profile controller Any particular reason to be inconsistent with other profile managers here? ::: dom/bluetooth/common/BluetoothCommon.h @@ +889,5 @@ > + uint16_t mVersion; > + uint8_t mCountryCode; > + uint16_t mDescriptorLength; > + /* Maximum descriptor length 884 defined in Bluez spec*/ > + uint8_t mDescriptorValue[884]; s/Bluez spec/BlueZ ipc spec Please also add the link to the line of this value defined in BlueZ ipc spec. @@ +898,5 @@ > + /** > + * Currently set maximum report length to 512 > + * and suppose it can suit all reports. > + */ > + uint8_t mReportData[512]; How do we decide this magic number? Have you check use cases to decide the number? ::: dom/bluetooth/common/BluetoothInterface.h @@ +289,5 @@ > + > + virtual void > + HidInfoNotification( > + const BluetoothAddress& aBdAddr, > + const BluetoothHidSetInfoParam& aSetInfoParam); Suggest to rename as 'BluetoothHidInfoParam' to fit both here and |SetInfo|. @@ +364,5 @@ > + > + /* Set Info */ > + > + virtual void SetInfo(const BluetoothAddress& aBdAddr, > + const BluetoothHidSetInfoParam& aSetInfoParam, DITTO. @@ +380,5 @@ > + /* Report */ > + > + virtual void GetReport(const BluetoothAddress& aBdAddr, > + BluetoothHidReportType aType, > + uint8_t aReportId, uint16_t aBuffSize, newline before uint16_t aBuffSize.

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #11) > I'm not sure all headers are necessary to be included, are they? > For example, BluetoothTypes.h? I will remove unnecessary header files. > @@ +38,5 @@ > > + > > +void > > +BluetoothHidManager::Reset() > > +{ > > + mHidConnected = false; > > How about using |IsConnected()| directly? I think |IsConnected()| is somehow used for determine whether the state is connected or not. Maybe directly set the mHidConnected to false inside |Reset()| is more proper? > @@ +41,5 @@ > > +{ > > + mHidConnected = false; > > + mController = nullptr; > > + mConnectionState = HID_CONNECTION_STATE_DISCONNECTED; > > + mPrevConnectionState = HID_CONNECTION_STATE_DISCONNECTED; > > unused member? I will remove it. > @@ +602,5 @@ > > + OnConnect(EmptyString()); > > + } else if (aState == HID_CONNECTION_STATE_DISCONNECTED) { > > + mHidConnected = false; > > + OnDisconnect(EmptyString()); > > + } > > Are other states possible here? Yes. But right now for connection/disconnection, i think we handle these two states is sufficient? > @@ +605,5 @@ > > + OnDisconnect(EmptyString()); > > + } > > + > > + NotifyConnectionStateChanged( > > + NS_LITERAL_STRING(BLUETOOTH_HID_STATUS_CHANGED_ID)); > > In other profile managers, the sequence is > 1. NotifyConnectionStateChanged > 2. call onConnect/onDisconnect and notify profile controller > > Any particular reason to be inconsistent with other profile managers here? My mistake for not noticing the sequence. I will move the |NotifyConnectionStateChanged()| prior to onConnect/onDisconnect. > @@ +898,5 @@ > > + /** > > + * Currently set maximum report length to 512 > > + * and suppose it can suit all reports. > > + */ > > + uint8_t mReportData[512]; > > How do we decide this magic number? > Have you check use cases to decide the number? Hmm... The magic number is decided by discussion with Shawn only. And it seems that there is no use cases for using |SetInfoCmd()| and |HidInfoNtf()| which will use the var.

(In reply to Louis Chang [:lochang] from comment #12) > (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #11) > > > @@ +38,5 @@ > > > + > > > +void > > > +BluetoothHidManager::Reset() > > > +{ > > > + mHidConnected = false; > > > > How about using |IsConnected()| directly? > > I think |IsConnected()| is somehow used for determine whether the state is > connected or not. > Maybe directly set the mHidConnected to false inside |Reset()| is more > proper? > Sorry, maybe I commented in a wrong place which introduces some misunderstanding. What I meant was why we need mHidConnected since we could always know the value by calling IsConnected()? > > @@ +602,5 @@ > > > + OnConnect(EmptyString()); > > > + } else if (aState == HID_CONNECTION_STATE_DISCONNECTED) { > > > + mHidConnected = false; > > > + OnDisconnect(EmptyString()); > > > + } > > > > Are other states possible here? > > Yes. But right now for connection/disconnection, i think we handle these two > states is sufficient? > So our expected behavior for other statuses is 'do nothing'? Suggest to add comments before this block to describe all the cases and corresponding behavior. > > @@ +898,5 @@ > > > + /** > > > + * Currently set maximum report length to 512 > > > + * and suppose it can suit all reports. > > > + */ > > > + uint8_t mReportData[512]; > > > > How do we decide this magic number? > > Have you check use cases to decide the number? > > Hmm... The magic number is decided by discussion with Shawn only. > And it seems that there is no use cases for using |SetInfoCmd()| and > |HidInfoNtf()| which will use the var. Hmm, that didn't actually answer my question. To me, we should have feasible reasons whenever we are defining maximum sizes. I checked other parts in our code base, there are also some places that we didn't enforce the maximum size yet and just uses nsTArray to store data at the first place and directly send PDU from b2g to bluetoothd though. I think it's a feasible way in the short term, but it might be better for us to define reasonable maximum sizes for all kinds of data buffer.

Hi Jocelyn, I have fixed the problems mentioned in the comments. And the new patch is based on the 7 patches in Bug 1239979. Please review again thanks.

Hi Jocelyn, Sorry of misunderstanding about the comment in last patch. I store the report data in nsTArray in this patch. Please review again thanks.

Comment on attachment 8710347 [details] [diff] [review] Bug 1223729 - Patch (v4) : HID Connection Implementation Review of attachment 8710347 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for my late response here. Please see my review comments below. And I think there's some logic error in both connect and disconnect cases, please revisit the logic of your patch. Thanks, Jocelyn ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp @@ +1528,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + return PackPDU( > + PackArray<uint8_t>(aIn.mDescriptorValue, aIn.mDescriptorLength), aPDU); I think we should check if mDescriptorLength exceeds the array size. Warning and return |NS_ERROR_ILLEGAL_VALUE| if it exceeds. @@ +1537,5 @@ > +{ > + uint8_t* reportData = > + const_cast<uint8_t*>(aIn.mReportData.Elements()); > + > + nsresult rv = PackPDU(aIn.mReportLength, aPDU); pack mReportData.Length() directly. @@ +2009,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + /* unpack descriptor value */ > + return aPDU.Read(aOut.mDescriptorValue, aOut.mDescriptorLength); Suggest to check the descriptor length before reading the value with the length. @@ +2016,5 @@ > +nsresult > +UnpackPDU(DaemonSocketPDU& aPDU, BluetoothHidReport& aOut) > +{ > + uint8_t* reportData = > + const_cast<uint8_t*>(aOut.mReportData.Elements()); remove this line. @@ +2024,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + /* unpack report data */ > + return aPDU.Read(reportData, aOut.mReportLength); Use https://dxr.mozilla.org/mozilla-central/source/ipc/hal/DaemonSocketPDUHelpers.h#797 instead. ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.cpp @@ +54,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<DaemonSocketPDU> pdu( > + new DaemonSocketPDU(SERVICE_ID, OPCODE_CONNECT, 6)); suggest to have a comment to describe what '6' means, i.e. Address @@ +75,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<DaemonSocketPDU> pdu( > + new DaemonSocketPDU(SERVICE_ID, OPCODE_DISCONNECT, 6)); DITTO. ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +34,5 @@ > + mConnectionState = HID_CONNECTION_STATE_DISCONNECTED; > +} > + > +bool > +BluetoothHidManager::Init() Could you help to point out where is it called? Thanks. @@ +161,5 @@ > + } > + return; > + } > + > + auto interface = btInf->GetBluetoothHidInterface(); rename as hidInterface @@ +500,5 @@ > + } > + > + mController = aController; > + > + sBluetoothHidInterface->Disconnect(mDeviceAddress, In your patch, I only see mDeviceAddress is set in |Connect|, what if it is an inbound connection? @@ +516,5 @@ > + */ > + NS_ENSURE_TRUE_VOID(mController); > + > + mController->NotifyCompletion(aErrorStr); > + mController = nullptr; why is it cleared after connection established? @@ +531,5 @@ > + */ > + NS_ENSURE_TRUE_VOID(mController); > + > + mController->NotifyCompletion(aErrorStr); > + mController = nullptr; Use |reset()| here? And where do we clear the deviceAddress when disconnect? @@ +587,5 @@ > +BluetoothHidManager::ConnectionStateNotification( > + const BluetoothAddress& aBdAddr, BluetoothHidConnectionState aState) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + remove this empty line. @@ +598,5 @@ > + > + /** > + * There are totally 10 Connection states, > + * and their corresponding behavior are listed below: > + * TODO: For now we only handle CONNECTED and DISCONNECTED states, remove 'TODO:' I suggest to remove this two lines since they're covered by below comments. @@ +603,5 @@ > + * and do nothing for rest of the states. > + * > + * CONNECTED: Device connected, call |OnConnect| > + * CONNECTING: > + * DISCONNECTED: Device disconnected call |OnDisConnect| OnDisconnect @@ +610,5 @@ > + * FAILED_KEYBOARD_FROM_HOST: > + * FAILED_TOO_MANY_DEVICES: > + * FAILED_NO_HID_DRIVER: > + * FAILED_GENERIC: > + * UNKNOWN: You could just write down CONNECTED and DISCONNECTED, then use others to cover the rest. /** * There are totally 10 Connection states, corresponding behavior are listed below: * * CONNECTED: Device ... * DISCONNECTED: Device ... * Others: do nothing */ ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +2640,5 @@ > NS_ENSURE_TRUE_VOID(a2dp); > a2dp->HandleBackendError(); > + BluetoothHidManager* hid = BluetoothHidManager::Get(); > + NS_ENSURE_TRUE_VOID(hid); > + hid->HandleBackendError(); We should also revise the comment above. ::: dom/bluetooth/common/BluetoothCommon.h @@ +901,5 @@ > + uint16_t mReportLength; > + /** > + * Currently use nsTArray to store report data. > + * TODO: Use a const. maximum size. > + */ Suggest to remove this comment. And since we uses nsTArray, we could use |Length()| directly instead of saved the length in another member. ::: ipc/hal/DaemonSocketPDUHelpers.h @@ +545,5 @@ > +inline nsresult > +PackPDU(const T1& aIn1, const T2& aIn2, const T3& aIn3, > + const T4& aIn4, const T5& aIn5, const T6& aIn6, > + const T7& aIn7, const T8& aIn8, const T9& aIn9, > + const T10& aIn10, DaemonSocketPDU& aPDU) What's this for?

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #16) Hi Jocelyn, > ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp > @@ +34,5 @@ > > + mConnectionState = HID_CONNECTION_STATE_DISCONNECTED; > > +} > > + > > +bool > > +BluetoothHidManager::Init() > > Could you help to point out where is it called? Thanks. Hmm... It seems it wasn't used. I will remove it. > @@ +500,5 @@ > > + } > > + > > + mController = aController; > > + > > + sBluetoothHidInterface->Disconnect(mDeviceAddress, > > In your patch, I only see mDeviceAddress is set in |Connect|, what if it is > an inbound connection? Sorry, It's my mistake. For inbound connection, i now first set the address value then call the |NotifyConnectionStateChanged|. > @@ +531,5 @@ > > + */ > > + NS_ENSURE_TRUE_VOID(mController); > > + > > + mController->NotifyCompletion(aErrorStr); > > + mController = nullptr; > > Use |reset()| here? > And where do we clear the deviceAddress when disconnect? Sorry, i forgot to clear it. I clear the address in |reset()| and change to call the |reset()| here. > ::: ipc/hal/DaemonSocketPDUHelpers.h > @@ +545,5 @@ > > +inline nsresult > > +PackPDU(const T1& aIn1, const T2& aIn2, const T3& aIn3, > > + const T4& aIn4, const T5& aIn5, const T6& aIn6, > > + const T7& aIn7, const T8& aIn8, const T9& aIn9, > > + const T10& aIn10, DaemonSocketPDU& aPDU) > > What's this for? It was originally used for struct |HidInfoParam|. But after removing the field |priority| in the struct, i forgot to remove this |PackPDU|. I have fixed the remaining problems mentioned in comments. Please review again thank you!

Comment on attachment 8715747 [details] [diff] [review] Bug 1223729 - Patch (v5) : HID Connection Implementation Review of attachment 8715747 [details] [diff] [review]: ----------------------------------------------------------------- Please see my questions below, thanks. ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp @@ +1521,5 @@ > > +nsresult > +PackPDU(const BluetoothHidInfoParam& aIn, DaemonSocketPDU& aPDU) > +{ > + if (aIn.mDescriptorLength > 884) { Please either use a pre-defined value or sizeof(BluetoothHidInfoParam::mDescriptorValue), instead of a hard coded value here. @@ +1522,5 @@ > +nsresult > +PackPDU(const BluetoothHidInfoParam& aIn, DaemonSocketPDU& aPDU) > +{ > + if (aIn.mDescriptorLength > 884) { > + NS_NOTREACHED("Invalid property for packing"); Why would we need to trigger a program failure in the debug build in this case? As for warning message, suggest to use MOZ_HAL_IPC_PACK_WARN_IF directly. @@ +2022,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + /* unpack descriptor value */ > + if (aOut.mDescriptorLength > 884) { DITTO. @@ +2023,5 @@ > + return rv; > + } > + /* unpack descriptor value */ > + if (aOut.mDescriptorLength > 884) { > + NS_NOTREACHED("Invalid property for packing"); DITTO. @@ +2026,5 @@ > + if (aOut.mDescriptorLength > 884) { > + NS_NOTREACHED("Invalid property for packing"); > + return NS_ERROR_ILLEGAL_VALUE; > + } > + return aPDU.Read(aOut.mDescriptorValue, aOut.mDescriptorLength); How about using UnpackArray helper? ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.cpp @@ +234,5 @@ > +} > + > +nsresult > +BluetoothDaemonHidModule::SendDataCmd( > + const BluetoothAddress& aRemoteAddr, uint16_t aDataLen, uint8_t* aData, const uint8_t* aData? @@ +369,5 @@ > + [OPCODE_SET_REPORT] = &BluetoothDaemonHidModule::SetReportRsp, > + [OPCODE_SEND_DATA] = &BluetoothDaemonHidModule::SendDataRsp > + }; > + > + MOZ_ASSERT(!NS_IsMainThread()); // IO thread @@ +380,5 @@ > + RefPtr<BluetoothHidResultHandler> res = > + static_cast<BluetoothHidResultHandler*>(aRes); > + > + if (!res) { > + return; // Return early if no result handler has been st for response set @@ +639,5 @@ > +/* Send Data */ > + > +void > +BluetoothDaemonHidInterface::SendData( > + const BluetoothAddress& aBdAddr, uint16_t aDataLen, uint8_t* Data, aData ::: dom/bluetooth/bluedroid/BluetoothDaemonHidInterface.h @@ +84,5 @@ > + BluetoothHidReportType aType, > + const BluetoothHidReport& aReport, > + BluetoothHidResultHandler* aRes); > + > + /* Send Data*/ nit: space after 'Data'. ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +17,5 @@ > +namespace { > + StaticRefPtr<BluetoothHidManager> sBluetoothHidManager; > + static BluetoothHidInterface* sBluetoothHidInterface = nullptr; > + bool sInShutdown = false; > +} //namesapce // namespace @@ +23,5 @@ > +const int BluetoothHidManager::MAX_NUM_CLIENTS = 1; > + > +BluetoothHidManager::BluetoothHidManager() > +{ > + Reset(); nit: How about BluetoothHidManager::BluetoothHidManager() : mConnectionState(HID_CONNECTION_STATE_DISCONNECTED) { } since this is the only one we need to initialize by ourselves in the constructor? @@ +179,5 @@ > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + NS_ENSURE_TRUE_VOID(obs); > + > + if (NS_FAILED(obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID))) { > + BT_WARNING("Failed to remove shutdown observe!"); observer @@ +374,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + mController->NotifyCompletion(NS_LITERAL_STRING(ERR_CONNECTION_FAILED)); > + What are the possible values of mConnectionState here? @@ +437,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + NS_ENSURE_TRUE_VOID(mController); > + > + mController->NotifyCompletion(NS_LITERAL_STRING(ERR_CONNECTION_FAILED)); I think we should null out the mController here. @@ +486,5 @@ > + } > + > + mController = aController; > + > + sBluetoothHidInterface->Disconnect(mDeviceAddress, We probably need a |MOZ_ASSERT(!mDeviceAddress.Clear())| before calling |Disconnect|, right? @@ +578,5 @@ > + > + mConnectionState = aState; > + > + /** > + * There are totally 10 Connection states, corresponding behavior are connection, behaviors @@ +583,5 @@ > + * listed below: > + * > + * CONNECTED: Device connected, call |OnConnect| > + * DISCONNECTED: Device disconnected call |OnDisconnect| > + * Others: Do nothing Is it OK that we do nothing for those error connection state? I suppose those are happened when connect/disconnect operations encounter error? Could you check the stack behavior to see when will they be triggered? Maybe we should call something like onConnectError/onDisconnectError for those cases? @@ +593,5 @@ > + OnDisconnect(EmptyString()); > + } > + > + NotifyConnectionStateChanged( > + NS_LITERAL_STRING(BLUETOOTH_HID_STATUS_CHANGED_ID)); This is called for every states, which contradicts with your comment? (do nothing for other states) Which one is your expect behavior? @@ +594,5 @@ > + } > + > + NotifyConnectionStateChanged( > + NS_LITERAL_STRING(BLUETOOTH_HID_STATUS_CHANGED_ID)); > + remove this redundant new line. ::: dom/bluetooth/common/BluetoothCommon.h @@ +889,5 @@ > + uint16_t mVersion; > + uint8_t mCountryCode; > + uint16_t mDescriptorLength; > + /** > + * Maximum descriptor length 884 defined in BlueZ ipc spec. nit: The maximum descriptor length defined in BlueZ ipc spec.

Hi Jocelyn, > ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp > @@ +1521,5 @@ > > > > +nsresult > > +PackPDU(const BluetoothHidInfoParam& aIn, DaemonSocketPDU& aPDU) > > +{ > > + if (aIn.mDescriptorLength > 884) { > > Please either use a pre-defined value or > sizeof(BluetoothHidInfoParam::mDescriptorValue), instead of a hard coded > value here. I have defined a value |BLUETOOTH_HID_MAC_DSC_LENGTH| as 884 in BluetoothCommon.h, and use it over here with |MOZ_HAL_IPC_PACK_WARN_IF| to check if length is too long. > @@ +23,5 @@ > > +const int BluetoothHidManager::MAX_NUM_CLIENTS = 1; > > + > > +BluetoothHidManager::BluetoothHidManager() > > +{ > > + Reset(); > > nit: How about > > BluetoothHidManager::BluetoothHidManager() > : mConnectionState(HID_CONNECTION_STATE_DISCONNECTED) > { > } > > since this is the only one we need to initialize by ourselves in the > constructor? Yep, I think it also work. > @@ +374,5 @@ > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + mController->NotifyCompletion(NS_LITERAL_STRING(ERR_CONNECTION_FAILED)); > > + > > What are the possible values of mConnectionState here? I think the possible value would be DISCONNECTED, it should originally keep in DISCONNECTED. But I reassign it to DISCONNECTED in case unpredictable error. > @@ +437,5 @@ > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + NS_ENSURE_TRUE_VOID(mController); > > + > > + mController->NotifyCompletion(NS_LITERAL_STRING(ERR_CONNECTION_FAILED)); > > I think we should null out the mController here. Yep, I missed it. > @@ +486,5 @@ > > + } > > + > > + mController = aController; > > + > > + sBluetoothHidInterface->Disconnect(mDeviceAddress, > > We probably need a |MOZ_ASSERT(!mDeviceAddress.Clear())| before calling > |Disconnect|, right? Ok, i add it in the front of the function. > @@ +583,5 @@ > > + * listed below: > > + * > > + * CONNECTED: Device connected, call |OnConnect| > > + * DISCONNECTED: Device disconnected call |OnDisconnect| > > + * Others: Do nothing > > Is it OK that we do nothing for those error connection state? > I suppose those are happened when connect/disconnect operations encounter > error? > Could you check the stack behavior to see when will they be triggered? > Maybe we should call something like onConnectError/onDisconnectError for > those cases? I have rewrite my comment about this function to make the flow more clear. And I have check HID_CONNECTION_STATE_FAILED_* states, it seems they aren't used in bluedroid stack. So i only handle connected/ing, disconnected/ing these 4 states for now. Please review again, thank you!

Comment on attachment 8721844 [details] [diff] [review] Bug 1223729 - Patch (v6) : HID Connection Implementation Review of attachment 8721844 [details] [diff] [review]: ----------------------------------------------------------------- Hi Louis, Almost there. Thanks a lot for your effort. Please see my comments below, thanks. ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp @@ +1522,5 @@ > +nsresult > +PackPDU(const BluetoothHidInfoParam& aIn, DaemonSocketPDU& aPDU) > +{ > + if (MOZ_HAL_IPC_PACK_WARN_IF( > + aIn.mDescriptorLength > BLUETOOTH_HID_MAX_DSC_LEN, nit: BLUETOOTH_HID_MAX_DESC_LEN @@ +2023,5 @@ > + rv = UnpackPDU(aPDU, aOut.mDescriptorLength); > + if (NS_FAILED(rv)) { > + return rv; > + } > + /* unpack descriptor value */ put this comment after checking descriptor length. ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +377,5 @@ > + mController->NotifyCompletion(NS_LITERAL_STRING(ERR_CONNECTION_FAILED)); > + > + mConnectionState = HID_CONNECTION_STATE_DISCONNECTED; > + mController = nullptr; > + mDeviceAddress.Clear(); Call Reset() instead, since it does the same thing? @@ +469,5 @@ > +BluetoothHidManager::Disconnect(BluetoothProfileController* aController) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(!mController); > + MOZ_ASSERT(!mDeviceAddress.IsCleared()); If current state is disconnected, we will hit this MOZ_ASSERT, right? @@ +593,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + BT_LOGR("state %d", aState); > + > + mConnectionState = aState; I would prefer a simple true/false value for connected and disconnected for now. Currently we only use these two states in this class, it looks like unnecessary to me to save those intermediate states. ::: dom/bluetooth/common/BluetoothCommon.h @@ +312,5 @@ > + * The maximum descriptor length defined in BlueZ ipc spec. > + * Please refer to http://git.kernel.org/cgit/bluetooth/bluez.git/tree/\ > + * android/hal-ipc-api.txt#n532 > + */ > +#define BLUETOOTH_HID_MAX_DSC_LEN 884 nit: BLUETOOTH_HID_MAX_DESC_LEN ::: dom/bluetooth/common/BluetoothInterface.h @@ +391,5 @@ > + > + /* Send Data */ > + > + virtual void SendData(const BluetoothAddress& aBdAddr, > + uint16_t aDataLen, uint8_t* aData, const uint8_t* aData? ::: dom/bluetooth/moz.build @@ +61,5 @@ > 'bluez/BluetoothA2dpManager.cpp', > 'bluez/BluetoothAvrcpManager.cpp', > 'bluez/BluetoothDBusService.cpp', > 'bluez/BluetoothHfpManager.cpp', > + 'bluez/BluetoothHidManager.cpp', Remember to do hg move HidManager.cpp/h in your patch and make sure your patch won't break bluez build.

Hi Jocelyn, > @@ +469,5 @@ > > +BluetoothHidManager::Disconnect(BluetoothProfileController* aController) > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(!mController); > > + MOZ_ASSERT(!mDeviceAddress.IsCleared()); > > If current state is disconnected, we will hit this MOZ_ASSERT, right? Yep, that's my mistake. I have fixed it. > @@ +593,5 @@ > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + BT_LOGR("state %d", aState); > > + > > + mConnectionState = aState; > > I would prefer a simple true/false value for connected and disconnected for > now. > Currently we only use these two states in this class, it looks like > unnecessary to me to save those intermediate states. I have used a boolean variable |mHidConnected| to maintain the hid manager behavior instead of using |mHidConnectionState|. > ::: dom/bluetooth/moz.build > @@ +61,5 @@ > > 'bluez/BluetoothA2dpManager.cpp', > > 'bluez/BluetoothAvrcpManager.cpp', > > 'bluez/BluetoothDBusService.cpp', > > 'bluez/BluetoothHfpManager.cpp', > > + 'bluez/BluetoothHidManager.cpp', > > Remember to do hg move HidManager.cpp/h in your patch and make sure your > patch won't break bluez build. Sure, thanks for the reminding. And i've checked the bluez build won't break. Thank you for the reviewing too. Please review agian, thanks a lot :)

Comment on attachment 8723892 [details] [diff] [review] Bug 1223729 - Patch (v7) : HID Connection Implementation Review of attachment 8723892 [details] [diff] [review]: ----------------------------------------------------------------- r=me after comments addressed, please ask a DOM peer to review your change under dom/webidl(e.g. BluetoothAdapter.webidl). ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp @@ +466,5 @@ > +BluetoothHidManager::Disconnect(BluetoothProfileController* aController) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(!mController); > + MOZ_ASSERT(mDeviceAddress.IsCleared()); If we calling disconnect for existing connections, we will always hit this MOZ_ASSERT, right? What I meant was we should do |MOZ_ASSERT(!mDeviceAddress.IsCleared())| after ERR_ALREADY_DISCONNECTED case. ex: https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp#516 @@ +572,5 @@ > + > +/** > + * There are totally 10 connection states, and will receive 4 possible > + * states: "connected", "connecting", "disconnected", "disconnecting". > + * Here we only handle connected and disconnected state. We do nothing states @@ +575,5 @@ > + * states: "connected", "connecting", "disconnected", "disconnecting". > + * Here we only handle connected and disconnected state. We do nothing > + * for remaining states. > + * > + * Possible connection cases are listed below: Possible cases @@ +577,5 @@ > + * for remaining states. > + * > + * Possible connection cases are listed below: > + * CONNECTED: > + * 1. Attempt connection success Suggest to combine two cases as "Successful inbound or outbound connection". @@ +580,5 @@ > + * CONNECTED: > + * 1. Attempt connection success > + * 2. Successful inbound connection > + * DISCONNECTED: > + * 3. Attempt disconnection success succeeded @@ +582,5 @@ > + * 2. Successful inbound connection > + * DISCONNECTED: > + * 3. Attempt disconnection success > + * 4. Attempt connection failed > + * 5. Either unpair from device or device is out of range in connected state the remote device @@ +591,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + BT_LOGR("state %d", aState); > + > + if (aState == HID_CONNECTION_STATE_CONNECTED ) { remove the extra space. @@ +592,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + BT_LOGR("state %d", aState); > + > + if (aState == HID_CONNECTION_STATE_CONNECTED ) { > + // case 1, 2: Update address and release controller I think the state should be clear enough, no need to write case 1-5 in your comment here and below.

Hello Blake, Would you please review the webapi change? This patch is to support bluetooth HID profile, which allows gecko connects to bluetooth keyboard/mouse/joystick. Thank you!

Set feature-b2g:2.6+ instead since HID is a required TV 2.6 feature.

That doesn't compile :( make[6]: *** No rule to make target `../../../dom/bluetooth/common/BluetoothHidManager.cpp', needed by `BluetoothHidManager.o'. Stop.

OK a clean build fixed it.

Hi Shawn, Would you like to review the patch? The patch touches the CLOBBER for moving dom/bluetooth/common/BluetoothHidManager.cpp and BluetoothHidManager.h to dom/bluetooth/bluez/ since now we have BluetoothHidManager for bluedroid stack.