Closed Bug 1248836 Opened 8 years ago Closed 8 years ago

HID Features Implementation

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
2.6 S10 - 3/25
Tracking Status
firefox48 --- fixed

People

(Reporter: lochang, Assigned: lochang)

References

Details

Attachments

(1 file, 3 obsolete files)

This bug covers HID features that are mandatory to pass PTS including Virtual_Unplug, Send_Report, Get_Report, Send_Data.
Assignee: nobody → lochang
Blocks: 915534
Depends on: 1223729
Hi Jocelyn,

Would you like to review the patch? This patch implements the HID feature functions including |VirtualUnplug()|, |GetReport()|, |SetReport()| and |SendData()| which are mandatory to pass the HID PTS. The PICS and hack for testing these function would be attach in Bug 1223729.
Attachment #8725510 - Flags: review?(joliu)
Comment on attachment 8725510 [details] [diff] [review]
Bug 1248836 - Patch(v1)  : HID Features Implementation

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

Please see my review comments below, thanks!

::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp
@@ +527,5 @@
> +  : public BluetoothHidResultHandler
> +{
> +public:
> +  VirtualUnplugResultHandler(BluetoothHidManager* aHidManager)
> +    : mHidManager(aHidManager)

Why is it needed?
Same question for other result handlers.

@@ +550,5 @@
> +
> +  if(!IsConnected()) {
> +    BT_LOGR("Device is not connected");
> +    return;
> +  }

I would suggest to define a macro for these few lines since it would be used in many functions below.

@@ +557,5 @@
> +
> +  if(!sBluetoothHidInterface) {
> +    BT_LOGR("sBluetoothHidInterface is null");
> +    return;
> +  }

DITTO. Suggest to define another macro for checking HidInterface.

@@ +584,5 @@
> +  BluetoothHidManager* mHidManager;
> +};
> +
> +void
> +BluetoothHidManager::GetReport(const BluetoothHidReportType& aRptType,

nit: I would prefer aReportType and aReportId, not a strong preference though.

@@ +585,5 @@
> +};
> +
> +void
> +BluetoothHidManager::GetReport(const BluetoothHidReportType& aRptType,
> +                               const uint8_t& aRptId,

I would suggest use pass by value for these primitive types (ex: uint8_t, uint16_t) here and below.

@@ +628,5 @@
> +  BluetoothHidManager* mHidManager;
> +};
> +
> +void
> +BluetoothHidManager::SetReport(const BluetoothHidReportType& aRptType,

DITTO.

@@ +671,5 @@
> +  BluetoothHidManager* mHidManager;
> +};
> +
> +void
> +BluetoothHidManager::SendData(const uint16_t& aDataLen, const char* aData)

uint8_t?
Attachment #8725510 - Flags: review?(joliu)
Hi Jocelyn,

> ::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp
> @@ +527,5 @@
> > +  : public BluetoothHidResultHandler
> > +{
> > +public:
> > +  VirtualUnplugResultHandler(BluetoothHidManager* aHidManager)
> > +    : mHidManager(aHidManager)
> 
> Why is it needed?
> Same question for other result handlers.

Sorry, it seems they are not used. I have removed them.

> @@ +550,5 @@
> > +
> > +  if(!IsConnected()) {
> > +    BT_LOGR("Device is not connected");
> > +    return;
> > +  }
> 
> I would suggest to define a macro for these few lines since it would be used
> in many functions below.

Ok! I have define macros for checking whether device is connected also hid interface is existed or not.


I have fixed the remaining problems mentioned in the comments. Please review again thank you!
Attached patch hid_other.patch (obsolete) — Splinter Review
Attachment #8725510 - Attachment is obsolete: true
Attachment #8726052 - Flags: review?(joliu)
Comment on attachment 8726052 [details] [diff] [review]
hid_other.patch

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

::: dom/bluetooth/bluedroid/BluetoothHidManager.cpp
@@ +552,5 @@
> +void
> +BluetoothHidManager::VirtualUnplug()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

nit: How about we remove some of these empty lines?
     Could you group some of the lines to avoid having too many empty lines?

One grouping example as below, no need to strictly follow this since it's just an example. 

MOZ_ASSERT(NS_IsMainThread());
ENSURE_HID_DEV_IS_CONNECTED;
MOZ_ASSERT(!mDeviceAddress.IsCleared());
ENSURE_HID_INTF_IS_EXISTED;

sBluetoothHidInterface->VirtualUnplug(
  mDeviceAddress, new VirtualUnplugResultHandler());

@@ +579,5 @@
> +                               const uint8_t aReportId,
> +                               const uint16_t aBufSize)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +

DITTO, and same as other functions.
Attachment #8726052 - Flags: review?(joliu) → review+
Ok, thanks for the reviewing.
Attachment #8726052 - Attachment is obsolete: true
Flags: needinfo?(shuang)
https://hg.mozilla.org/mozilla-central/rev/731fcd6ecd81
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S10 - 3/25
You need to log in before you can comment on or make changes to this bug.