986375 - [Bluetooth] Define command strings in BluetoothHfpManager::SendCommand() as constant

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Per bug 981458 comment 10, we should define command strings in SendCommand() as constant to avoid bugs of string typo.

Comment 1

[Ben Tian (inactive)]

Jocelyn, please help on this bug. This is a good first bug to practice.

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v1) Bug 986375 : Define command strings for SendCommand

Command strings have been defined to avoid typo while calling BluetoothHfpManager::SendCommand().
Tested and passed TC_AG_PSI_BV_03_I PTS test case.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Sorry, I wrote the wrong test case name in previous comment.
The PTS test case was TC_AG_TWC_BV_01_I.

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8395567 [details] [diff] [review]
Patch1(v1) Bug 986375 : Define command strings for SendCommand

Only module owner (Eric) and peer (Gina) can review. Give f+ and set r? for Eric.

Comment 5

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

Comment on attachment 8395567 [details] [diff] [review]
Patch1(v1) Bug 986375 : Define command strings for SendCommand

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

Overall looks good! Please address the nit and request for review again.

Another thing I would like to point out is the patch summary. Since those constants are not only defined for SendCommand(), maybe "Use defined constants to represent HFP result codes to avoid typo" would be more clear.

::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
@@ +67,5 @@
> +#define CIND_RESPONSE      "+CIND: "
> +#define CLCC_RESPONSE      "+CLCC: "
> +#define BRSF_RESPONSE      "+BRSF: "
> +#define VGS_RESPONSE       "+VGS: "
> +#define CME_ERROR_RESPONSE "+CME ERROR: "

nit: I have a suggestion about the naming of these #defines. Personally I prefer using

  RESPONSE_CIEV, RESPONSE_CIND

more than

  CIEV_RESPONSE, CIND_RESPONSE

I have no strong reason but it's more like a convention.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch1(v2) Bug-986375 : Use defined constants for HFP result code to avoid typo

Hi Eric,
Thanks for the review comments.
I have addressed them in Patch1(v2).

Comment 7

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

Comment on attachment 8396899 [details] [diff] [review]
Patch1(v2) Bug-986375 : Use defined constants for HFP result code to avoid typo

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

Looks good to me. Thanks for cleaning this up.

Please obsolete your first patch, and be sure the commit message of this patch follows the committing rules before request for checkin-needed. (https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities)

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Patch1 Bug-986375 : Use defined constants for HFP result code to avoid typo

Comment 9

[Carsten Book [:Tomcat]]

welcome to mozilla! landed this on b2g-inbound https://hg.mozilla.org/integration/b2g-inbound/rev/9eff1a0854fb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please don't forget to clear checkin-needed when pushing. Or better yet, use mcMerge to mark them ;)

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9eff1a0854fb