Closed
Bug 1248836
Opened 9 years ago
Closed 9 years ago
HID Features Implementation
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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)
(deleted),
patch
|
Details | Diff | Splinter Review |
This bug covers HID features that are mandatory to pass PTS including Virtual_Unplug, Send_Report, Get_Report, Send_Data.
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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•9 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•9 years ago
|
||
Attachment #8725510 -
Attachment is obsolete: true
Attachment #8726052 -
Flags: review?(joliu)
Comment 5•9 years ago
|
||
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•9 years ago
|
||
Ok, thanks for the reviewing.
Attachment #8726052 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8727276 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(shuang)
Assignee | ||
Comment 9•9 years ago
|
||
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 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.
Description
•