HID Features Implementation

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lochang, Assigned: lochang)

Tracking

unspecified
2.6 S10 - 3/25
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
This bug covers HID features that are mandatory to pass PTS including Virtual_Unplug, Send_Report, Get_Report, Send_Data.
(Assignee)

Updated

3 years ago
Assignee: nobody → lochang
Blocks: 915534
(Assignee)

Updated

3 years ago
Depends on: 1223729
(Assignee)

Comment 1

3 years ago
Created attachment 8725510 [details] [diff] [review]
Bug 1248836 - Patch(v1)  : HID Features Implementation

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

Comment 3

3 years ago
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!
(Assignee)

Comment 4

3 years ago
Created attachment 8726052 [details] [diff] [review]
hid_other.patch
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+
(Assignee)

Comment 6

3 years ago
Created attachment 8727276 [details] [diff] [review]
Bug 1248836 - Patch(v2)  : HID Features Implementation

Ok, thanks for the reviewing.
Attachment #8726052 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8731547 [details] [diff] [review]
[Final]Bug 1248836 - HID Features Implementation, r=jocelyn
Attachment #8727276 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Flags: needinfo?(shuang)

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/731fcd6ecd81
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S10 - 3/25
Flags: needinfo?(shuang)
You need to log in before you can comment on or make changes to this bug.