1162902 - Implement PBAP SetPhoneBook function

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Implement SetPhoneBook function per Section 5.2, PBAP 1.2.

Comment 1

[Ben Tian (inactive)]

Attachment: WIP patch v1

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Implement PBAP SetPhoneBook function

Comment 3

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

Comment on attachment 8603990 [details] [diff] [review]
Patch 1 (v1): Implement PBAP SetPhoneBook function

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +289,5 @@
> +                                       const ObexHeaderSet& aHeader)
> +{
> +  nsString newPath = mCurrentPath;
> +
> +  // Section 5.2 "SetPhoneBook Function", PBAP 1.2

Please change comment style to multiline comment.

@@ +290,5 @@
> +{
> +  nsString newPath = mCurrentPath;
> +
> +  // Section 5.2 "SetPhoneBook Function", PBAP 1.2
> +  //

Here the comments seems to be strange, maybe I misunderstood the spec.

@@ +292,5 @@
> +
> +  // Section 5.2 "SetPhoneBook Function", PBAP 1.2
> +  //
> +  // Three cases:
> +  // 1) Go up 1 level   - flags bit 0 is 1

I checked PBAP 1.2 Section 5.2, Go up 1 level: both bit 0 and bit 1 are 1, and the name header is optional.

@@ +293,5 @@
> +  // Section 5.2 "SetPhoneBook Function", PBAP 1.2
> +  //
> +  // Three cases:
> +  // 1) Go up 1 level   - flags bit 0 is 1
> +  // 2) Go back to root - flags bit 0 is 0 AND name header is empty

Go back to root: bit 0 is 0, bit 1 is 1 AND name header is empty.

@@ +294,5 @@
> +  //
> +  // Three cases:
> +  // 1) Go up 1 level   - flags bit 0 is 1
> +  // 2) Go back to root - flags bit 0 is 0 AND name header is empty
> +  // 3) Go down 1 level - flags bit 0 is 0 AND name header is not empty,

bit 0 is 0, bit 1 is 1 and  name header is not empty

@@ +299,5 @@
> +  //                      where name header is the name of child folder
> +  if (flags & 1) {
> +    // Go up 1 level
> +    if (!newPath.IsEmpty()) {
> +      newPath = StringHead(newPath, newPath.RFindChar('/'));

I don't understand why do you set root path but the comment said 'Go up 1 level'?

@@ +319,5 @@
> +
> +  // Ensure the new path is legal
> +  if (!IsLegalPath(newPath)) {
> +    BT_LOGR("Illegal phone book path [%s]",
> +            NS_ConvertUTF16toUTF8(newPath).get());

If the SetPhoneBookPath is not legal, we shall reply Obex error code "NOT FOUND" response code "0xC4".

@@ +324,5 @@
> +    return false;
> +  }
> +
> +  mCurrentPath = newPath;
> +  BT_LOGR("[%s]", NS_ConvertUTF16toUTF8(mCurrentPath).get());

Please remove this line or print something more descriptive?

Comment 4

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> @@ +293,5 @@
> > +  // Section 5.2 "SetPhoneBook Function", PBAP 1.2
> > +  //
> > +  // Three cases:
> > +  // 1) Go up 1 level   - flags bit 0 is 1
> > +  // 2) Go back to root - flags bit 0 is 0 AND name header is empty
> 
> Go back to root: bit 0 is 0, bit 1 is 1 AND name header is empty.

All three cases has bit 2 is 1 so I omit it. Do you suggest to add following check before?

  if ((flags >> 1) != 1) {
    // bit 1: 1, bit 2~7: 0
    return ObexResponseCode::BadRequest;
  }

> @@ +299,5 @@
> > +  //                      where name header is the name of child folder
> > +  if (flags & 1) {
> > +    // Go up 1 level
> > +    if (!newPath.IsEmpty()) {
> > +      newPath = StringHead(newPath, newPath.RFindChar('/'));
> 
> I don't understand why do you set root path but the comment said 'Go up 1
> level'?

This is NOT root path but substring to the last "/". |RFindChar| gets the last index of "/" and |StringHead| returns the substring in |newPath| (start index=0, length=the last index of "/") as the 1-level up folder.

An example in HFP manager for reference.
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp#1510

> @@ +319,5 @@
> > +
> > +  // Ensure the new path is legal
> > +  if (!IsLegalPath(newPath)) {
> > +    BT_LOGR("Illegal phone book path [%s]",
> > +            NS_ConvertUTF16toUTF8(newPath).get());
> 
> If the SetPhoneBookPath is not legal, we shall reply Obex error code "NOT
> FOUND" response code "0xC4".

OK. I'll make this function return ObexResponseCode for reply.

> @@ +324,5 @@
> > +    return false;
> > +  }
> > +
> > +  mCurrentPath = newPath;
> > +  BT_LOGR("[%s]", NS_ConvertUTF16toUTF8(mCurrentPath).get());
> 
> Please remove this line or print something more descriptive?

Any suggestion on description? I think the following logs is descriptive enough since BT_LOGR shows function name.

05-18 23:02:43.366   206   206 I GeckoBluetooth: SetPhoneBookPath: [/telecom]
05-18 23:02:43.426   206   206 I GeckoBluetooth: SetPhoneBookPath: [/telecom/och]

Comment 5

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

(In reply to Ben Tian [:btian][PTO: 5/25~6/5] from comment #4)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> > @@ +293,5 @@
> > > +  // Section 5.2 "SetPhoneBook Function", PBAP 1.2
> > > +  //
> > > +  // Three cases:
> > > +  // 1) Go up 1 level   - flags bit 0 is 1
> > > +  // 2) Go back to root - flags bit 0 is 0 AND name header is empty
> > 
> > Go back to root: bit 0 is 0, bit 1 is 1 AND name header is empty.
> 
> All three cases has bit 2 is 1 so I omit it. Do you suggest to add following
> check before?
You mean bit 1 is 1 so you omit it?
I just thought it will be clear to have specification compliance comments.
> 
>   if ((flags >> 1) != 1) {
>     // bit 1: 1, bit 2~7: 0
>     return ObexResponseCode::BadRequest;
>   }
> 
Yes, that looks good to me.

> > @@ +299,5 @@
> > > +  //                      where name header is the name of child folder
> > > +  if (flags & 1) {
> > > +    // Go up 1 level
> > > +    if (!newPath.IsEmpty()) {
> > > +      newPath = StringHead(newPath, newPath.RFindChar('/'));
> > 
> > I don't understand why do you set root path but the comment said 'Go up 1
> > level'?
> 
> This is NOT root path but substring to the last "/". |RFindChar| gets the
> last index of "/" and |StringHead| returns the substring in |newPath| (start
> index=0, length=the last index of "/") as the 1-level up folder.
> 
> An example in HFP manager for reference.
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/hfp/
> BluetoothHfpManager.cpp#1510
> 
Oh! My bad! I misunderstood the code.

> > @@ +319,5 @@
> > > +
> > > +  // Ensure the new path is legal
> > > +  if (!IsLegalPath(newPath)) {
> > > +    BT_LOGR("Illegal phone book path [%s]",
> > > +            NS_ConvertUTF16toUTF8(newPath).get());
> > 
> > If the SetPhoneBookPath is not legal, we shall reply Obex error code "NOT
> > FOUND" response code "0xC4".
> 
> OK. I'll make this function return ObexResponseCode for reply.
> 
> > @@ +324,5 @@
> > > +    return false;
> > > +  }
> > > +
> > > +  mCurrentPath = newPath;
> > > +  BT_LOGR("[%s]", NS_ConvertUTF16toUTF8(mCurrentPath).get());
> > 
> > Please remove this line or print something more descriptive?
> 
> Any suggestion on description? I think the following logs is descriptive
> enough since BT_LOGR shows function name.
> 
> 05-18 23:02:43.366   206   206 I GeckoBluetooth: SetPhoneBookPath: [/telecom]
> 05-18 23:02:43.426   206   206 I GeckoBluetooth: SetPhoneBookPath:
> [/telecom/och]
How about this?
BT_LOGR("Current path: [%s]", NS_ConvertUTF16toUTF8(mCurrentPath).get());

Comment 6

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Implement PBAP SetPhoneBook function

Revise based on reviewer's comment.

Comment 7

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

Comment on attachment 8607348 [details] [diff] [review]
Patch 1 (v2): Implement PBAP SetPhoneBook function

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +220,5 @@
>  
>        ReplyToDisconnectOrAbort();
>        AfterPbapDisconnected();
>        break;
> +    case ObexRequestCode::SetPath: {

We probably should check receivedLength less than maximum total length here (MAX_PACKET_LENGTH) in the beginning of |ReceiveSocketData| .

@@ +223,5 @@
>        break;
> +    case ObexRequestCode::SetPath: {
> +        // Section 3.3.6 "SetPath", IrOBEX 1.2
> +        // [opcode:1][length:2][flags:1][contants:1][Headers:var]
> +        if (!ParseHeaders(&data[5], receivedLength - 5, &pktHeaders)) {

If the remote device sends illegal OBEX format, i'm afraid directly access data[5] is a bit dangerous--it's possible to hit SIGSEGV. It all depends on the behavior that remote sends the correct format obex packets. We shall check the |receivedLength| first before accessing data[5].

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8607348 [details] [diff] [review]
Patch 1 (v2): Implement PBAP SetPhoneBook function

I prefer to open another bug to fix comment 7 problems as they involve overall  structural change (including OPP manager) rather than SetPhonebook only.

Set r? again.

Comment 9

[Ben Tian (inactive)]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/5c0d1976af28

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c0d1976af28

Comment 12

[Ben Tian (inactive)]

Wesley, please help mark this bug feature-b2g:2.2r since BT feature PBAP depends on this bug.

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/b558a43ce91e