1200124 - Correct PBAP order mapping

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

The order mapping doesn't follow section 6.2.1 PBAP spec 1.2.
  0x00 = indexed
  0x01 = alphanumeric
  0x02 = phonetic

Also replace nsString with uint32_t for parameter passing to avoid such string mismatch error.

Comment 1

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 1: Correct PBAP order mapping, r=shuang, r=mrbkap

Comment 2

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 2 (v1): Pass order and search key with uint32_t instead of nsString to avoid string mismatch error

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8654727 [details] [diff] [review]
[2.2r] Patch 2 (v1): Pass order and search key with uint32_t instead of nsString to avoid string mismatch error

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

Hi Ben,
Thanks for fixing this. Only one thing I think we shall handle it in BluetoothPbapManager instead of BluetoothAdapter. Do you agree?

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +1173,5 @@
>      } else if (name.EqualsLiteral("order")) {
> +      using namespace mozilla::dom::vCardOrderTypeValues;
> +      uint32_t order =
> +        value.get_uint32_t() < ArrayLength(strings) ? value.get_uint32_t()
> +                                                    : 0; // default: indexed

Honestly I think the error handling (here the unexpected value and use default value "0, indexed") shall be handled in BluetoothPbapManager, not BluetoothAdapter. BluetoothPbapManager shall know PBAP logic and BluetoothAdapter shall simply compose |BluetoothVCardListingEventInit| event. Doe it make sense to you?

@@ +1179,5 @@
> +    } else if (name.EqualsLiteral("searchKey")) {
> +      using namespace mozilla::dom::vCardSearchKeyTypeValues;
> +      uint32_t searchKey =
> +        value.get_uint32_t() < ArrayLength(strings) ? value.get_uint32_t()
> +                                                    : 0; // default: name

Ditto.

Comment 4

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> Honestly I think the error handling (here the unexpected value and use
> default value "0, indexed") shall be handled in BluetoothPbapManager, not
> BluetoothAdapter. BluetoothPbapManager shall know PBAP logic and
> BluetoothAdapter shall simply compose |BluetoothVCardListingEventInit|
> event. Doe it make sense to you?

I got your point. The reason I put it in BluetoothAdapter is to use the table stored in BluetoothPbapParameters.webidl. If the logic is in PBAP manager, PBAP manager has to either 1) keep an identical table or 2) access table in BluetoothPbapParameters.webidl to handle unexpected value.

Do you prefer option 2)? Also do you think we can remove the tables from BluetoothPbapParameters.webidl?

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Option 2) seems to be reasonable. If we removed tables (in this case, vCardOrderType/vCardSearchKeyType), we don't have dictionaries to let Gaia use, right? I think keeping the dictionaries and simply do type-conversion in BluetoothAdapter and do boundary check in BluetoothPbapManager.

Comment 6

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 2: Pass order and search key with uint32_t instead of nsString to avoid string mismatch error, r=shuang

Revise based on reviewer's comment.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8655867 [details] [diff] [review]
[2.2r] Patch 2: Pass order and search key with uint32_t instead of nsString to avoid string mismatch error, r=shuang

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

Code looks good to me. Thank.

Comment 8

[Ben Tian (inactive)]

Attachment: [m-c] Patch 1: Correct PBAP order mapping, r=shuang

Upload m-c patch.

Blake, can you review the webidl change that corrects order mapping to follow bluetooth Phonebook Access Profile (PBAP) spec as comment 0?

Comment 9

[Ben Tian (inactive)]

Attachment: [m-c] Patch 2: Pass order and search key with uint32_t instead of nsString to avoid string mismatch error, r=shuang

Comment 10

[Ben Tian (inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f777c9d940b0

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/fa0de2cded57
https://hg.mozilla.org/integration/b2g-inbound/rev/582d68dfb107

Comment 12

[Ben Tian (inactive)]

Set 2.2r+ blocker since the bug disobeys PBAP spec.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa0de2cded57
https://hg.mozilla.org/mozilla-central/rev/582d68dfb107

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/44ed5d3a9a19

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/4b50a718d0e6