1159179 - Implement PBAP connection related functions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

TODO
- Listen PBAP socket as PSE & accept incoming connection
- Reply to PBAP client connection/disconnection request

Comment 1

[Ben Tian (inactive)]

Attachment: WIP patch (v1)

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 1/3 (v1): Add OBEX related functions

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 2/3 (v1): Revise profile disconnection when BT stops

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 3/3 (v1): Implement PBAP manger

Comment 5

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

Comment on attachment 8599175 [details] [diff] [review]
Patch 1/3 (v1): Add OBEX related functions

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

Overall code looks good.

::: dom/bluetooth/ObexBase.cpp
@@ +27,5 @@
>    return headerLength;
>  }
>  
>  int
> +AppendHeader(uint8_t aHeaderId, uint8_t* aRetBuf, int aValue)

Do you mind adding one line comment to explain the purpose of this function? After renaming this function, it's a bit hard to understand the purpose.

::: dom/bluetooth/ObexBase.h
@@ +209,5 @@
>        if (mHeaders[i]->mId == ObexHeaderId::Body ||
>            mHeaders[i]->mId == ObexHeaderId::EndOfBody) {
> +        uint8_t* ptr = mHeaders[i]->mData.get();
> +        *aRetBody = new uint8_t[mHeaders[i]->mDataLength];
> +

nit:Remove this empty line.

@@ +229,3 @@
>          uint8_t* ptr = mHeaders[i]->mData.get();
> +        *aRetTarget = new uint8_t[mHeaders[i]->mDataLength];
> +

nit:Remove this empty line.

Comment 6

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

Comment on attachment 8599176 [details] [diff] [review]
Patch 2/3 (v1): Revise profile disconnection when BT stops

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +351,5 @@
> +    }
> +
> +    if (sProfiles[i]->IsConnected()) {
> +      sProfiles[i]->Disconnect(nullptr);
> +    } else if (!profileName.EqualsLiteral("OPP") &&

What would happen if someone modifies managers' name? Maybe it's not a real problem, but if the change happens it's not easy to find the side effects.

Comment 7

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> What would happen if someone modifies managers' name? Maybe it's not a real
> problem, but if the change happens it's not easy to find the side effects.

What do you mean by 'modify manager's name'? The name is queried on request [1] rather than a member variable. Also any suggestion? One way is to have empty |Reset| for OPP & PBAP managers.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothOppManager.h#46

Comment 8

[Ben Tian (inactive)]

Attachment: [final] Patch 1/3: Add OBEX related functions, r=shuang

Revise per reviewer's comment 5.

Comment 9

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

(In reply to Ben Tian [:btian] from comment #7)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > What would happen if someone modifies managers' name? Maybe it's not a real
> > problem, but if the change happens it's not easy to find the side effects.
> 
> What do you mean by 'modify manager's name'? The name is queried on request
> [1] rather than a member variable. Also any suggestion? One way is to have
> empty |Reset| for OPP & PBAP managers.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothOppManager.h#46

Keeping empty |Reset| seems to be trivial to me. Originally, I thought it's bad to check literal string (it depends on the assumption that no one is going to refactor the name) but now I think this is the easiest way. So please ignore my question.

Comment 10

[Ben Tian (inactive)]

Attachment: Patch 3/3 (v2): Implement PBAP manger

Revise |CompareHeaderTarget| function.

Comment 11

[Ben Tian (inactive)]

Attachment: Patch 3/3 (v3): Implement PBAP manger

Changes:
- add functions |AfterPbapConnected| and |AfterPbapDisconnected| to set/reset member variables
- revise logs to be clear

Comment 12

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

Comment on attachment 8603981 [details] [diff] [review]
Patch 3/3 (v3): Implement PBAP manger

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

Code looks good, and it's very similar with OPP manager. Some nits need your help.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +11,5 @@
> +#include "BluetoothSocket.h"
> +#include "BluetoothUuid.h"
> +#include "ObexBase.h"
> +
> +//#include "mozilla/dom/bluetooth/BluetoothTypes.h"

nit: Please remove this line.

@@ +163,5 @@
> +
> +  mServerSocket =
> +    new BluetoothSocket(this, BluetoothSocketType::RFCOMM, false, true);
> +
> +  BT_LOGR("PBAP");

nit: Maybe you can have just one single message with a descriptive string.

@@ +186,5 @@
> +  const uint8_t* data = aMessage->GetData();
> +  int receivedLength = aMessage->GetSize();
> +  uint8_t opCode = data[0];
> +
> +  BT_LOGR("> 0x%x", opCode);

If the opCode won't create much trouble, I prefer don't print it.

@@ +208,5 @@
> +
> +      ReplyToConnect();
> +      AfterPbapConnected();
> +      break;
> +

nit: Please remove this blank line.

@@ +222,5 @@
> +
> +      ReplyToDisconnectOrAbort();
> +      AfterPbapDisconnected();
> +      break;
> +

nit: Please remove this blank line.

@@ +231,5 @@
> +    case ObexRequestCode::SetPath:
> +      ReplyError(ObexResponseCode::BadRequest);
> +      BT_LOGR("Unsupported ObexRequestCode %x", opCode);
> +      break;
> +

nit: Please remove this blank line.

@@ +350,5 @@
> +
> +void
> +BluetoothPbapManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize)
> +{
> +  BT_LOGR("< 0x%x", aOpcode);

Do we really need to print opcode?

@@ +362,5 @@
> +  MOZ_ASSERT(aSocket);
> +  MOZ_ASSERT(aSocket == mServerSocket);
> +  MOZ_ASSERT(!mSocket);
> +
> +  BT_LOGR("PBAP");

Ditto.

Comment 13

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> @@ +186,5 @@
> > +  const uint8_t* data = aMessage->GetData();
> > +  int receivedLength = aMessage->GetSize();
> > +  uint8_t opCode = data[0];
> > +
> > +  BT_LOGR("> 0x%x", opCode);
> 
> If the opCode won't create much trouble, I prefer don't print it.
> 
> @@ +350,5 @@
> > +
> > +void
> > +BluetoothPbapManager::SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize)
> > +{
> > +  BT_LOGR("< 0x%x", aOpcode);
> 
> Do we really need to print opcode?

I keep them for later development debugging. I'll remove in next patch.

Comment 14

[Ben Tian (inactive)]

Attachment: [final] Patch 3/3: Implement PBAP manger, r=shuang

Comment 15

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

Comment on attachment 8604009 [details] [diff] [review]
[final] Patch 3/3: Implement PBAP manger, r=shuang

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +265,5 @@
> +
> +void
> +BluetoothPbapManager::AfterPbapConnected()
> +{
> +  mConnected = true;

Do you plan to add more pieces of code in the future? I guess one line function is not necessary.

@@ +271,5 @@
> +
> +void
> +BluetoothPbapManager::AfterPbapDisconnected()
> +{
> +  mConnected = false;

Ditto.

Comment 16

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #15)
> Do you plan to add more pieces of code in the future? I guess one line
> function is not necessary.

Yes, please refer to bug 1162902. I'll do some cleanup once all implementation are done.

Comment 17

[Ben Tian (inactive)]

ICS emulator fails to build since PBAP manager is unsupported for BlueZ. Revising patch and retry.

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

Comment 18

[Ben Tian (inactive)]

Attachment: [final] Patch 2/3: Revise profile disconnection when BT stops, r=shuang

Add build flag to build for BlueZ on ICS emulator.

Comment 19

[Ben Tian (inactive)]

Try with comment 18 patch. All builds pass.

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/83a9963ed83e
https://hg.mozilla.org/integration/b2g-inbound/rev/167fb3ad5f71
https://hg.mozilla.org/integration/b2g-inbound/rev/79e5169adc16

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/83a9963ed83e
https://hg.mozilla.org/mozilla-central/rev/167fb3ad5f71
https://hg.mozilla.org/mozilla-central/rev/79e5169adc16

Comment 22

[Ben Tian (inactive)]

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

Comment 23

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 1/3: Add OBEX related functions, r=shuang

Comment 24

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 2/3: Revise profile disconnection when BT stops, r=shuang

Comment 25

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 3/3: Implement PBAP manger, r=shuang

Comment 26

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/60060b904a77
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/72e8518103d4
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/e0c17434760f